D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Plasma, broulik, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bruns requested review of this revision.

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  ksmserver_no_kdelibs4support

REVISION DETAIL
  https://phabricator.kde.org/D22296

AFFECTED FILES
  ksmserver/CMakeLists.txt
  ksmserver/logout.cpp
  ksmserver/server.h

To: bruns, #plasma, broulik, davidedmundson
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Aleix Pol Gonzalez]

apol added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: apol, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Stefan Brüns]

bruns added a comment.


  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?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: apol, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Harald Sitter]

sitter added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: sitter, apol, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Stefan Brüns]

bruns added a comment.


  In D22296#492147 , @sitter wrote:
  
  >
  
  
  
  
  > 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.).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: sitter, apol, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> logout.cpp:399-400
>  } else {
> -Solid::PowerManagement::stopSuppressingSleep(inhibitCookie);
> +delete inhibitionJob;
> +inhibitionJob = nullptr;
>  qCDebug(KSMSERVER) << "Client " << c->program() << " (" << 
> c->clientId() << ") canceled shutdown.";

Make job autodelete if not.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: anthonyfieroni, sitter, apol, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[David Edmundson]

davidedmundson added a comment.


  Given it's already broken, I would say just kill it.
  
  I've not had any related bug reports.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: anthonyfieroni, sitter, apol, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: anthonyfieroni, sitter, apol, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D22296: Port KSMServer to Solid::Power, drop KDELibs4Support requirement

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in logout.cpp:399-400
> Make job autodelete if not.

This is no KJob

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22296

To: bruns, #plasma, broulik, davidedmundson
Cc: anthonyfieroni, sitter, apol, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart