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

[Kevin Ottens]

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


A very good start already, a couple of improvements would be welcome though. Looking forward to the second revision of this patch. ;-)


/trunk/KDE/kdelibs/solid/solid/powermanagement.cpp
<http://svn.reviewboard.kde.org/r/5729/#comment8839>

    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.



/trunk/KDE/kdelibs/solid/solid/powermanagement.cpp
<http://svn.reviewboard.kde.org/r/5729/#comment8840>

    Why the check on the baseService? Care to enlighten me? (maybe a comment explaining that would be welcome).



/trunk/KDE/kdelibs/solid/solid/powermanagement.cpp
<http://svn.reviewboard.kde.org/r/5729/#comment8838>

    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.


- Kevin


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/537568c6/attachment.htm