| dhaumann |
| Frameworks |
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
Tests pass, except where marked as QEXPECT_FAIL.
| No Linters Available |
| No Unit Test Coverage |
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
| autotests/ksqueezedtextlabelautotest.cpp | ||
|---|---|---|
| 39 ↗ | (On Diff #17775) | 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". |
| 46 ↗ | (On Diff #17775) | Use QString() instead of QStringLiteral("") for empty strings. |
| 140 ↗ | (On Diff #17775) | Same here: Prefer QString() over QStringLiteral("") |
| 217–234 ↗ | (On Diff #17775) | 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... Example: |
| autotests/ksqueezedtextlabelautotest.h | ||
| 35–36 ↗ | (On Diff #17775) | 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 :-) |
| autotests/ksqueezedtextlabelautotest.cpp | ||
|---|---|---|
| 217–234 ↗ | (On Diff #17775) | 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. |
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 ;)
Address comments:
| autotests/ksqueezedtextlabelautotest.cpp | ||
|---|---|---|
| 217–234 ↗ | (On Diff #17775) | 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 |