KSqueezedTextLabel: call updateGeometry() when text changes
AcceptedPublic

Authored by brauch on Jul 30 2017, 9:30 PM.

Details

Reviewers
dfaure
cfeck
rkflx
Summary

By changing the full text, the sizeHint() of the widget changes. If the
text is currently squeezed, this however does not imply that the text
passed to QLabel::setText() changes (which is a no-up if not). Thus we need
to call updateGeometry() to notify the layout system about the size change.
Otherwise, the label misbehaves when you e.g. set the size policy to
Minimum and place it next to a spacer with Expanding policy, and then
change the text.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
brauch created this revision.Jul 30 2017, 9:30 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 30 2017, 9:30 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
aacid added a subscriber: aacid.Jul 30 2017, 9:50 PM

do we have autotests for this? Can they be added?

With enough dedication, probably yes ...

cfeck requested changes to this revision.Aug 3 2017, 3:47 PM

The title says "when text changes", but you call it even if the text does not change. Note that squeezeTextToLabel() is called from many places, including resizeEvent(), so avoid calling updateGeometry() if not needed.

This revision now requires changes to proceed.Aug 3 2017, 3:47 PM
brauch updated this revision to Diff 17638.Aug 3 2017, 4:19 PM
brauch edited edge metadata.

Right. Better call it here.

dhaumann added a subscriber: dhaumann.

Add Henrik, since he has several patches in the pipe as well...

rkflx edited edge metadata.Aug 10 2017, 4:57 PM

Looking at the Qt docs, we see:

Call QWidget::updateGeometry() whenever the size hint, minimum size hint or size policy changes. This will cause a layout recalculation.

To decide whether this is also meaningful for a squeezable label, a testcase would be good, though. Might even be a totally different issue.

In fact, I tried adding a test for this when implementing the autotests in D7163 last week already, but got stuck:

  • What is the size policy of the containing widget?
  • How to enter the initial squeezed state?
  • If the text is already squeezed, is the label actually supposed to change geometry if the text changes?
  • What is exactly misbehaving, i.e. what is the expected and what is the observed behaviour?
  • How to test updateGeometry() (vs. adjustSize(), which seems to call sizeHint() directly)?

Not sure if this helps, but hopefully we'll figure it out eventually.

TL;DR: Could not reproduce, but fix might still be correct.

I can write a test if you think this helps. I think reading the docs it is quite clear we must call updateGeometry() here: our sizeHint() changes when changing the text.

Are you sure you are calling updateGeometry() in the right place and that there are no other places where it should be called? Having a test case clearly demonstrating the connection between the docs quote and your last sentence of the summary would be reassuring not only for your reviewers, but also future contributors working on KSqueezedTextLabel and wondering about the call.

So, if you already have a case where this breaks for you, extracting a test would be great. Please rebase and "Depend on" D7164, if possible at all.

(OTOH, I'm not really an expert in this area. If someone more experienced than me is willing to accept this without an autotest, that's fine with me too.)

rkflx added a comment.Aug 20 2017, 2:44 PM

Probably related to R32:3e530e78 (I guess Phabricator only adds "mentions" for shortlinks like Dxxx, but not full URLs?).

rkflx resigned from this revision.Aug 24 2018, 10:46 PM
dfaure accepted this revision.Aug 25 2018, 11:37 AM
dfaure added a subscriber: dfaure.

Makes sense to me, actually. A unittest is difficult to write but an interactive test program would be an easy way to see the difference that this makes.

Henrik: everything OK? I see you unsubscribed from a number of phabricator requests....

brauch added a comment.Sep 5 2018, 7:48 AM

For the record, I tried writing a test for this but didn't succeed and eventually put it aside, although the difference is easily visible in a test application. There must be a reason why the naive test case behaves differently from an interactive application ... I could take another look, I guess.

dhaumann added a subscriber: rkflx.Sep 5 2018, 6:53 PM

@dfaure: writing "Henrik" when he already unsubscribeb will never reach him. Ping @rkflx

@cfeck Can you review this again?

I guess this could be improved with if (text == d->fullText) return; but the patch is already an improvement as is.

cfeck resigned from this revision.Nov 24 2019, 12:49 PM

Yes, I would prefer the check condition. Feel free to commandeer; it looks like the original author didn't find time to further investigate.

This revision is now accepted and ready to land.Nov 24 2019, 12:49 PM