D15328: kfilewidget: convert connect syntax
jtamate planned changes to this revision. jtamate added a comment. QTest::qWaitForWindowActive fails because I use kwin Focus stealing prevention High, therefore the windows doesn't become active until I click on them in the task bar or switch to them. And qWaitForWindowExposed doesn't help because the window/widget needs to have the focus. Is creating a README file and a kwin script to disable stealing prevention for kio tests an acceptable solution? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15328 To: jtamate, dfaure, #frameworks Cc: bruns, anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham
D15328: kfilewidget: convert connect syntax
jtamate updated this revision to Diff 43106. jtamate added a comment. Fix a crash ,caused by a still connected signal, after running again the unittests. kfilewidgettest still doesn't pass because QTest::qWaitForWindowActive fails for me. Don't accept this revision until kfilewidgettest works flawlessly. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15328?vs=41246&id=43106 REVISION DETAIL https://phabricator.kde.org/D15328 AFFECTED FILES src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: jtamate, dfaure, #frameworks Cc: bruns, anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham
D15328: kfilewidget: convert connect syntax
jtamate updated this revision to Diff 41246. jtamate added a comment. Use nullptr instead of 0. Remove the copied/pasted part of the cause of two dissconnects. @bruns, as I'm not sure if the code emits other signals for lineEdit, I prefer to keep it as it was. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15328?vs=41150&id=41246 REVISION DETAIL https://phabricator.kde.org/D15328 AFFECTED FILES src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: jtamate, dfaure, #frameworks Cc: bruns, anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham
D15328: kfilewidget: convert connect syntax
bruns added inline comments.
INLINE COMMENTS
> anthonyfieroni wrote in kfilewidget.cpp:1225
> QObject::disconnect(m_connEditTextChanged);
> m_connEditTextChanged = QMetaObject::Connection();
Me neither ...
But me wonders if this should not use `{QSignalBlocker(locationEdit); ...}`
instead.
Also, the comment above is slightly wrong, there is no `setCurrentItem()` below.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D15328
To: jtamate, dfaure, #frameworks
Cc: bruns, anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham
D15328: kfilewidget: convert connect syntax
jtamate updated this revision to Diff 41150. jtamate marked an inline comment as done. jtamate added a comment. Changed to 'Anonymous' connects and disconnects. Even if the documentation say: use 0 as a wildcard, the compiler thinks it should be a nullptr kfilewidget.cpp:1222:74: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant] REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15328?vs=41148&id=41150 REVISION DETAIL https://phabricator.kde.org/D15328 AFFECTED FILES src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: jtamate, dfaure, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
jtamate marked an inline comment as done. jtamate added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfilewidget.cpp:1225 > QObject::disconnect(m_connEditTextChanged); > m_connEditTextChanged = QMetaObject::Connection(); I'm sorry, but I still don't get it. Doesn't connect/disconnect work more or less as new/delete? I mean, if m_connEditTextChanged is reconnected, can't it be disconnected safely? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15328 To: jtamate, dfaure, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
anthonyfieroni added inline comments. INLINE COMMENTS > jtamate wrote in kfilewidget.cpp:1225 > I'm sorry but I don't understand your comment. > m_connEditTextChanged is created in the constructor, line 585. > Afterwards it is disconnected and reconnected in: > setDummyHistoryEntry, removeDummyHistoryEntry and readRecentFiles. QObject::disconnect(m_connEditTextChanged); m_connEditTextChanged = QMetaObject::Connection(); REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15328 To: jtamate, dfaure, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
jtamate added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfilewidget.cpp:1225 > Whenever disconnect after that reset connection to QMetaObject::Connection > cause disconnect empty connection is safe, double disconnect same one - does > not. I'm sorry but I don't understand your comment. m_connEditTextChanged is created in the constructor, line 585. Afterwards it is disconnected and reconnected in: setDummyHistoryEntry, removeDummyHistoryEntry and readRecentFiles. > broulik wrote in kfilewidget.cpp:440 > Shouldn't this be using the `url` from the signal? > Also, please prefer explicit captures over `&` or `=` I think it is using the url, the parameter to the lambda and _k_urlEntered(url); Changed & for this, d can't be used directly. > broulik wrote in kfilewidget.cpp:473 > Why not change this as well? Because I didn't knew KActionCollection have it already! Done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15328 To: jtamate, dfaure, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
anthonyfieroni added inline comments. INLINE COMMENTS > kfilewidget.cpp:1225 > // the KDirOperator's view-selection in there > -QObject::disconnect(locationEdit, SIGNAL(editTextChanged(QString)), > -q, SLOT(_k_slotLocationChanged(QString))); > +QObject::disconnect(m_connEditTextChanged); > Whenever disconnect after that reset connection to QMetaObject::Connection cause disconnect empty connection is safe, double disconnect same one - does not. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15328 To: jtamate, dfaure, #frameworks Cc: anthonyfieroni, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
jtamate updated this revision to Diff 41148. jtamate marked 2 inline comments as done. jtamate added a comment. Changed [&] by [this]in the lambdas. Added a lambda for KActionCollection::addAction. I didn't knew it was already possible. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15328?vs=41145&id=41148 REVISION DETAIL https://phabricator.kde.org/D15328 AFFECTED FILES src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: jtamate, dfaure, #frameworks Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
broulik added inline comments.
INLINE COMMENTS
> kfilewidget.cpp:440
> +connect(d->ops, &KDirOperator::urlEntered, this,
> +[&](const QUrl &url){d->_k_urlEntered(url);});
> +connect(d->ops, &KDirOperator::fileHighlighted, this,
Shouldn't this be using the `url` from the signal?
Also, please prefer explicit captures over `&` or `=`
> kfilewidget.cpp:473
>
> QAction *goToNavigatorAction =
> coll->addAction(QStringLiteral("gotonavigator"), this,
> SLOT(_k_activateUrlNavigator()));
> goToNavigatorAction->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_L));
Why not change this as well?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D15328
To: jtamate, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D15328: kfilewidget: convert connect syntax
jtamate created this revision. jtamate added reviewers: dfaure, Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. jtamate requested review of this revision. REVISION SUMMARY Convert from the old syntax to the new connect syntax TEST PLAN The open file dialog works. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15328 AFFECTED FILES src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
