Get rid of KDELibs4Support
ClosedPublic

Authored by denisshienkov on Mar 5 2017, 9:19 AM.

Details

Summary

The power management code is adapted from the kscreenlocker project, and added to the "powerdevilcode" library. Also the missed canHybridSuspend property is added to this code.

Diff Detail

Repository
R122 Powerdevil
Lint
Lint Skipped
Unit
Unit Tests Skipped
denisshienkov created this revision.Mar 5 2017, 9:19 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 5 2017, 9:19 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

While the QPointer changes look sensible, you cannot just remove the Solid Power stuff as you would essentially remove the ability to suspend and hibernate. I know it's in KDELibs4Support and we don't have a replacement but just removing it is not an option.

volkov edited edge metadata.Mar 6 2017, 10:07 AM

Please, replace QWeakPointer with QPointer in a separate commit.

As for Solid::PowerManagement, it was actually added to the solid library as Solid::Power, but it's not built by default.
Try to port to that new API.

but it's not built by default. Try to port to that new API.

Unfortunately Solid::Power is just as dead as Solid::PowerManagement... sorry.

Will it be hard to reanimate it?

Actually we depend only on Solid::PowerManagement::supportedSleepStates().
But there is PowerDevil::BackendInterface::supportedSuspendMethods().
I guess it would be right to use it instead.

Will it be hard to reanimate it?

Well, once you release it you can never fundamentally change it, so it needs to be really thought through.

Solid::PowerManagement::supportedSleepStates() seems to be calling PowerDevil itself over DBus, so indeed it could probably be bypassed and do that inside of PowerDevil.

Will it be hard to reanimate it?

Yes and no.

It wouldn't be hard to finish it as-is, but it's an implementation of this specification:

https://www.freedesktop.org/wiki/Specifications/power-management-spec/
which says
"This spec is considered obsolete. "

We still use it in Powerdevil, so it works (at least in Plasma), but it's an odd thing to add to new code.

So, what is final decision? Should I "abandon" it?

volkov added a comment.Mar 6 2017, 1:00 PM
  1. Extract QWeakPointer/QPointer change into a separate commit.
  2. Use PowerDevil::BackendInterface::supportedSuspendMethods() instead of Solid::PowerManagement::supportedSleepStates().

Just as an FYI: I reimplemented parts of the API in kscreenlocker. This code could be used here as well. We just need to split it out into an own library - could be inside Plasma.

How about this? :)

denisshienkov edited the summary of this revision. (Show Details)Mar 6 2017, 7:39 PM
ltoscano set the repository for this revision to R122 Powerdevil.Mar 6 2017, 9:43 PM
volkov added inline comments.Mar 7 2017, 10:36 AM
daemon/CMakeLists.txt
111

I guess it can be just removed. These kf5_org.* files were dropped from solid three years ago.

daemon/actions/bundled/CMakeLists.txt
22

Is it necessary to add KF5::Solid?

