KSqueezedTextLabel: Add several autotests

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



Tests most of the basic functionality and uncovers bugs for a few edge
cases. Testing some of the advanced features is skipped for now, there
might be more bugs lurking.

Checking the text elision in relation to font metrics is already being
covered by Qt's autotests.

Depends on D7162

Test Plan

Tests pass, except where marked as QEXPECT_FAIL.

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.Aug 6 2017, 12:33 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 6 2017, 12:33 PM

I think this is already a very good patch. I just have some minor comments. Could you have another look?

By the way: Given you have more patches that build on this one, you should consider applying for a KDE developer account, if you don't have one yet, see: https://community.kde.org/Infrastructure/Get_a_Developer_Account


To me, it was not immediately clear whether amount referts to pixel or to characters. So you may want to rename the parameter to pixelAmount or pxAmout or amountInPixels, or maybe even simply "pixels".


Use QString() instead of QStringLiteral("") for empty strings.


Same here: Prefer QString() over QStringLiteral("")


margin, indent, and lineWidth are all Q_PROPERTYs, maybe you can use QObject::setProperty(const char *, QVariant) to set the values instead of using a function pointer? The function pointer is a bit ugly...

label->setProperty("indent", 40); // will automatically call label->setIndent(40);


Given you don't use any member variables, these could also be free functions in e.g. an anonymous namespace in the .cpp file. This is just a comment though, no strict requirement. I guess it is fine as is :-)

This comment was removed by dhaumann.
dhaumann added inline comments.Aug 6 2017, 9:09 PM

Looking at your other change requests: You may want to ignore the note about using setProperty(), since this will call QLabel::setIndent(), and not the one that you add in KSqueezedTextLabel (since setIndent() etc. are not virtual). Maybe this needs further discussion to find the best solution.

rkflx added a comment.Aug 10 2017, 4:50 PM

I think this is already a very good patch. I just have some minor comments.

Thanks for looking at all these patches and taking the time for detailed feedback. This is very helpful and really appreciated!

you should consider applying for a KDE developer account

Will do shortly (apply, not consider ;)

rkflx updated this revision to Diff 17971.Aug 10 2017, 4:52 PM

Address comments:

  • anonymous namespace
  • QString() instead of QStringLiteral("")
  • "pixels" for "amount"
  • setProperty() instead of pointer to member function, remove switch and enum
rkflx marked 5 inline comments as done.Aug 10 2017, 4:53 PM
rkflx added inline comments.

I like the you suggestion nevertheless. For D7164, we could just set indent, margin and lineWidth as Q_PROPERTIES again. According to [1], this should be BC. This would bring the set of properties of KSqueezedTextLabel more in line with those of QLabel and QFrame, too.

[1] https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts

rkflx updated this revision to Diff 19331.Sep 9 2017, 9:40 AM
  • wait for resize event after setting properties as discussed in D7164#143540
  • prevent leaks
dhaumann accepted this revision.Sep 10 2017, 1:24 PM
This revision is now accepted and ready to land.Sep 10 2017, 1:24 PM
This revision was automatically updated to reflect the committed changes.