[Kde-hardware-devel] Re: Review Request: Use new KDE Power Management System's PolicyAgent for suppressing sleep and screen power management

[Dario Freddi]

> On 2010-11-02 07:24:38, Kevin Ottens wrote:
> > A very good start already, a couple of improvements would be welcome though. Looking forward to the second revision of this patch. ;-)

Awesome :) could you please comment on the DBus interface as well? Do you think it's fine this way?


> On 2010-11-02 07:24:38, Kevin Ottens wrote:
> > /trunk/KDE/kdelibs/solid/solid/powermanagement.cpp, line 34
> > <http://svn.reviewboard.kde.org/r/5729/diff/1/?file=40466#file40466line34>
> >
> >     Would be good to keep the inhibitIface around and fallback to it when policyAgentIface is invalid. Keep in mind that this library could in theory be used in an environment where the org.freedesktop.PowerManagement* ifaces would be the only ones provided.

Definitely, you are right. I'll add support for a fallback then.


> On 2010-11-02 07:24:38, Kevin Ottens wrote:
> > /trunk/KDE/kdelibs/solid/solid/powermanagement.cpp, line 96
> > <http://svn.reviewboard.kde.org/r/5729/diff/1/?file=40466#file40466line96>
> >
> >     Why the check on the baseService? Care to enlighten me? (maybe a comment explaining that would be welcome).

Definitely, I totally forgot that. Long story short: the Policy Agent is capable of watching over a DBus service associated with the application, and automatically release the inhibitions it has once the dbus service disappears, which means either the developers suck because they didn't release, or the app crashed. So your PC won't be stuck because of a dead application. With the check for the baseService we make sure the app is registered to the bus.


> On 2010-11-02 07:24:38, Kevin Ottens wrote:
> > /trunk/KDE/kdelibs/solid/solid/powermanagement.cpp, line 98
> > <http://svn.reviewboard.kde.org/r/5729/diff/1/?file=40466#file40466line98>
> >
> >     Not a huge fan of magic numbers hidden deep in the code. Any chance you could use an enum internal to this namespace? Defined only in the cpp to not be used by third party code.

Makes more sense, you are right.


- Dario


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5729/#review8471
-----------------------------------------------------------


On 2010-10-29 17:01:09, Dario Freddi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5729/
> -----------------------------------------------------------
> 
> (Updated 2010-10-29 17:01:09)
> 
> 
> Review request for Solid and Kevin Ottens.
> 
> 
> Summary
> -------
> 
> With recent commits in kdebase/workspace/powerdevil, the KDE Power Management System has gained a new shiny PolicyAgent for handling inhibition requests, now with the killer feature of suppressing screen power management as well. This patch adds 2 more methods to Solid::PowerManagement and the DBus interface for the policy agent. If the patch will be accepted, the interface will be of course removed from powerdevil/, given that it is installed from within kdelibs.
> 
> Side effect - this patch also removes the usage of the fd.o interface from within Solid - even if the PolicyAgent is still compatible with it.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/solid/solid/org.kde.Solid.PowerManagement.PolicyAgent.xml PRE-CREATION 
>   /trunk/KDE/kdelibs/solid/solid/powermanagement.h 1190634 
>   /trunk/KDE/kdelibs/solid/solid/CMakeLists.txt 1190634 
>   /trunk/KDE/kdelibs/solid/solid/powermanagement.cpp 1190634 
>   /trunk/KDE/kdelibs/solid/solid/powermanagement_p.h 1190634 
> 
> Diff: http://svn.reviewboard.kde.org/r/5729/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dario
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20101102/93c37b2f/attachment-0001.htm