Conversation
|
Logging out of a session is the session's job. Shutting down a system is a system job. I'm talking as an LXQt dev, not as an LXQt representative: Until now, your suggestions were about cluttering LXQt's codes for this or that app to work without an lxqt-session. Well, that's the problem of this or that app. LXQt has a session and handles it gracefully. |
|
Thanks @tsujan! As I mentioned in an email to you, I'm happy to discuss this with you further privately if desired. |
This isn't a private matter ;) |
|
Thanks @tsujan, this pull request isn't the right place to have a broader discussion about systemd support at a high level. You are welcome to respond to my points in lxqt/lxqt-session#623 if you wish to continue the conversation publicly on GitHub. I remain available to discuss the design in an audio or video chat, which may be recorded and made publicly available afterward (if desired). |
palinek
left a comment
There was a problem hiding this comment.
This PR seems perfectly valid (and thoroughly explained 👍) and I agree with the logic (I'm just putting some nitpicking code-flow comments).
Ad changing the static function signature -> it is a private not-exposed symbol, so IMO we don't need to be too cautious here
lxqtpower/lxqtpowerproviders.cpp
Outdated
|
|
||
| bool SystemdProvider::canAction(Power::Action action) const | ||
| { | ||
| if (action == Power::PowerLogout) |
There was a problem hiding this comment.
I would prefer to put this into the pre-existing switch.
lxqtpower/lxqtpowerproviders.cpp
Outdated
|
|
||
| bool SystemdProvider::doAction(Power::Action action) | ||
| { | ||
| if (action == Power::PowerLogout) |
There was a problem hiding this comment.
Can this also be put into the switch and even the unified dbusCall used for all cases afterwards?
There was a problem hiding this comment.
Can this also be put into the switch
Done in commit d91ff3e
[Can] even the unified dbusCall used for all cases afterwards?
Possibly? From the docs:
CanPowerOff(), CanReboot(), CanHalt(), CanSuspend(), CanHibernate(), CanHybridSleep(), CanSuspendThenHibernate(), CanSleep(), CanRebootParameter(), CanRebootToFirmwareSetup(), CanRebootToBootLoaderMenu(), and CanRebootToBootLoaderEntry() test whether the system supports the respective operation and whether the calling user is allowed to execute it. Returns one of "na", "yes", "no", and "challenge".
The docs say nothing about the return value of the Reboot, PowerOff, Suspend, and Hibernate commands, so I'm not sure if it's safe to replace the calls to dbusCallSystemd (which has special handling for the string values "yes" and "challenge") with calls to dbusCall (which does not have such special handling). The existing behavior of the reboot, shutdown, suspend, or hibernate functionality could likely be cleaned up, but this code cleanup deserves to be implemented and tested as part of a separate PR rather than adding risk to this PR.
https://build.opensuse.org/request/show/1331460 by user sfalken + anag_factory - Added 0001-Support-logout-in-SystemdProvider.patch, for better systemd compaitibility and functionality, please see lxqt/liblxqt#372
|
@palinek is this good now? |
|
Seems OK. But to be honor, I haven't tested/run anything of these changes. |
I've no interest in uwsm but I'll run it for some days on my setup and if I notice no issue I'll merge it. |
Background
lxqtpowerprovides a provider mechanism. Providers are initialized inliblxqt/lxqtpower/lxqtpower.cpp
Lines 39 to 45 in 46711d9
useLxqtSessionProvideris true, thenLXQtProvideris initialized. This communicates withlxqt-sessionvia DBus.SystemdProvideris unconditionally initialized with lower precedence.When power actions are performed, the providers are tried in order:
liblxqt/lxqtpower/lxqtpower.cpp
Lines 57 to 81 in 46711d9
LXQtProvideris tried (ifuseLxqtSessionProvideris true), and thenSystemdProvideris tried. Thus we have established thatlxqtpowerdoes not depend onlxqt-session.Problem
When
SystemdProvideris in use (i.e., whenlxqt-sessionis not running),lxqt-leaveshows a grayed-out logout button, but the shutdown and reboot buttons are not grayed out.Evaluation
SystemdProvider::canActionandSystemdProvider::doActioninclude case statements forcase Power::PowerShutdownandcase Power::PowerReboot, but not forcase Power::PowerLogout:liblxqt/lxqtpower/lxqtpowerproviders.cpp
Lines 404 to 477 in 46711d9
Solution
As discussed in lxqt/lxqt-wayland-session#105 (comment), add the missing case, calling systemd's
TerminateSessionvia DBus. This is the equivalent ofloginctl terminate-session(man page) which is documented as follows:Implementation details
XDG_SESSION_IDis set by systemd-logind for a user session. If it's missing, we are not running inside a logind-managed session and consider the functionality unavailable. systemd does not offer aCanLogoutorCanTerminateSessionmethod analogous toCanReboot,CanPowerOff, etc. So we therefore check if we have anXDG_SESSION_IDand that the DBus service, object, and interface are present on the system bus, then consider the functionality available.For actually logging out the user,
TerminateSessiontakes a string ID rather than aninteractiveboolean like thePowerOffandRebootmethods, so thedbusCallSystemdfunction is not suitable. Instead, we modify thedbusCallmethod to support arbitrary arguments (with a default value of no arguments). If binary compatibility is a concern, I can leave the existing function signature alone and create a new signature for invoking DBus methods with arguments.Testing done
Tested in Fedora 43 under UWSM (without
lxqt-sessionrunning). Before this PR, the logout button was grayed out. After this PR, it is not grayed out, and clicking it logs out the session.