KSqueezedTextLabel: Squeeze text when changing indent or margin

Authored by rkflx on Sep 24 2017, 8:24 PM.



When the label is not able to resize in a given layout and calls to
setIndent or setMargin further reduce the space available, the text
should be squeezed. Qt does not always send a resize event which would notify
us to squeeze the text. Therefore, these functions need
reimplementations adding manual calls to squeezeTextToLabel.

This fixes the autotest testChrome failing only on the CI (see T6982).

Test Plan
  • ksqueezedtextlabelautotest passes locally and with xvfb-run -s '-screen 0 800x600x24'
  • abi-compliance-checker suggests the changes are binary compatible

Diff Detail

R236 KWidgetsAddons
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
rkflx created this revision.Sep 24 2017, 8:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 24 2017, 8:24 PM
rkflx added a comment.Sep 24 2017, 8:25 PM

Ok, now we are back to the initial patch, which is probably fine.

@cfeck Do you have any issues with this, or would you also agree with this patch?

cfeck added a comment.Sep 25 2017, 1:34 AM

Qt does not send a resize event

But does it send QEvent::ContentsRectChange?

rkflx added a comment.Sep 25 2017, 4:10 PM
In D7977#148689, @cfeck wrote:

Qt does not send a resize event

But does it send QEvent::ContentsRectChange?

No, because this one is only emitted by setContentsMargin, but not by setMargin (read D7164#143540 (¹) again for what's the difference) or setIndent.

Even if there were resize related events (in some situations we even get some), we are not interested in them. The label should react by squeezing, not by adjusting size, as the detailed investigations in T6982#111367 conclude.

testChrome suggests to unify the behaviour of all its three chrome related calls. My patch is the most straightforward way to get there and I don't see how we could bolt on something convoluted through the event system without even reimplementing the setters to emit an otherwise missing event.

Let me know if you have any concerns against my approach, I'll gladly respond to them.

rkflx edited the summary of this revision. (Show Details)Sep 25 2017, 4:11 PM
rkflx added a comment.Sep 28 2017, 5:12 PM

As the CI keeps sending me mails with "BUILD UNSTABLE", I'd love to get this fix in.

If it takes you more time to think about this and we'll miss 5.39, that's fine too. Just tell me and I can at least disable the test in the meantime.

cfeck accepted this revision.Sep 28 2017, 5:40 PM

I have to admit that I feel not comfortable with overriding properties. But if this is the only way to fix unit tests (and those are supposed to reveal bugs in our code), then please commit it for the next frameworks release.

This revision is now accepted and ready to land.Sep 28 2017, 5:40 PM
This revision was automatically updated to reflect the committed changes.