This revision was automatically updated to reflect the committed changes.
Closed by commit R236:ec3b3e105064: CHANGELOG: Extract lineedit password widget
(authored by mlaurent).
REPOSITORY
R236 KWidgetsAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17621&id=17657
REVI
cfeck removed a reviewer: elvisangelaccio.
This revision is now accepted and ready to land.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure
Cc: kfunk, kossebau, elvisangelaccio, #frameworks
cfeck accepted this revision.
cfeck added a comment.
Together with the Designer stuff, please.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks
dfaure accepted this revision.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks
elvisangelaccio added a comment.
+1, looks good to me now.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks
cfeck added a comment.
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.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvis
mlaurent updated this revision to Diff 17621.
mlaurent added a comment.
+1
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17620&id=17621
REVISION DETAIL
https://phabricator.kde.org/D7011
AFFECTED FILES
autotests/CMakeLists.txt
autotests/knewpasswordwidgettest.cpp
au
cfeck added inline comments.
INLINE COMMENTS
> kfunk wrote in kpasswordlineedit.cpp:30
> +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_PR
cfeck added inline comments.
INLINE COMMENTS
> kpasswordlineedit.cpp:3
> + Copyright (c) 2017 Montel Laurent
> +
> + This library is free software; you can redistribute it and/or modify
Elvis, you wrote the "visibilityAction" stuff, right? If yes, needs to be
mentioned here.
> kpasswordli
mlaurent updated this revision to Diff 17620.
mlaurent added a comment.
Remove Q_PRIVATE_SLOT
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17617&id=17620
REVISION DETAIL
https://phabricator.kde.org/D7011
AFFECTED FILES
autotests/CMakeLists.txt
autotests/knewpassword
mlaurent updated this revision to Diff 17617.
mlaurent added a comment.
update
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17615&id=17617
REVISION DETAIL
https://phabricator.kde.org/D7011
AFFECTED FILES
autotests/CMakeLists.txt
autotests/knewpasswordwidgettest.cpp
mlaurent added inline comments.
INLINE COMMENTS
> cfeck wrote in kpasswordlineedit.cpp:30
> 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.
wi
kfunk added inline comments.
INLINE COMMENTS
> cfeck wrote in kpasswordlineedit.cpp:30
> 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.
+1. D
cfeck added inline comments.
INLINE COMMENTS
> mlaurent wrote in kpasswordlineedit.cpp:30
> Otherwise it failed to using Q_PRIVATE_SLOT...
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_P
mlaurent added inline comments.
INLINE COMMENTS
> cfeck wrote in kpasswordlineedit.cpp:30
> You made it a namespaced class again. Why?
Otherwise it failed to using Q_PRIVATE_SLOT...
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, e
cfeck added inline comments.
INLINE COMMENTS
> cfeck wrote in kpasswordlineedit.cpp:30
> Ping?
You made it a namespaced class again. Why?
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks
mlaurent updated this revision to Diff 17615.
mlaurent added a comment.
Fix all comment reported
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17606&id=17615
REVISION DETAIL
https://phabricator.kde.org/D7011
AFFECTED FILES
autotests/CMakeLists.txt
autotests/knewpassw
mlaurent marked 4 inline comments as done.
mlaurent added inline comments.
INLINE COMMENTS
> cfeck wrote in knewpasswordwidget.cpp:118
> Ah, now I understand. Also, use the passed echoMode, instead of accessing
> lineEdit().
we can't as we use it in :
connect(toggleEchoModeAction, &QAction::tri
cfeck added inline comments.
INLINE COMMENTS
> cfeck wrote in knewpasswordwidget.cpp:75
> While you are at renaming slots: -> _k_passwordChanged
Ah wait, you are using the same slot for the verify widget, so maybe scratch
that comment.
REVISION DETAIL
https://phabricator.kde.org/D7011
To: m
cfeck added inline comments.
INLINE COMMENTS
> 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
cfeck added inline comments.
INLINE COMMENTS
> elvisangelaccio wrote in knewpasswordwidget.cpp:118
> Then please rename the slot to `_k_echoModeToggled`, otherwise is confusing.
Ah, now I understand. Also, use the passed echoMode, instead of accessing
lineEdit().
REVISION DETAIL
https://phab
cfeck added inline comments.
INLINE COMMENTS
> cfeck wrote in kpasswordlineedit.cpp:30
> Does this class need to be a QObject? If not, remove the QObject stuff.
Ping?
> kpasswordlineedit.h:53
> +Q_PROPERTY(bool clearButtonEnabled READ isClearButtonEnabled WRITE
> setClearButtonEnabled)
> +
elvisangelaccio added inline comments.
INLINE COMMENTS
> mlaurent wrote in knewpasswordwidget.cpp:118
> Nope it's ok as code is called after changing echomode
> I created an autotest for testing it
Then please rename the slot to `_k_echoModeToggled`, otherwise is confusing.
REVISION DETAIL
ht
mlaurent marked 3 inline comments as done.
mlaurent added inline comments.
INLINE COMMENTS
> cfeck wrote in 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 :)
Nope i
mlaurent updated this revision to Diff 17606.
mlaurent marked an inline comment as done.
mlaurent added a comment.
Fix include order, remove textChanged, add argument to signal etc.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17541&id=17606
REVISION DETAIL
https://phabr
cfeck added inline comments.
INLINE COMMENTS
> CMakeLists.txt:21
>ksplittercollapserbuttontest.cpp
> + kpasswordlineedittest.cpp
>LINK_LIBRARIES Qt5::Test KF5::WidgetsAddons
If that file is sorted (at least a bit), try to insert instead of append. Same
for other places.
> knewpassword
mlaurent added a comment.
https://phabricator.kde.org/D7062 (designer plugin)
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks
mlaurent updated this revision to Diff 17541.
mlaurent added a comment.
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)
CHANGES SINCE LAST UPDATE
https://phabricator.kde.o
dfaure added a comment.
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.
REVISION DETAIL
mlaurent updated this revision to Diff 17535.
mlaurent added a comment.
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
designe
mlaurent added inline comments.
INLINE COMMENTS
> kossebau wrote in knewpasswordwidget.ui:88
> rather extends QWidget now? unsure what exactly the consequences are
The consequence is that we don't see it in designer :)
So why not but it's better to show a lineedit no ?
> kossebau wrote in kpass
kossebau added inline comments.
INLINE COMMENTS
> knewpasswordwidget.ui:88
> + KPasswordLineEdit
> + QLineEdit
> + kpasswordlineedit.h
rather extends QWidget now? unsure what exactly the consequences are
> kpassworddialog.ui:163
> + KPasswordLineEdit
> + QLineEdit
> + kpasswordlinee
mlaurent updated this revision to Diff 17533.
mlaurent added a comment.
Fix copyright
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17471&id=17533
REVISION DETAIL
https://phabricator.kde.org/D7011
AFFECTED FILES
autotests/CMakeLists.txt
autotests/knewpasswordwidgette
mlaurent updated this revision to Diff 17471.
mlaurent added a comment.
Rename method as lineEdit as requested by David
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7011?vs=17470&id=17471
REVISION DETAIL
https://phabricator.kde.org/D7011
AFFECTED FILES
autotests/CMakeLists.tx
mlaurent retitled this revision from "Extract lineedit password" to "Extract
KPasswordLineEdit class".
REVISION DETAIL
https://phabricator.kde.org/D7011
To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks
35 matches
Mail list logo
