KSqueezedTextLabel: Respect indent, margin and frame width
ClosedPublic

Authored by rkflx on Aug 6 2017, 12:34 PM.

Details

Summary

Text could display cut off when setting indent, margin and/or frame of
the label. On top of that, even when following the size hint to display
the complete text, text could be elided.

This is fixed by taking all chrome into account when determining elision
width and when returning the size hint.

Depends on D7163

Test Plan

Previously expect-failing autotests now pass.

Diff Detail

Repository
R236 KWidgetsAddons
Branch
arcpatch-D7164 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx created this revision.Aug 6 2017, 12:34 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 6 2017, 12:34 PM

This patch adds four functions that all "reimplement" functions that exist in base classes.
However, these functions are not virtual. As such, adding the functions is probably fine (BC), but essentially the correct behavior now depends on calling the correct function (namely the ones from KSqueezedTextLabel).

This by itself is a bit unfortunate, since it opens doors for bugs. This, however, is not properly fixable for KF 5 / Qt 5, so I guess this approach is fine.

The functions itself are ok, but I think the remark "Reimplementation of... but see setText() for additional remark" is a bit fuzzy. I would prefer to copy the "@warning This function in the base class is not virtual. Therefore make sure to call this function" or some similar text. This would be much more explicit. And again, I would prefer having such a remark also in the detailed description of the class itself.

Any other comments would be very much appreciated :-)

Finally, would that be fixable for Qt 6? Maybe instead of deriving from QLabel, derive from QWidget and then *use* a QLabel internally as member variable? Nothing for this patch, but still maybe worth to think about. If we have a proper solution, we should add a comment like // TODO: KF6 ...

Some thoughts, inconclusive though:

Another idea: Sometimes additional functionality for non-virtual functions is provided by hooking into the changeEvent(). However, for our use case we won't get such events from QLabel.

Things we might want to consider when going with aggregation instead of inheritance regarding QLabel (which seems like the best solution so far):

  • Are there APIs or legacy users which can only take QLabels? (How to check without manually analyzing lxr search results?)
  • Which functions of QLabel and QFrame should we reimplement? (What is used? What semantics do not make sense? Also see TODO in autotest.)

Ideal solution: Add virtual in Qt itself or make it a signal. Should we try and ask for Qt6? It has been done before: https://codereview.qt-project.org/#/c/111849/

I wonder if there is a general best practice solution for this issue in KF5, e.g. when looking at the Qt4 → Qt5 BIC transition?

rkflx updated this revision to Diff 17972.Aug 10 2017, 4:56 PM

Same wording for reimplementation warning as in D7161.

Make indent, margin and lineWidth Q_PROPERTIES.

Add link to phabricator discussion as KF6 TODO (not sure if we can reach a conclusion yet, so better than nothing).

dhaumann added a subscriber: cfeck.Aug 22 2017, 8:14 PM

To me this looks good.

It seems Q_PROPERTYs can be overridden (at least, this follows imho from "The presence of the FINAL attribute indicates that the property will not be overridden by a derived class" from http://doc.qt.io/qt-5/properties.html). That we should be fine.

@cfeck A second review would be appreciated.

rkflx added a reviewer: cfeck.Sep 5 2017, 10:38 PM

@cfeck Friendly ping :)
Please let me know if you have any questions or someone else should review this. We really don't want to BIC-break a Tier 1 framework…

cfeck edited edge metadata.Sep 6 2017, 1:37 AM

I probably miss the big picture here. Why are these attributes needed? The lineWidth attribute, for example, looks like a thing from the past, where you could control the thickness of frames (CDE vs. Motif style).

cfeck added a comment.Sep 6 2017, 1:51 AM

QLabel sends out a QEvent::ContentsRectChange event when the margins change or the frame style causes a size change. Could this be catched (using an event filter) instead of reimplementing the properties?

rkflx planned changes to this revision.Sep 6 2017, 5:32 PM

Thanks for looking at this so quickly, Christoph! I believe you inspired me to find a good solution. I'll submit updates over the weekend.

If you are interested in the backstory, read on:

I probably miss the big picture here. Why are these attributes needed? The lineWidth attribute, for example, looks like a thing from the past, where you could control the thickness of frames (CDE vs. Motif style).

These are public API of KSqueezedLabel (via QLabel), thus any bug in them should be fixed. The main motivation was D6696#130719 for setMargin, but I don't see a reason why we should leave the rest buggy if the fix is just the same.

QLabel sends out a QEvent::ContentsRectChange event when the margins change or the frame style causes a size change. Could this be catched (using an event filter) instead of reimplementing the properties?

