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
