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.
Details
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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.
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.)
Probably related to R32:3e530e78 (I guess Phabricator only adds "mentions" for shortlinks like Dxxx, but not full URLs?).
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....
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.
@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.
Yes, I would prefer the check condition. Feel free to commandeer; it looks like the original author didn't find time to further investigate.