My first reaction was: "Sadly there is no such event to be observed.", because that's what I concluded in D7164#134363 and observed again today (e.g. for setMargin, KSqueezedTextLabel::resizeEvent was never called, so the duplication of squeezeTextToLabel in setMargin was needed). Then I thought: "Surely, such behaviour must be a bug in Qt." (¹). After playing around some more, I now think the issue is just a misunderstanding in the autotests (see D7163): If I remove squeezeTextToLabel from setMargin, then

label->setProperty(attribute.toLatin1().data(), amount);
QVERIFY(label->isSqueezed());

fails for ksqueezedtextlabelautotest testChrome:margin, while

label->setProperty(attribute.toLatin1().data(), amount);
QTest::qWaitForWindowExposed(label);
QVERIFY(label->isSqueezed());

passes. This means I can rip out all three property reimplementations (²) and we should be good to go (³). Next challenge: Finding a new reviewer for D6696.


(¹) E.g. there's a difference between setMargin and setContentsMargin: The former is between text and (Q)frame aka "inside", the latter is between (Q)frame and the surrounding widgets aka "outside". I suspected both behaved differently regarding event emission.

(²) Now not needed anymore, but I'm curious anyway: Do you know of any tool available to help check BC automatically?

(³) I'll also change contentsRect to be public (it's a reimplementation of a public function, after all) and update the @since to 5.39.

rkflx updated this revision to Diff 19332.Sep 9 2017, 9:43 AM
rkflx edited the summary of this revision. (Show Details)
  • remove custom setters for lineWidth, margin and indent and do not re-mark them as properties, i.e. no more uncertainty about BIC (not needed anymore after fixing autotest in D7163)
  • make contentsRect() public
  • bump @since

@dhaumann: I think this and D7163 are ready now. Could you review again, please?

Looks good to me, except for the licensing issue. I am not really an expert here, but LGPLv3 to LGPLv2 only is problematic, as far as I know. Do you see a good solution?

src/ksqueezedtextlabel.cpp
128

Hm, this possibly is a problem: This file is licensed LGPLv2 only.
Qt is licensed under LGPLv3, as far as I know, so you cannot copy the code.

In an ideal world, you now would forget everything you have seen and write this code again :-)
In practice, the result often looks more or less the same.

So if you can claim the following code comes from you, then I think we are on the safe side...

rkflx updated this revision to Diff 19345.Sep 9 2017, 10:03 PM
rkflx edited the summary of this revision. (Show Details)

Spent some time trying to solve this properly, but it's difficult.

We could get the Qt code as LGPLv2.1 from an older release, and relicensecheck.pl for ksqueezedtextlabel.{cpp,h} on kwidgetsaddons and kdelibs (including moves) doesn't look too bad for "lgplv23 lgplv2+". However, technically we would need to relicense to either "v3" or "v2.1", but the script only asks for "v2 or v3" and "v2+". Thus, no luck.

Therefore, I opened the Qt API docs to check whether a reimplementation without looking at the code is possible, which states already quite algorithmically:

If indent is negative, or if no indent has been set, the label computes the effective indent as follows: If frameWidth() is 0, the effective indent becomes 0. If frameWidth() is greater than 0, the effective indent becomes half the width of the "x" character of the widget's current font(). By default, the indent is -1, meaning that an effective indent is calculating in the manner described above.

The implementation I'm proposing (as of now :) should be doing exactly that, of course with adaptations to also account for margin and lineWidth. Let me know if this is enough.

dhaumann accepted this revision.Sep 10 2017, 1:15 PM

I think this is good enough, especially since according to your research the previous code also exists in certain Qt releases also as LGPLv2.

This revision is now accepted and ready to land.Sep 10 2017, 1:15 PM
This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Sep 10 2017, 8:03 PM

Thank you so much for your help until now, Dominik! Unfortunately, there's one more thing: The CI does not like testChrome :( Could you tell me how the workflow is now?

  • Should this be discussed here? New review request? Just commit?
  • What's the timeframe I'm expected to solve this?
  • Is there any way I could test whether the CI likes my patch before committing?
  • How can I recreate the conditions in the CI regarding showing windows, rendering and resize events locally?

I just built with Qt 5.6, but this is also fine. Any ideas what to check next?

dfaure added a subscriber: dfaure.Oct 16 2017, 8:03 AM

Looks like the CI issue is gone?
https://build.kde.org/job/Frameworks%20kwidgetsaddons%20kf5-qt5%20XenialQt5.7/ looks green to me (well, blue, the jenkins people got daltonism issues lately).

rkflx added a comment.Oct 16 2017, 8:10 AM

Looks like the CI issue is gone?

Thanks for caring, David. Maybe I could have made this more clear, but work regarding the CI failure is tracked in T6982, were I solved the problem and marked it as done. (It has some minor additional action items though, so it does not show up as closed yet.)