D7010: KSqueezedTextLabel: call updateGeometry() when text changes
cfeck resigned from this revision. cfeck added a comment. This revision is now accepted and ready to land. Yes, I would prefer the check condition. Feel free to commandeer; it looks like the original author didn't find time to further investigate. REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, dfaure Cc: rkflx, dfaure, dhaumann, aacid, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
dfaure added a comment. @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. REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, dfaure Cc: rkflx, dfaure, dhaumann, aacid, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
dhaumann added a subscriber: rkflx. dhaumann added a comment. @dfaure: writing "Henrik" when he already unsubscribeb will never reach him. Ping @rkflx REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, dfaure Cc: rkflx, dfaure, dhaumann, aacid, #frameworks, michaelh, ngraham, bruns
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
brauch added a comment. 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. REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, dfaure Cc: dfaure, dhaumann, aacid, #frameworks, michaelh, ngraham, bruns
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
dfaure accepted this revision. dfaure added a comment. 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 REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, dfaure Cc: dfaure, dhaumann, aacid, #frameworks, michaelh, ngraham, bruns
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
rkflx resigned from this revision. REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck Cc: dhaumann, aacid, #frameworks, michaelh, ngraham, bruns
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
rkflx added a comment. Probably related to https://phabricator.kde.org/R32:3e530e780b596e9505b66e1ae7b1044d335adb07 (I guess Phabricator only adds "mentions" for shortlinks like Dxxx, but not full URLs?). REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, rkflx Cc: dhaumann, aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
rkflx added a comment. 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" https://phabricator.kde.org/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.) REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, rkflx Cc: dhaumann, aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
brauch added a comment. 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. REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, rkflx Cc: dhaumann, aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
rkflx added a comment. 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 https://phabricator.kde.org/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. REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, rkflx Cc: dhaumann, aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
dhaumann added a reviewer: rkflx. dhaumann added a comment. Add Henrik, since he has several patches in the pipe as well... REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck, rkflx Cc: dhaumann, aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
brauch updated this revision to Diff 17638. brauch added a comment. Right. Better call it here. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7010?vs=17411=17638 REVISION DETAIL https://phabricator.kde.org/D7010 AFFECTED FILES src/ksqueezedtextlabel.cpp To: brauch, cfeck Cc: aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
cfeck requested changes to this revision. cfeck added a comment. This revision now requires changes to proceed. 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. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck Cc: aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
brauch added a comment. With enough dedication, probably yes ... REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck Cc: aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
aacid added a comment. do we have autotests for this? Can they be added? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7010 To: brauch, cfeck Cc: aacid, #frameworks
D7010: KSqueezedTextLabel: call updateGeometry() when text changes
brauch created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION 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. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7010 AFFECTED FILES src/ksqueezedtextlabel.cpp To: brauch, cfeck Cc: #frameworks
