Details
- Reviewers
mpyne - Commits
- R344:8b9b0d553f80: Inhibit power management when playing audio.
Diff Detail
- Repository
- R344 Juk
- Branch
- master-pm (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 266 Build 266: arc lint + arc unit
The patch looks good (though you say it doesn't actually work??)
However power management is not what the PlayerManager class should be responsible for, so the power management inhibition commands should not be implemented in playermanager.cpp
Instead I would say to connect the PlayerManager signals to the appropriate Solid::Inhibition slots (setup the connections in juk.cpp in the same spot where you're creating the new inhibition.
That will avoid mixing power management concerns with playback code (the player manager is if anything already too mixed in with the GUI though I haven't been able to untangle it all yet).
I'm sorry for wasting your time yesterday, especially since it takes so long between reviews. But after reading more into the Solid code it looks like we actually have to process this as a multiple step process.
- Call Solid::Power::inhibition(...) in JuK::JuK to obtain an InhibitionJob, which must be started and have its result signal connected to a new slot in JuK.
- In that new slot, cast the Solid::Job to a Solid::InhibitionJob and then call its inhibition method if no error was found to obtain an Inhibition. Connect that PlayerManager's signals as you've already done to the Inhibition object.
See the comments for detail.
I think you had mentioned that the code didn't work for you, I think this would explain why.
| juk.cpp | ||
|---|---|---|
| 128 | This allocation will immediately leak since job is overwritten on the next line. | |
| 129 | This could probably just be something like (void) Solid::Power::inhibit(Solid::Power::Sleep, QLatin1Literal(...), this); (note that we need to add this as the parent object). The return value to this function is not an Inhibition, but an InhibitionJob. So we also need to start the job, and connect its result signal to a new slot that actually extracts the Inhibition object. | |
| 131 | Since we can't actually call job->inhibition() until the job's result is emitted, these connect calls actually need to be moved to the new slot I mentioned above. That is the new slot just needs to extract the Inhibition object, setup these connections, and go. I think it's OK to let the Inhibition object leak in the slot since it's a one-time call but if you want you can hold onto the pointer by adding a new Solid::Inhibition member to juk.h. I apologize for telling you to move it here in the first place, I didn't realize the mistake until I read closer into Solid's source code (half of these APIs seem to be completely undocumented :( ). But I think that explains why it wasn't working in your testing. | |
This revision is probably closer to what an implementation could look like. Solid::Inhibition::result returns a Solid::Job which doesn't look usable (I would have expected a Solid::InhibitionJob here). Though it still doesn't inhibit power management that I can tell. I wonder if the API is finished and known working? Dragon's code currently seems to use DBus where I mistakenly thought it used code similar to the first revision.
It still not be working because you start the inhibition job before you've setup the slot to accept the result.
Also, the Solid::Job you're talking about is because redeclaring that method to pass a Solid::InhibitionJob would technically be a different method/slot. If you use that value you're expected to downcast it back to Solid::InhibitionJob. Of course just capturing the local job should work just as well.
I'd say try it with calling inhibitJob->start *after* the slots are setup and see if that works. But I agree this should be closer.
Starting the job after the result slot is connected doesn't make a difference, since we aren't using the result slot to drive the player state. Recasting the Solid::Job in the result slot doesn't appear to make a difference either.
It seems that the support in Solid is still experimental as this didn't even compile for me now that I've finally been able to test it. Turns out it's an optional feature in Solid which is disabled by default.
All the same I think it looks OK so it might be a matter of more debugging outputs and looking into Solid itself. I'll try playing around with that tonight.
Of course I "experimental" when the last commit to the relevant code in Solid seems to have been in 2014... I suppose it's as good as it will get for now. But still I will want to fix our own CMake checks first before I recompile Solid with the support enabled.
Both KTorrent and Dragon currently do this over DBus, so my guess is that the Solid power inhibition API is not in a workable state, or is otherwise lacking.
It may be the case that Solid isn't ready for this yet. The code in Solid's freedesktop backend (solid/src/solid/power/backends/freedesktop/fdinhibition.cpp) appears to handle this DBus stuff as well but even though I made sure to enable the fdo backend, it doesn't work for me either.
In the meantime I made up the CMake check I was talking about in case we decide to still go with Solid (and maybe try to fix the code).
diff --git a/CMakeLists.txt b/CMakeLists.txt index 6b28f23..128cfd0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -45,6 +45,13 @@ set_package_properties(FEATURE PROPERTIES DESCRIPTION "A library for developing # with whatever is actually current this decade. set(HAVE_TUNEPIMP 0) +# Check to see if we can use Solid power inhibition +# check_include_file_cxx doesn't work as it won't transitively pull in CMake +# exported target dependencies in the way that try_compile will. +try_compile(JUK_SOLID_HAS_NEW_POWER_APIS ${CMAKE_BINARY_DIR}/tmp + SOURCES ${CMAKE_SOURCE_DIR}/cmake/CheckSolidPower.cxx + LINK_LIBRARIES KF5::Solid) + ########### next target ############### include_directories( ${TAGLIB_INCLUDES} ) diff --git a/cmake/CheckSolidPower.cxx b/cmake/CheckSolidPower.cxx index e69de29..3f926a6 100644 --- a/cmake/CheckSolidPower.cxx +++ b/cmake/CheckSolidPower.cxx @@ -0,0 +1,4 @@ +// Used for CMake Solid feature check +#include <Solid/Inhibition> +#include <Solid/InhibitionJob> +int main() { return 0; }
It may be easier to manually do the DBus call(s) ourselves though.
I've tested, it compiles and installs fine and seems to work in my testing with qdbusviewer to verify that the inhibition is set during playback. I'm not sure of a better way to test on my system since I don't go crazy with power management anyways but it's at least good enough to commit and get out to a wider user base for testing.
Thanks for the patch!