D7011: Extract lineedit password
mlaurent updated this revision to Diff 17470. mlaurent added a comment. Create a real class for private class as requested + fix signal etc. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7011?vs=17469=17470 REVISION DETAIL https://phabricator.kde.org/D7011 AFFECTED FILES autotests/CMakeLists.txt autotests/knewpasswordwidgettest.cpp autotests/kpassworddialogautotest.cpp autotests/kpasswordlineedittest.cpp autotests/kpasswordlineedittest.h src/CMakeLists.txt src/knewpasswordwidget.cpp src/knewpasswordwidget.h src/knewpasswordwidget.ui src/kpassworddialog.cpp src/kpassworddialog.h src/kpassworddialog.ui src/kpasswordlineedit.cpp src/kpasswordlineedit.h tests/CMakeLists.txt tests/kpasswordlineedit_test.cpp To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
kossebau added inline comments.
INLINE COMMENTS
> kpasswordlineedit.cpp:30
> +
> +class KPasswordLineEdit::KPasswordLineEditPrivate : public QObject
> +{
Make this a normal class KPasswordLineEditPrivate here, following fixes
proposed above for the header.
> kpasswordlineedit.h:25
> +class QAction;
> +class PasswordLineEditPrivate;
> +
change to KPasswordLineEditPrivate, so it is the forward declaration of the
non-nested private class which KPasswordLineEdit should use.
> kpasswordlineedit.h:49
> +Q_PROPERTY(QString password READ password WRITE setPassword NOTIFY
> passwordChanged)
> +Q_PROPERTY(bool clearButton READ isClearButtonEnabled WRITE
> setClearButtonEnabled)
> +public:
better name this property `clearButtonEnabled` instead of `clearButton`,
following the property name in qlinedit
> kpasswordlineedit.h:128
> + */
> +void echoModeTriggered();
> +void passwordChanged();
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.
Just an suggestion while you expose this as public API, I see the old internal
code was fine to just query again. But not really matching the typical Qt-style
API that we all so like :)
Perhaps consider changing this to
`void echoModeChanged(QLineEdit::EchoMode echoMode);`
> kpasswordlineedit.h:133
> +private:
> +class KPasswordLineEditPrivate;
> +KPasswordLineEditPrivate *const d;
Remove this embedded class forward declaration -> results in leaked symbols due
to visibility inherited.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
mlaurent updated this revision to Diff 17469. mlaurent added a comment. I renamed class/test apps/autotests as requested I fixed autotest with new autotest from master I fixed all comment on this review etc. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7011?vs=17420=17469 REVISION DETAIL https://phabricator.kde.org/D7011 AFFECTED FILES autotests/CMakeLists.txt autotests/knewpasswordwidgettest.cpp autotests/kpassworddialogautotest.cpp autotests/kpasswordlineedittest.cpp autotests/kpasswordlineedittest.h src/CMakeLists.txt src/knewpasswordwidget.cpp src/knewpasswordwidget.h src/knewpasswordwidget.ui src/kpassworddialog.cpp src/kpassworddialog.h src/kpassworddialog.ui src/kpasswordlineedit.cpp src/kpasswordlineedit.h tests/CMakeLists.txt tests/kpasswordlineedit_test.cpp To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
mlaurent marked 7 inline comments as done. REVISION DETAIL https://phabricator.kde.org/D7011 To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
kossebau added inline comments.
INLINE COMMENTS
> mlaurent wrote in 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)
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.
> passwordlineedit.h:24
> +#include
> +class QLineEdit;
> +class QAction;
Can be removed, given the QLineEdit include (for QLineEdit::EchoMode)
> mlaurent wrote in passwordlineedit.h:27
> Just for info LineEditUrlDropEventFilter has not K prefix
> but ok I will rename it
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).
> passwordlineedit.h:27
> +class PasswordLineEditPrivate;
> +class KWIDGETSADDONS_EXPORT PasswordLineEdit : public QWidget
> +{
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.
> passwordlineedit.h:33
> + * Constructs a lineedit password widget.
> + * @since 5.37
> + *
@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).
> passwordlineedit.h:61
> + */
> +void setClearButtonEnabled(bool clear);
> +
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.
> passwordlineedit.h:66
> + */
> +void setEchoMode(QLineEdit::EchoMode mode);
> +/**
getter missing as well for balanced api, and should become a property as well.
> passwordlineedit.h:94
> +Q_SIGNALS:
> +void echoModeTriggered();
> +
Please add api dox for this signal. Which echo mode is triggered here? and
can't it be reverted?
> passwordlineedit.h:97
> +private:
> +class PasswordLineEditPrivate;
> +PasswordLineEditPrivate *const d;
can be removed, class already forward-declared at the beginning, as normal
non-nested one (as helpful for avoing visibility inheritance).
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
mlaurent marked 5 inline comments as done. REVISION DETAIL https://phabricator.kde.org/D7011 To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
mlaurent added inline comments. INLINE COMMENTS > cfeck wrote in knewpasswordwidgettest.cpp:63 > Does using setPassword() not work for these tests? Nope as setPassword not authorize to see icon (it's a security when we setPassword from apps you don't want to see it) > cfeck wrote in knewpasswordwidget.ui:81 > Why this sizeHint? designer bug :) > cfeck wrote in kpassworddialog.ui:191 > Revert manual white-space edit. manual ? :) nope it's designer which added it > kossebau wrote in passwordlineedit.h:27 > Surprised to see no K-prefix being used here? Why that? > > Please add one and rename to `KPasswordLineEdit`: > > - for consistency with the existing classes from kwidgetaddons/kde frameworks > - to protect against potential clashes, even if rare chance, with existing > code linking to kwidgetaddons, which assumes using unprefixed/namespace-less > names is safe in own code and might use that name for something else already. > - namespace-less class name lack any hint to origin, but rather used for > project-internal code, so confuses code readers Just for info LineEditUrlDropEventFilter has not K prefix but ok I will rename it REVISION DETAIL https://phabricator.kde.org/D7011 To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: kossebau, elvisangelaccio, #frameworks
D7011: Extract lineedit password
cfeck requested changes to this revision. REVISION DETAIL https://phabricator.kde.org/D7011 To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: elvisangelaccio, #frameworks
D7011: Extract lineedit password
cfeck added a comment.
Any reason it should be PasswordLineEdit, and not KPasswordLineEdit? I do not
care about names, but I care about consistency.
INLINE COMMENTS
> knewpasswordwidgettest.cpp:63
> const QString password = QStringLiteral("1234");
> -linePassword->setText(password);
> +linePassword->passwordLineEdit()->setText(password);
> lineVerifyPassword->setText(password);
Does using setPassword() not work for these tests?
> knewpasswordwidget.cpp:75
>
> -connect(ui.linePassword, SIGNAL(textChanged(QString)), q,
> SLOT(_k_showToggleEchoModeAction(QString)));
> -connect(ui.linePassword, SIGNAL(textChanged(QString)), q,
> SLOT(_k_textChanged()));
> +connect(ui.linePassword->passwordLineEdit(),
> SIGNAL(textChanged(QString)), q, SLOT(_k_textChanged()));
> connect(ui.lineVerifyPassword, SIGNAL(textChanged(QString)), q,
> SLOT(_k_textChanged()));
See discussion about properies.
> knewpasswordwidget.ui:81
>
> +
> +
Why this sizeHint?
> kpassworddialog.ui:191
>
> -
> +
>
Revert manual white-space edit.
> passwordlineedit.h:29
> +{
> +Q_OBJECT
> +public:
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.
> passwordlineedit.h:46
> + */
> +void setPassword(const QString );
> +
Either remove the 'p', or name it 'password'.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: elvisangelaccio, #frameworks
D7011: Extract lineedit password
elvisangelaccio requested changes to this revision. elvisangelaccio added a comment. This revision now requires changes to proceed. I spotted one small regression, I added a test case for it in https://phabricator.kde.org/R236:380b35439a39950769fcca63f7ec31cc6170707a. For manual test you can also use `tests/knewpasswordwidget_test`: - type a password - click the visibility action - make sure the second line edit gets hidden. REVISION DETAIL https://phabricator.kde.org/D7011 To: mlaurent, cfeck, dfaure, elvisangelaccio Cc: elvisangelaccio, #frameworks
D7011: Extract lineedit password
mlaurent updated this revision to Diff 17420. mlaurent added a comment. I renamed class name. I ported KNewPasswordWidget . CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7011?vs=17413=17420 REVISION DETAIL https://phabricator.kde.org/D7011 AFFECTED FILES autotests/CMakeLists.txt autotests/knewpasswordwidgettest.cpp autotests/kpassworddialogautotest.cpp autotests/passwordlineedittest.cpp autotests/passwordlineedittest.h src/CMakeLists.txt src/knewpasswordwidget.cpp src/knewpasswordwidget.h src/knewpasswordwidget.ui src/kpassworddialog.cpp src/kpassworddialog.h src/kpassworddialog.ui src/passwordlineedit.cpp src/passwordlineedit.h tests/CMakeLists.txt tests/testpasswordlineedit.cpp To: mlaurent, cfeck, dfaure Cc: elvisangelaccio, #frameworks
D7011: Extract lineedit password
elvisangelaccio added a comment. Can you try to use this new widget also in KNewPasswordWidget? I'm also not sure about the name. I think PasswordLineEdit would be more clear? (since this is a line edit, not a password). REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7011 To: mlaurent, cfeck, dfaure Cc: elvisangelaccio, #frameworks
D7011: Extract lineedit password
mlaurent created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Now we can use lineedit password outside kpassworddialog. It's useful in kdepim for example. TEST PLAN I added autotest and test apps. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7011 AFFECTED FILES autotests/CMakeLists.txt autotests/kpassworddialogautotest.cpp autotests/lineeditpasswordtest.cpp autotests/lineeditpasswordtest.h src/CMakeLists.txt src/kpassworddialog.cpp src/kpassworddialog.h src/kpassworddialog.ui src/lineeditpassword.cpp src/lineeditpassword.h tests/CMakeLists.txt tests/testpasswordlineedit.cpp To: mlaurent, cfeck, dfaure Cc: #frameworks
