D7161: KSqueezedTextLabel: Small improvements to API docs
rkflx added a comment. > Can you commit yourself? Now I can :) (Thanks for your support with that!) Will commit once all four patches have been accepted. REPOSITORY R236 KWidgetsAddons BRANCH arcpatch-D7161 REVISION DETAIL https://phabricator.kde.org/D7161 To: rkflx, #frameworks, dhaumann Cc: dhaumann
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
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
D7164: KSqueezedTextLabel: Respect indent, margin and frame width
rkflx updated this revision to Diff 17972. rkflx added a comment. Same wording for reimplementation warning as in https://phabricator.kde.org/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). REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7164?vs=17776=17972 BRANCH arcpatch-D7164 (branched from master) REVISION DETAIL https://phabricator.kde.org/D7164 AFFECTED FILES autotests/ksqueezedtextlabelautotest.cpp src/ksqueezedtextlabel.cpp src/ksqueezedtextlabel.h To: rkflx, #frameworks Cc: dhaumann
D7164: KSqueezedTextLabel: Respect indent, margin and frame width
rkflx added a comment. 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? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7164 To: rkflx, #frameworks Cc: dhaumann
D7163: KSqueezedTextLabel: Add several autotests
rkflx added inline comments. INLINE COMMENTS > dhaumann wrote in ksqueezedtextlabelautotest.cpp:217-234 > 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 like the you suggestion nevertheless. For https://phabricator.kde.org/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 REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7163 To: rkflx, #frameworks Cc: dhaumann
D7163: KSqueezedTextLabel: Add several autotests
rkflx marked 5 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7163 To: rkflx, #frameworks Cc: dhaumann
D7163: KSqueezedTextLabel: Add several autotests
rkflx updated this revision to Diff 17971.
rkflx added a comment.
Address comments:
- anonymous namespace
- QString() instead of QStringLiteral("")
- "pixels" for "amount"
- setProperty() instead of pointer to member function, remove switch and enum
REPOSITORY
R236 KWidgetsAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7163?vs=17775=17971
BRANCH
arcpatch-D7163
REVISION DETAIL
https://phabricator.kde.org/D7163
AFFECTED FILES
autotests/CMakeLists.txt
autotests/ksqueezedtextlabelautotest.cpp
autotests/ksqueezedtextlabelautotest.h
To: rkflx, #frameworks
Cc: dhaumann
D7163: KSqueezedTextLabel: Add several autotests
rkflx added a comment. > 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 ;) REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7163 To: rkflx, #frameworks Cc: dhaumann
D7162: KSqueezedTextLabel: Add isSqueezed() for convenience
rkflx marked an inline comment as done. REPOSITORY R236 KWidgetsAddons BRANCH arcpatch-D7162 REVISION DETAIL https://phabricator.kde.org/D7162 To: rkflx, #frameworks, dhaumann Cc: dhaumann
D7162: KSqueezedTextLabel: Add isSqueezed() for convenience
rkflx updated this revision to Diff 17970. rkflx added a comment. Improve wording to be as elegant as suggested by Dominik. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7162?vs=17774=17970 BRANCH arcpatch-D7162 REVISION DETAIL https://phabricator.kde.org/D7162 AFFECTED FILES src/ksqueezedtextlabel.cpp src/ksqueezedtextlabel.h To: rkflx, #frameworks, dhaumann Cc: dhaumann
D7161: KSqueezedTextLabel: Small improvements to API docs
rkflx updated this revision to Diff 17969. rkflx added a comment. > Mabye the "@note" about some methods being not virtual in the base class is even worth to put into the main class documentation. Good idea, let's move it to the class description (to be referenced from each affected function). REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7161?vs=17773=17969 BRANCH arcpatch-D7161 REVISION DETAIL https://phabricator.kde.org/D7161 AFFECTED FILES src/ksqueezedtextlabel.h To: rkflx, #frameworks, dhaumann Cc: dhaumann
