D19389: Rewrite kworkspace logout, shutdown and suspend API
This revision was automatically updated to reflect the committed changes. Closed by commit R120:cd3e9a291a66: Rewrite kworkspace logout, shutdown and suspend API (authored by davidedmundson). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D19389?vs=60969&id=61013#toc REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19389?vs=60969&id=61013 REVISION DETAIL https://phabricator.kde.org/D19389 AFFECTED FILES libkworkspace/CMakeLists.txt libkworkspace/kworkspace.cpp libkworkspace/kworkspace.h libkworkspace/kworkspace_p.h libkworkspace/login1_manager_interface.cpp libkworkspace/loginddbustypes.h libkworkspace/org.freedesktop.ConsoleKit.Manager.xml libkworkspace/org.freedesktop.UPower.xml libkworkspace/org.freedesktop.login1.Manager.xml libkworkspace/org.freedesktop.login1.Seat.xml libkworkspace/org.freedesktop.login1.Session.xml libkworkspace/org.freedesktop.login1.User.xml libkworkspace/sessionmanagement.cpp libkworkspace/sessionmanagement.h libkworkspace/sessionmanagementbackend.cpp libkworkspace/sessionmanagementbackend.h libkworkspace/tests/CMakeLists.txt libkworkspace/tests/sessiontest.cpp To: davidedmundson, #plasma, broulik Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
broulik accepted this revision.
broulik added inline comments.
This revision is now accepted and ready to land.
INLINE COMMENTS
> loginddbustypes.h:105
> +{
> +public:
> +uint userId;
It's a `struct` already
REPOSITORY
R120 Plasma Workspace
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D19389
To: davidedmundson, #plasma, broulik
Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2,
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg,
abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
davidedmundson updated this revision to Diff 60969. davidedmundson marked an inline comment as done. davidedmundson added a comment. Keep canSwitchUser behaviour from KDisplayManager This patch fixes up libkworkspace all the powerdevil code, all the shutdown code and all the ksmserver calls. Plan was to do the session switching stuff next patch canSwitchUser is an annoying hybrid of both. So for now to address Kai's comment I can just use KDisplayManager. When an improved session switching API lands we can update this REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19389?vs=60269&id=60969 BRANCH master REVISION DETAIL https://phabricator.kde.org/D19389 AFFECTED FILES libkworkspace/CMakeLists.txt libkworkspace/kworkspace.cpp libkworkspace/kworkspace.h libkworkspace/kworkspace_p.h libkworkspace/login1_manager_interface.cpp libkworkspace/loginddbustypes.h libkworkspace/org.freedesktop.ConsoleKit.Manager.xml libkworkspace/org.freedesktop.UPower.xml libkworkspace/org.freedesktop.login1.Manager.xml libkworkspace/org.freedesktop.login1.Seat.xml libkworkspace/org.freedesktop.login1.Session.xml libkworkspace/org.freedesktop.login1.User.xml libkworkspace/sessionmanagement.cpp libkworkspace/sessionmanagement.h libkworkspace/sessionmanagementbackend.cpp libkworkspace/sessionmanagementbackend.h libkworkspace/tests/CMakeLists.txt libkworkspace/tests/sessiontest.cpp To: davidedmundson, #plasma Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
broulik added inline comments. INLINE COMMENTS > davidedmundson wrote in sessionmanagement.cpp:95 > I don't see where it did KDisplayManager::isSwitchable() checks `CanMultiSession` on the session manager. and there's also `numReserve()` which seems to be hardcoded to `1` for more modern systems, so that might not be an issue. > davidedmundson wrote in sessionmanagement.h:93 > We did, but that isn't needed now. > > If you want low level, you can use the SessionManagementBackend to just > perform the action directly Right > davidedmundson wrote in sessionmanagementbackend.cpp:200 > Honestly, because it's super legacy and I don't really want to spend any time > on it. Especially as I need to ask others to test again. > > The old code blocked, anyway Ah, right consolekit1, fine then. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D19389 To: davidedmundson, #plasma Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
davidedmundson added inline comments. INLINE COMMENTS > broulik wrote in sessionmanagement.cpp:95 > The old one also checked KDM or whatever for "free ttys", is that still a > thing? I don't see where it did > broulik wrote in sessionmanagement.h:93 > Didn't we have a "don't ask" thing that also doesn't ask for apps to close? We did, but that isn't needed now. If you want low level, you can use the SessionManagementBackend to just perform the action directly > broulik wrote in sessionmanagementbackend.cpp:63 > what's the TODO? KConfigWatcher support, but that's part of a bigger task. It's not worse than the current state. > broulik wrote in sessionmanagementbackend.cpp:200 > Why is logind all async but consolekit is not? :) Honestly, because it's super legacy and I don't really want to spend any time on it. Especially as I need to ask others to test again. The old code blocked, anyway REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D19389 To: davidedmundson, #plasma Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
davidedmundson updated this revision to Diff 60269. davidedmundson marked 6 inline comments as done. davidedmundson added a comment. Review comments. Also now doesn't break old API so we don't have to merge all at once. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19389?vs=59811&id=60269 BRANCH master REVISION DETAIL https://phabricator.kde.org/D19389 AFFECTED FILES libkworkspace/CMakeLists.txt libkworkspace/kworkspace.cpp libkworkspace/kworkspace.h libkworkspace/kworkspace_p.h libkworkspace/login1_manager_interface.cpp libkworkspace/loginddbustypes.h libkworkspace/org.freedesktop.ConsoleKit.Manager.xml libkworkspace/org.freedesktop.UPower.xml libkworkspace/org.freedesktop.login1.Manager.xml libkworkspace/org.freedesktop.login1.Seat.xml libkworkspace/org.freedesktop.login1.Session.xml libkworkspace/org.freedesktop.login1.User.xml libkworkspace/sessionmanagement.cpp libkworkspace/sessionmanagement.h libkworkspace/sessionmanagementbackend.cpp libkworkspace/sessionmanagementbackend.h libkworkspace/tests/CMakeLists.txt libkworkspace/tests/sessiontest.cpp To: davidedmundson, #plasma Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
broulik added inline comments.
INLINE COMMENTS
> loginddbustypes.h:103
> +
> +class NamedUserPath
> +{
You're using `struct` and `class` with all public inconsistently
> loginddbustypes.h:135
> +QString mode;
> +int userId;
> +uint processId;
`userId` is unsigned
> sessionmanagement.h:74
> +
> +
> +SessionManagement(QObject *parent = nullptr);
Whitespace
> sessionmanagement.h:80
> +
> +bool canShutdown();
> +bool canReboot();
Why are these not `const`?
> sessionmanagement.h:93
> + * These requestX methods will either launch a prompt to shutdown or
> + * The user may cancel it at any point in the process
> + */
Didn't we have a "don't ask" thing that also doesn't ask for apps to close?
> sessionmanagementbackend.cpp:63
> +{
> +return m_kserverConfig->group("General").readEntry("ConfirmLogout",
> true); // TODO
> +}
what's the TODO?
> sessionmanagementbackend.cpp:84
> +} else {
> +// both "yes" and "challenge" will show up in the UI
> +*argToUpdate = reply.value() != QLatin1String("no") ? true :
> false;
Better explicitly check for `yes` and `challenge`, as there is also:
> If "na" is returned the operation is not available because hardware, kernel
> or drivers do not support it.
> sessionmanagementbackend.cpp:91
> +emit stateChanged();
> +emit canShutdownChanged();
> +emit canRebootChanged();
Can we emit those only if actually different now?
> sessionmanagementbackend.cpp:200
> +auto canStop = m_ck->CanStop();
> +canStop.waitForFinished();
> +m_canShutdown = canStop.value();
Why is logind all async but consolekit is not? :)
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D19389
To: davidedmundson, #plasma
Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2,
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg,
abetts, sebas, apol, mart
D19389: Rewrite kworkspace logout, shutdown and suspend API
davidedmundson updated this revision to Diff 59811. davidedmundson added a comment. Update REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19389?vs=55363&id=59811 REVISION DETAIL https://phabricator.kde.org/D19389 AFFECTED FILES libkworkspace/CMakeLists.txt libkworkspace/kworkspace.cpp libkworkspace/kworkspace_p.h libkworkspace/login1_manager_interface.cpp libkworkspace/loginddbustypes.h libkworkspace/org.freedesktop.ConsoleKit.Manager.xml libkworkspace/org.freedesktop.UPower.xml libkworkspace/org.freedesktop.login1.Manager.xml libkworkspace/org.freedesktop.login1.Seat.xml libkworkspace/org.freedesktop.login1.Session.xml libkworkspace/org.freedesktop.login1.User.xml libkworkspace/sessionmanagement.cpp libkworkspace/sessionmanagement.h libkworkspace/sessionmanagementbackend.cpp libkworkspace/sessionmanagementbackend.h libkworkspace/tests/CMakeLists.txt libkworkspace/tests/sessiontest.cpp To: davidedmundson, #plasma Cc: pino, broulik, plasma-devel, LeGast00n, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