daemon/powerdevilcore.cpp
102 ↗(On Diff #12250)

And who will set backend for Core?

Now I think that it was a bad idea to request supported suspend methods from a backend,
because GUI doesn't depend on it. You need to load the backend in GUI, it will do some initialization...

It seems to be more reasonable to request supported methods from powerdevil daemon by dbus calllings
org.freedesktop.PowerManagement.{CanSuspend, CanHibernate, CanHybridSuspend}

107 ↗(On Diff #12250)

unrelated

kcmodule/activities/activitypage.cpp
76 ↗(On Diff #12250)

Shouldn't it be in a separate change?

denisshienkov added inline comments.Mar 7 2017, 12:45 PM
daemon/CMakeLists.txt
111

Where I need to take an appropriate XML files then?

daemon/actions/bundled/CMakeLists.txt
22

I will try later.

daemon/powerdevilcore.cpp
102 ↗(On Diff #12250)

It seems to be more reasonable to request supported methods from powerdevil daemon by dbus calllings

Ok, maybe then I can take a suggestion from @graesslin ?

"Just as an FYI: I reimplemented parts of the API in kscreenlocker. This code could be used here as well."

107 ↗(On Diff #12250)

ok

kcmodule/activities/activitypage.cpp
76 ↗(On Diff #12250)

Then it does not compiled with Qt5 (without of KDELibs4Support).

volkov added inline comments.Mar 7 2017, 1:26 PM
daemon/CMakeLists.txt
111

Copy them here.

daemon/powerdevilcore.cpp
102 ↗(On Diff #12250)

Yes, powermanagement.{h, cpp} from kscreenlocker is what you need.
The question is whether to create a library from it?

kcmodule/activities/activitypage.cpp
76 ↗(On Diff #12250)

Why don't make QWeakPointer -> QPointer commit first and base this one on it?

denisshienkov added inline comments.Mar 7 2017, 6:18 PM
daemon/powerdevilcore.cpp
102 ↗(On Diff #12250)

The question is whether to create a library from it?

I don't know. It is not in my competence. I just can do copy/paste a code from kscreenlocker.

kcmodule/activities/activitypage.cpp
76 ↗(On Diff #12250)

Why don't make QWeakPointer -> QPointer

  1. Then, I suggest do not use QPointer at all, as it is overhead for this case. I suggest to use the pointers directly..

commit first and base this one on it?

I don't know how to do it with phabricator. It is my first issue in here. :)

denisshienkov added inline comments.Mar 7 2017, 7:29 PM
kcmodule/activities/activitypage.cpp
76 ↗(On Diff #12250)

I don't know how to do it with phabricator. It is my first issue in here. :)

Please see D4970

denisshienkov edited the summary of this revision. (Show Details)
denisshienkov edited the summary of this revision. (Show Details)
denisshienkov set the repository for this revision to R122 Powerdevil.
denisshienkov marked an inline comment as done.Mar 14 2017, 7:25 PM
denisshienkov added inline comments.
daemon/CMakeLists.txt
111

Are you sure that I need to copy a files as kf5_org.freedesktop.PowerManagement.xml and kf5_org.freedesktop.PowerManagement.Inhibit.xml from: /usr/share/dbus-1/interfaces/ to this project? What for?

volkov added inline comments.Mar 15 2017, 9:59 AM
daemon/CMakeLists.txt
111

Where do they come from in /usr/share/dbus-1/interfaces/ ?

denisshienkov added inline comments.Mar 16 2017, 6:00 AM
daemon/CMakeLists.txt
111

Oops, from KDELibs4Support :).

denisshienkov updated this revision to Diff 12509.EditedMar 16 2017, 6:03 AM
denisshienkov marked an inline comment as done.

I have added missed "canHybridSuspend" property to class PowerManagement.

Should I to add a new entry like "<method name="HybridSuspend"/>" to org.freedesktop.PowerManagement.xml file and implement the PowerManagement::hybridSuspend() method?

denisshienkov edited the summary of this revision. (Show Details)

Looks good to me.

powerdevilpowermanagement.{h,cpp} should be extracted into a library, but it can be done in a separate commit.
Since they deal with org.freedesktop.PowerManagement interface which is obsolete and implemented only by
PowerDevil, I suppose such a library should be a part of PowerDevil.

davidedmundson accepted this revision.Mar 29 2017, 9:51 AM
This revision is now accepted and ready to land.Mar 29 2017, 9:51 AM

Who can do push this patch? As I have not rigths and have not a developer account.

This revision was automatically updated to reflect the committed changes.

Done, sorry about that delay.

broulik edited edge metadata.Apr 26 2017, 9:19 PM

Can someone please fix the breakage of the suspend methods in the KCMs? In PowerDevil settings the "What to do when closing the lid" and other combos are missing them because they buildUi() runs initially and checks whether is supported at a time where the async query has not yet returned…