Now we can use lineedit password outside kpassworddialog.
It's useful in kdepim for example.
Details
- Reviewers
cfeck dfaure - Commits
- R236:ec3b3e105064: CHANGELOG: Extract lineedit password widget
I added autotest and test apps.
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
autotests/knewpasswordwidgettest.cpp | ||
---|---|---|
63 | Does using setPassword() not work for these tests? | |
src/knewpasswordwidget.cpp | ||
75 | See discussion about properies. | |
src/knewpasswordwidget.ui | ||
81 | Why this sizeHint? | |
src/passwordlineedit.h | ||
29 ↗ | (On Diff #17420) | Please add a Q_PROPERTY for password, including passwordChanged() signal (NOTIFY method). Applications should not have to deal with the passwordLineEdit() function in the regular use-case, but having the accessor function for special purpose is a good idea. |
46 ↗ | (On Diff #17420) | Either remove the 'p', or name it 'password'. |
src/passwordlineedit.h | ||
---|---|---|
27 ↗ | (On Diff #17420) | Surprised to see no K-prefix being used here? Why that? Please add one and rename to KPasswordLineEdit:
|
autotests/knewpasswordwidgettest.cpp | ||
---|---|---|
63 | Nope as setPassword not authorize to see icon (it's a security when we setPassword from apps you don't want to see it) | |
src/knewpasswordwidget.ui | ||
81 | designer bug :) | |
src/kpassworddialog.ui | ||
191 | manual ? :) nope it's designer which added it | |
src/passwordlineedit.h | ||
27 ↗ | (On Diff #17420) | Just for info LineEditUrlDropEventFilter has not K prefix |
autotests/knewpasswordwidgettest.cpp | ||
---|---|---|
63 | Could you add this as comment to this line? it is not directly obvious, so would be good to store this knowledge directly in the code, so the next code reader gets why the code is like this. | |
src/passwordlineedit.h | ||
24 ↗ | (On Diff #17420) | Can be removed, given the QLineEdit include (for QLineEdit::EchoMode) |
27 ↗ | (On Diff #17420) | Thanks :) Yes, seems LineEditUrlDropEventFilter slipped in without somebody catching that. When you do could you please also adapt PasswordLineEditPrivate? Even the test best would follow the naming pattern, so when scanning the dir with tests to find the matching test, it is found where expected (with default sorting by names). |
27 ↗ | (On Diff #17420) | Please also add API dox comment in front of the class, otherwise kapidox and ecm_add_qch will not pick up this class, given their doxygen settings. |
33 ↗ | (On Diff #17420) | @since should go to central class api dox comment (see other comment), or be behind the @param (see https://community.kde.org/Frameworks/Frameworks_Documentation_Policy#Document_Public_and_Protected_Members). |
61 ↗ | (On Diff #17420) | For a complete API this would also have isClearButtonEnabled() const here, and making this a clearButtonEnabled property. With these properties e.g. integration in Qt Designer can be improved. |
66 ↗ | (On Diff #17420) | getter missing as well for balanced api, and should become a property as well. |
94 ↗ | (On Diff #17420) | Please add api dox for this signal. Which echo mode is triggered here? and can't it be reverted? |
97 ↗ | (On Diff #17420) | can be removed, class already forward-declared at the beginning, as normal non-nested one (as helpful for avoing visibility inheritance). |
I renamed class/test apps/autotests as requested
I fixed autotest with new autotest from master
I fixed all comment on this review
etc.
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | Make this a normal class KPasswordLineEditPrivate here, following fixes proposed above for the header. | |
src/kpasswordlineedit.h | ||
26 | change to KPasswordLineEditPrivate, so it is the forward declaration of the non-nested private class which KPasswordLineEdit should use. | |
50 | better name this property clearButtonEnabled instead of clearButton, following the property name in qlinedit | |
129 | Thanks for the docs. Hm, the name "echoModeTriggered" by itself is telling me that some "echomode" is triggered. While actually the signal tells that the echomode has been toggled/changed. So I would propose to rename it. And ideally the signal would also carry the new echo mode, so the slot does not need to query again what the new mode is. Perhaps consider changing this to | |
134 | Remove this embedded class forward declaration -> results in leaked symbols due to visibility inherited. |
src/knewpasswordwidget.ui | ||
---|---|---|
88 | rather extends QWidget now? unsure what exactly the consequences are | |
src/kpassworddialog.ui | ||
163 | Again QWidget? | |
src/kpasswordlineedit.h | ||
7 | Should this not be rather lgpl2_+_? |
Use a correct license (I copied a header from david class so I think it's ok now) as in kwidgetaddons there is lgpl2 lgpl2.1 lgpl3 lgpl...
I didn't change ui file a QWidget otherwise we can't see qlineedit in designer.
But if you use QLineEdit as a base class, the developer can set properties for a QLineEdit, which might not apply to KPasswordLineEdit.
Better add the widget to the kdesigner plugin framework, then you'll have proper integration with Qt Designer.
I created a designer plugin for it, I will put url here after create review
I add property for echo mode
Now we can see lineedit in designer (created from a plugin)
autotests/CMakeLists.txt | ||
---|---|---|
21 | If that file is sorted (at least a bit), try to insert instead of append. Same for other places. | |
src/knewpasswordwidget.cpp | ||
118 | Can you double-check this change? To me, the echoMode() checks are now reversed compared to the original code. I have not tested the code yet :) | |
src/kpasswordlineedit.cpp | ||
31 | Does this class need to be a QObject? If not, remove the QObject stuff. | |
src/kpasswordlineedit.h | ||
134 | Also needs the const QString &password argument. Or if I misunderstand it, what is the difference between passwordChanged() and textChanged()? This class has no text() property, so having a textChanged() looks unexpected. |
src/knewpasswordwidget.cpp | ||
---|---|---|
118 | Nope it's ok as code is called after changing echomode |
src/knewpasswordwidget.cpp | ||
---|---|---|
118 | Then please rename the slot to _k_echoModeToggled, otherwise is confusing. |
src/knewpasswordwidget.cpp | ||
---|---|---|
118 | Ah, now I understand. Also, use the passed echoMode, instead of accessing lineEdit(). |
src/knewpasswordwidget.cpp | ||
---|---|---|
75 | Ah wait, you are using the same slot for the verify widget, so maybe scratch that comment. |
src/knewpasswordwidget.cpp | ||
---|---|---|
118 | we can't as we use it in : |
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | You made it a namespaced class again. Why? |
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | Otherwise it failed to using Q_PRIVATE_SLOT... |
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | That's fishy. I just checked a single class (KColumnResizer), and it has just "class KColumnResizerPrivate" (no namespace, no QObject), and still has a Q_PRIVATE_SLOT in its KColumnResizer class. |
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | wierd it didn't want to build, but after a make clean it works... |
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | +1. Don't use nested private classes. Nested classes/structs inherit the symbols visibility. Also see: https://phabricator.kde.org/D6927 PS: In times of Qt5 & lambda-connects, you don't need Q_PRIVATE_SLOT anymore, anyway: |
src/kpasswordlineedit.cpp | ||
---|---|---|
3 ↗ | (On Diff #17620) | Elvis, you wrote the "visibilityAction" stuff, right? If yes, needs to be mentioned here. |
26 ↗ | (On Diff #17620) | QLineEdit is included in the .h file, can be removed here. |
36 ↗ | (On Diff #17620) | Remove empty line. |
60 ↗ | (On Diff #17620) | Remove doubled empty line. |
116 ↗ | (On Diff #17620) | Does this need a protection for not emitting if the value did not actually change? I think it helps to prevent recursion. |
src/kpasswordlineedit.cpp | ||
---|---|---|
31 | Oh, that would make a nice "junior job" to get those cleaned out everywhere. Is removing a private slot considered API/ABI compatible? |
Looks good to go from my side. Let's wait a bit if someone has more objections.
Thanks Laurent, it was a needed change considering the code duplication that was present before.