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
D6876: Fix install apk target
apol accepted this revision. apol added a comment. This revision is now accepted and ready to land. That path changed since the `--gradle` change, commit together. REPOSITORY R240 Extra CMake Modules BRANCH fix_install_apk REVISION DETAIL https://phabricator.kde.org/D6876 To: aacid, apol, mart, kossebau Cc: #frameworks, #build_system
D6875: Add --gradle to androiddeployqt
apol accepted this revision. apol added a comment. This revision is now accepted and ready to land. Fixes the issue for me too. REPOSITORY R240 Extra CMake Modules BRANCH add_gradle REVISION DETAIL https://phabricator.kde.org/D6875 To: aacid, apol, kossebau, mart Cc: #frameworks, #build_system
D6972: Add CC BY-SA 4.0 International and set it as default
ltoscano requested review of this revision. ltoscano added a comment. Now the patch should be more complete and allows its usage in translated documentation. REPOSITORY R238 KDocTools REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell, lueck Cc: jriddell, lueck, #frameworks, #documentation, skadinna
D6972: Add CC BY-SA 4.0 International and set it as default
ltoscano updated this revision to Diff 17466. ltoscano added a comment. Make the entity usable for translated documents REPOSITORY R238 KDocTools CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6972?vs=17451=17466 BRANCH add-ccbysa4-translatable REVISION DETAIL https://phabricator.kde.org/D6972 AFFECTED FILES common/en/ccbysa4-license.html src/customization/af/lang.entities src/customization/bg/lang.entities src/customization/ca/lang.entities src/customization/cs/lang.entities src/customization/da/lang.entities src/customization/de/lang.entities src/customization/el/lang.entities src/customization/en-GB/lang.entities src/customization/en/catalog.xml src/customization/en/entities/ccbysa4-notice.docbook src/customization/en/entities/underCCBYSA4.docbook src/customization/en/lang.entities src/customization/entities/general.entities src/customization/eo/lang.entities src/customization/es/lang.entities src/customization/et/lang.entities src/customization/fi/lang.entities src/customization/fo/lang.entities src/customization/fr/lang.entities src/customization/gl/lang.entities src/customization/he/lang.entities src/customization/hu/lang.entities src/customization/id/lang.entities src/customization/it/lang.entities src/customization/ja/lang.entities src/customization/ko/lang.entities src/customization/lt/lang.entities src/customization/nds/lang.entities src/customization/nl/lang.entities src/customization/nn/lang.entities src/customization/no/lang.entities src/customization/pl/lang.entities src/customization/pt-BR/lang.entities src/customization/pt/lang.entities src/customization/ro/lang.entities src/customization/ru/lang.entities src/customization/sk/lang.entities src/customization/sl/lang.entities src/customization/sq/lang.entities src/customization/sr/lang.entities src/customization/sr@ijekavian/lang.entities src/customization/sr@ijekavianlatin/lang.entities src/customization/sr@latin/lang.entities src/customization/sv/lang.entities src/customization/th/lang.entities src/customization/tr/lang.entities src/customization/uk/lang.entities src/customization/wa/lang.entities src/customization/xh/lang.entities src/customization/xx/lang.entities src/customization/zh-CN/lang.entities src/customization/zh-TW/lang.entities src/template.docbook To: ltoscano, jriddell, lueck Cc: jriddell, lueck, #frameworks, #documentation, skadinna
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
D6972: Add CC BY-SA 4.0 International and set it as default
ltoscano added inline comments. INLINE COMMENTS > general.entities:57 >"../en/entities/lgpl-notice.docbook"> > + + "-//KDE//DOCUMENT CC BY-SA 4.0 License Notice//EN" Rereading the code, I was wondering if I should make this translatable (aka defined into en/entities like FDLNotice). REPOSITORY R238 KDocTools BRANCH add-ccbysa4 REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell, lueck Cc: jriddell, lueck, #frameworks, #documentation, skadinna
D6972: Add CC BY-SA 4.0 International and set it as default
lueck accepted this revision. lueck added a comment. This revision is now accepted and ready to land. thanks REPOSITORY R238 KDocTools BRANCH add-ccbysa4 REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell, lueck Cc: jriddell, lueck, #frameworks, #documentation, skadinna
D6820: Add QValidator to KTimeCombobox
bednar updated this revision to Diff 17452.
bednar added a comment.
Ran through uncrustify.
Found a regression in my patch : colon (":") wouldn't be shown anymore, as it
is dependent on the input mask. So I left the input mask, and the QValidator
works in addition to the input mask.
REPOSITORY
R236 KWidgetsAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6820?vs=16984=17452
REVISION DETAIL
https://phabricator.kde.org/D6820
AFFECTED FILES
src/ktimecombobox.cpp
To: bednar
Cc: cfeck, #frameworks
D6972: Add CC BY-SA 4.0 International and set it as default
ltoscano requested review of this revision. REPOSITORY R238 KDocTools REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell Cc: jriddell, lueck, #frameworks, #documentation, skadinna
D6972: Add CC BY-SA 4.0 International and set it as default
ltoscano retitled this revision from "Add CC BY-SA 4.0 International" to "Add CC BY-SA 4.0 International and set it as default". REPOSITORY R238 KDocTools BRANCH add-ccbysa4 REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell Cc: jriddell, lueck, #frameworks, #documentation, skadinna
D6972: Add CC BY-SA 4.0 International
ltoscano updated this revision to Diff 17451. ltoscano added a comment. This revision is now accepted and ready to land. Address the comments REPOSITORY R238 KDocTools CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6972?vs=17339=17451 BRANCH add-ccbysa4 REVISION DETAIL https://phabricator.kde.org/D6972 AFFECTED FILES common/en/ccbysa4-license.html src/customization/en/catalog.xml src/customization/en/entities/ccbysa4-notice.docbook src/customization/en/entities/underCCBYSA4.docbook src/customization/en/lang.entities src/customization/entities/general.entities src/template.docbook To: ltoscano, jriddell Cc: jriddell, lueck, #frameworks, #documentation, skadinna
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
D6972: Add CC BY-SA 4.0 International
ltoscano planned changes to this revision. ltoscano added a comment. I need to apply the changes requested by Burkhard first. REPOSITORY R238 KDocTools REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell Cc: jriddell, lueck, #frameworks, #documentation, skadinna
D6972: Add CC BY-SA 4.0 International
jriddell accepted this revision. jriddell added a comment. This revision is now accepted and ready to land. thanks REPOSITORY R238 KDocTools BRANCH add-ccbysa4 REVISION DETAIL https://phabricator.kde.org/D6972 To: ltoscano, jriddell Cc: jriddell, lueck, #frameworks, #documentation, skadinna
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
KDE CI: Frameworks extra-cmake-modules kf5-qt5 XenialQt5.7 - Build # 32 - Unstable!
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20extra-cmake-modules%20kf5-qt5%20XenialQt5.7/32/ Project: Frameworks extra-cmake-modules kf5-qt5 XenialQt5.7 Date of build: Mon, 31 Jul 2017 11:57:37 + Build duration: 6 min 53 sec and counting JUnit Tests Name: (root) Failed: 1 test(s), Passed: 48 test(s), Skipped: 0 test(s), Total: 49 test(s)Failed: TestSuite.ECMPoQmToolsTest Cobertura Report Project Coverage Summary Name Cobertura Coverage Report build.log Description: Binary data
KDE CI: Frameworks extra-cmake-modules kf5-qt5 FreeBSDQt5.7 - Build # 41 - Still Unstable!
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20extra-cmake-modules%20kf5-qt5%20FreeBSDQt5.7/41/ Project: Frameworks extra-cmake-modules kf5-qt5 FreeBSDQt5.7 Date of build: Mon, 31 Jul 2017 11:57:37 + Build duration: 4 min 27 sec and counting JUnit Tests Name: (root) Failed: 1 test(s), Passed: 48 test(s), Skipped: 0 test(s), Total: 49 test(s)Failed: TestSuite.KDEInstallDirsTest.relative_or_absolute build.log Description: Binary data
D6701: Make ECMPoQmToolsTestactually fail if a translation is wrong
This revision was automatically updated to reflect the committed changes. Closed by commit R240:899519f0be03: Make ECMPoQmToolsTest actually fail if a translation is wrong (authored by wbauer). REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6701?vs=16705=17422 REVISION DETAIL https://phabricator.kde.org/D6701 AFFECTED FILES tests/ECMPoQmToolsTest/check.cmake.in To: wbauer, skelly, apol Cc: apol, #frameworks, #build_system
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
D6963: Don't block starting notification service
vandenoever added a comment. Removing this call is in line with the advice in the dbus spec. An improvement to the patch would be to remove `bool startfdo`. Q_WS_WIN seems to suggest that windows always needs to start the service manually. That case would break with this patch. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D6963 To: davidedmundson Cc: vandenoever, mck182, dvratil, #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
D6990: Allow to build Sonnet without Qt5Widgets
vkrause added a comment. Fair point, I'll add options for this. REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D6990 To: vkrause, #frameworks, cordlandwehr Cc: aacid
D6963: Don't block starting notification service
davidedmundson added a comment. No, his one still blocks. This doesn't. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D6963 To: davidedmundson Cc: mck182, dvratil, #frameworks
