Rewrite kworkspace logout, shutdown and suspend API
ClosedPublic

Authored by davidedmundson on Feb 27 2019, 3:15 PM.

Details

Summary

The old code path is old and even sometimes uses the ICE protocol to
start a logout from ksmserver. It's quite archaic and unreadable,
KDisplayManager is even worse.

The new code consists of two parts.

A QML friendly front end QObject that we can register in a plugin that
can trigger actions such as prompting a logout or suspending the system
and a singleton that talks to the relevant backend.

Backends are:

  • Any logind interface
  • Consolekit2
  • Consolekit0.4 + UPower

New API loading is also entirely asyncronous.

There are some behavioural changes:

  • Creating a logout prompt is called directly instead of going via

ksmserver. The prompt then calls into ksmserver when accepted

  • Suspend is called directly instead of going via powerdevil through

some kdelibs4support solid code.

All DBus calls use generated XML files, instead of big QDBusMessage
commands. It's a bit overkill in terms of what we generate, but that's
ready for moving SessionModel in here and killing KDisplayManager.

Patch looks huge because of all the XML files, but is otherwise very
simple.

This is intended to be an API break, which is fine as kworkspace is only
to be used by plasma. The relevant DataSource will remain compatiable.

WIP: As I need to port all the calling code, and I want to port
SessionsModel (probably in a different patch)

Test Plan

Ran sessionstest on my logind machine
A BSD user tested CK1

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Feb 27 2019, 3:15 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 27 2019, 3:15 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Feb 27 2019, 3:15 PM

update one line

add hybrid suspend

+1
Nice and clean, no real complaints.

libkworkspace/kworkspace.cpp
105

:)

libkworkspace/sessionmanagement.cpp
33

*service

96

The old one also checked KDM or whatever for "free ttys", is that still a thing?

libkworkspace/sessionmanagementbackend.cpp
47

Coding style

52

Can this even be hit?

85

I would say yes. If one wants to hide from UI they can use KIOSK?
Docs perhaps need updating that Polkit is obviously prefered since kiosk is mostly UI and not real security

davidedmundson marked 3 inline comments as done.

updates

Wrap saveSession

pino added a subscriber: pino.Apr 3 2019, 9:57 PM
  • loginddbustypes.h requires a license header
  • SessionBackend needs a d-pointer to minimize BIC changes
  • I'd move all the SessionBackend subclasses in a private header; they are not exported anyway, so unusable for users of this public library

Sessionbackend.h is not installed.

I should rename it _p.h maybe

pino added a comment.Apr 4 2019, 5:21 AM

Sessionbackend.h is not installed.

sessionmanagement.h (not sessionbackend.h) is indeed installed, according to your patch. Considering that D20237: Port to new KWorkspace API is in another repository, then making it a public header is the only sane choice (please do not copy it, it would be even worse).

davidedmundson retitled this revision from [WIP] Rewrite kworkspace logout, shutdown and suspend API to Rewrite kworkspace logout, shutdown and suspend API.Jun 12 2019, 12:10 PM
davidedmundson edited the test plan for this revision. (Show Details)
broulik added inline comments.Jun 21 2019, 4:25 PM
libkworkspace/loginddbustypes.h
104

You're using struct and class with all public inconsistently

136

userId is unsigned

libkworkspace/sessionmanagement.h
75

Whitespace

81

Why are these not const?

94

Didn't we have a "don't ask" thing that also doesn't ask for apps to close?

libkworkspace/sessionmanagementbackend.cpp
64

what's the TODO?

85

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.

92

Can we emit those only if actually different now?

201

Why is logind all async but consolekit is not? :)

davidedmundson marked 6 inline comments as done.

Review comments.

Also now doesn't break old API so we don't have to merge all at once.

davidedmundson added inline comments.Jun 21 2019, 8:31 PM
libkworkspace/sessionmanagement.cpp
96

I don't see where it did

libkworkspace/sessionmanagement.h
94

We did, but that isn't needed now.

If you want low level, you can use the SessionManagementBackend to just perform the action directly

libkworkspace/sessionmanagementbackend.cpp
64

KConfigWatcher support, but that's part of a bigger task. It's not worse than the current state.

201

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

broulik added inline comments.Jun 23 2019, 9:14 AM
libkworkspace/sessionmanagement.cpp
96
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.

libkworkspace/sessionmanagement.h
94

Right

libkworkspace/sessionmanagementbackend.cpp
201

Ah, right consolekit1, fine then.

davidedmundson marked an inline comment as done.

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

broulik accepted this revision.Jul 2 2019, 4:19 PM
broulik added inline comments.
libkworkspace/loginddbustypes.h
106

It's a struct already

This revision is now accepted and ready to land.Jul 2 2019, 4:19 PM
This revision was automatically updated to reflect the committed changes.