D10822: Store temporary authorization status in IdleSlave
chinmoyr created this revision. chinmoyr added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. chinmoyr requested review of this revision. REVISION SUMMARY It will be used by klauncher to decide whether or not to kill the IdleSlave. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D10822 AFFECTED FILES src/core/idleslave.cpp src/core/idleslave.h To: chinmoyr, dfaure Cc: #frameworks, michaelh, kmorwinski
D10822: Store temporary authorization status in IdleSlave
chinmoyr added a dependent revision: D10824: Delete IdleSlave having temporary authorization. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10822 To: chinmoyr, dfaure Cc: #frameworks, michaelh, kmorwinski
D10822: Store temporary authorization status in IdleSlave
chinmoyr edited the summary of this revision. chinmoyr added a dependency: D10820: Send slave's polkit authorization status to the host. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10822 To: chinmoyr, dfaure Cc: #frameworks, michaelh, kmorwinski
D10822: Store temporary authorization status in IdleSlave
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> idleslave.cpp:82
> // Overload with (bool) onHold, (QUrl) url.
> if (!stream.atEnd()) {
> QUrl url;
Reading tempAuth before url breaks protocol, i.e. people who upgrade KF5 in a
running plasma session will be in big trouble.
At least check if (!stream.atEnd()) before stream >> tempAuth.
Hmm unfortunately that "last optional argument" URL breaks any possibility to
make the change fully compatible :(
How about this? Define MSG_SLAVE_STATUS_V2 which always sends all its arguments
(including tempAuth, onHold, and url), and modify SlaveBase to send that, and
support both here, at least temporarily?
New KIO on old klauncher will lead to "Unexpected data from KIO slave" (unknown
command, just above) but that'll be a clear message to logout or restart
kdeinit5, while deserializing a bool into a QUrl or vice-versa is just messy
and could lead to very strange and hard to debug behaviour.
And this way in the future we don't need a V3, we can just append new
arguments, with a atEnd() check.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10822
To: chinmoyr, dfaure
Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
chinmoyr updated this revision to Diff 28543. chinmoyr added a comment. Used MSG_SLAVE_STATUS_V2 REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10822?vs=27998&id=28543 BRANCH D10822 REVISION DETAIL https://phabricator.kde.org/D10822 AFFECTED FILES src/core/idleslave.cpp src/core/idleslave.h To: chinmoyr, dfaure Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> idleslave.cpp:70
> deleteLater();
> -} else if (cmd != MSG_SLAVE_STATUS) {
> +} else if (cmd != MSG_SLAVE_STATUS_V2) {
> qCritical() << "Unexpected data from KIO slave.";
This could be made more runtime compatible by not rejecting MSG_SLAVE_STATUS
completely, and handling it below (e.g. with a test on cmd at the right place,
to still share the local variables).
(That's what I meant by "support both here").
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10822
To: chinmoyr, dfaure
Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
chinmoyr updated this revision to Diff 28577. chinmoyr added a comment. Added support for MSG_SLAVE_STATUS REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10822?vs=28543&id=28577 BRANCH D10822 REVISION DETAIL https://phabricator.kde.org/D10822 AFFECTED FILES src/core/idleslave.cpp src/core/idleslave.h To: chinmoyr, dfaure Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
dfaure added a comment.
Thanks. Can you just add one comment, for the future?
INLINE COMMENTS
> idleslave.cpp:91
> +}
> +} else {
> +if (!stream.atEnd()) {
// compat code for KF < 5.45. TODO KF6: remove
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10822
To: chinmoyr, dfaure
Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
chinmoyr updated this revision to Diff 28581. chinmoyr added a comment. added the required comment REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10822?vs=28577&id=28581 BRANCH D10822 REVISION DETAIL https://phabricator.kde.org/D10822 AFFECTED FILES src/core/idleslave.cpp src/core/idleslave.h To: chinmoyr, dfaure Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks (I just realized we need a similar comment next to the enum value MSG_SLAVE_STATUS though) REPOSITORY R241 KIO BRANCH D10822 REVISION DETAIL https://phabricator.kde.org/D10822 To: chinmoyr, dfaure Cc: #frameworks, michaelh
D10822: Store temporary authorization status in IdleSlave
This revision was automatically updated to reflect the committed changes. Closed by commit R241:d38b373c8b76: Store temporary authorization status in IdleSlave (authored by chinmoyr). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10822?vs=28581&id=31254 REVISION DETAIL https://phabricator.kde.org/D10822 AFFECTED FILES src/core/idleslave.cpp src/core/idleslave.h To: chinmoyr, dfaure Cc: #frameworks, michaelh, ngraham
