[ksmserver] Remove obsolete KDELibs4Support dependency
ClosedPublic

Authored by bruns on Jul 6 2019, 11:02 AM.

Details

Summary

The inhibition was dysfunctional, as powerdevil delays the inhibition
by 5 seconds (and is already stopped when the delay times out). Also
powerdevil handles concurrent user session shutdowns and sleep requests
itself, so the code is obsolete.

As KDELibs4Support pulled in a number of Frameworks, we have to
explicitly add these now.

Diff Detail

Repository
R120 Plasma Workspace
Branch
ksmserver_no_kdelibs4support
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13789
Build 13807: arc lint + arc unit
bruns created this revision.Jul 6 2019, 11:02 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 6 2019, 11:02 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bruns requested review of this revision.Jul 6 2019, 11:02 AM
apol added a subscriber: apol.Jul 6 2019, 11:26 AM

Solid::InhibitionJob isn't installed by default, is it?
Is the plan to make it the public API now?

Not disagreeing with the change, just would need changes in Solid first.

bruns added a comment.Jul 6 2019, 2:22 PM

After having looked into the callchain again, I am not sure what the best action here is:

The old code did a DBus call of org.freedesktop.PowerManagement.Inhibit, which is implemented by powerdevil. Powerdevil only "schedules" the inhibition, but delays it for 5 seconds:
https://github.com/KDE/powerdevil/blob/master/daemon/powerdevilpolicyagent.cpp#L521

... but closing the session triggers a shutdown of powerdevil, i.e. the inhibition was not enforced at all (we only did a pointless synchronous DBus call to powerdevil).

What about just removing the code?

sitter added a subscriber: sitter.Jul 8 2019, 8:49 AM

FTR: The async solid power API is not built by default as it is unfinished, it's also not API stable. I recently poked Alex Fiestas about it and he said that the parts of the new async API that actually were implemented when he handed over maintainer ship of solid were more or less done, there were some open concerns over the job class itself as supposedly that duplicates kjob a bit. I haven't had a closer look beyond that though.
So, in order to port to the API, it'd first need finishing up really.

bruns added a comment.Jul 8 2019, 4:55 PM

So, in order to port to the API, it'd first need finishing up really.

The question is - do we really want this call here, or not. As is, it is just an expensive noop.

Iff we want the inhibition, I see 3 possibilities:

  1. Teach powerdevil to behave correctly:
    • do the inhibition immediately
    • keep running until all proxied inhibitions are released (maybe this is inversed - do not kill powerdevil from ksmserver).
    • somehow interact with powerdevil from ksmserver
  2. Take an inhibitor lock via DBus (i.e. call org.freedesktop.login1.Inhibit) directly
  3. Take an inhibitor lock using Solid::Power::inhibit

(3.) is nothing more than a thin wrapper around (2.) see https://github.com/KDE/solid/blob/master/src/solid/power/backends/freedesktop/fdinhibition.cpp
Although Solid::Power would give some abstraction, the reality is:

  • only blocking inhibitions are supported
  • only implementation is org.freedesktop.login1
  • Solid::Power is very incomplete, e.g. the statesJob is just a stub.

My preference is (0.) or (2.).

anthonyfieroni added inline comments.
ksmserver/logout.cpp
389–390

Make job autodelete if not.

Given it's already broken, I would say just kill it.

I've not had any related bug reports.

PowerDevil checks if (KWorkSpace::isShuttingDown()) { before suspending, so I think the ksmserver stuff is obsolete

bruns marked an inline comment as done.Jul 8 2019, 6:35 PM
bruns added inline comments.
ksmserver/logout.cpp
389–390

This is no KJob

bruns updated this revision to Diff 61475.Jul 10 2019, 12:20 AM
bruns marked an inline comment as done.
bruns retitled this revision from Port KSMServer to Solid::Power, drop KDELibs4Support requirement to [ksmserver] Remove obsolete KDELibs4Support dependency.
bruns edited the summary of this revision. (Show Details)
bruns removed subscribers: anthonyfieroni, sitter, apol.

remove inhibition

davidedmundson accepted this revision.Jul 10 2019, 12:23 AM
This revision is now accepted and ready to land.Jul 10 2019, 12:23 AM
This revision was automatically updated to reflect the committed changes.