Store temporary authorization status in IdleSlave
ClosedPublic

Authored by chinmoyr on Feb 25 2018, 10:49 AM.

Details

Summary

It will be used by klauncher to decide whether or not to kill the IdleSlave.

Depends on D10820

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Feb 25 2018, 10:49 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 25 2018, 10:49 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr requested review of this revision.Feb 25 2018, 10:49 AM
dfaure requested changes to this revision.Mar 1 2018, 7:01 AM
dfaure added inline comments.
src/core/idleslave.cpp
80–81

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.

This revision now requires changes to proceed.Mar 1 2018, 7:01 AM
chinmoyr updated this revision to Diff 28543.Mar 4 2018, 6:38 AM

Used MSG_SLAVE_STATUS_V2

dfaure requested changes to this revision.Mar 4 2018, 10:43 AM
dfaure added inline comments.
src/core/idleslave.cpp
70

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").

This revision now requires changes to proceed.Mar 4 2018, 10:43 AM
chinmoyr updated this revision to Diff 28577.Mar 4 2018, 12:00 PM

Added support for MSG_SLAVE_STATUS

Thanks. Can you just add one comment, for the future?

src/core/idleslave.cpp
91

// compat code for KF < 5.45. TODO KF6: remove

chinmoyr updated this revision to Diff 28581.Mar 4 2018, 12:15 PM

added the required comment

dfaure accepted this revision.Mar 4 2018, 12:18 PM

Thanks (I just realized we need a similar comment next to the enum value MSG_SLAVE_STATUS though)

This revision is now accepted and ready to land.Mar 4 2018, 12:18 PM
This revision was automatically updated to reflect the committed changes.