[Kde-hardware-devel] Review Request: Only add DPMS to the profile if it is supported

[Oliver Henshaw]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107503/#review22730
-----------------------------------------------------------


Sorry, this doesn't look right:


powerdevil/daemon/powerdevilprofilegenerator.cpp
<http://git.reviewboard.kde.org/r/107503/#comment17368>

    This won't work because isSupported() is a non-static member function - it needs an object. It shouldn't compile, but it does because...



powerdevil/daemon/powerdevilprofilegenerator.cpp
<http://git.reviewboard.kde.org/r/107503/#comment17369>

    I don't understand why, but these ifdef's seem to evaluate to false even when dpms support is available.


So powerdevilprofilegenerator generates configurations with no DPMS action unconditionally.

What's the best thing to do? Maybe move all the code that touches the DPMS extension (and so needs ifdef'ing) out of powerdevildpmsaction (into non-member functions, maybe in a private include file) leaving only the core action logic. And then the profile generator (and handlebuttoneventsconfig, but that's just cosmetic) could use that. I made a start on a half-baked patch series to do the code movement, but it's currently on the shelf as it raised some questions abut xlib error handling and it didn't seem necessary given the approach in review #107257. I hadn't planned to revisit it until after 4.10 branched.

A slightly quicker fix is just to move the isSupported code to a non-member function. You'd still need to make sure that the x error handling is sane in the function and in the action though (I suspect that using kxerrorhandler from kdelibs/kdeui/util is the best way forward).

The most-short-term option, and the one I favour, is to revert this and try the ifdef'd powerdevildpmsaction stub (from review #106863) instead.


P.S. Are you able to test the DPMS code paths in a virtual machine? Why don't you have DPMS support available anyway?


- Oliver Henshaw


On Nov. 28, 2012, 2:14 p.m., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107503/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2012, 2:14 p.m.)
> 
> 
> Review request for Solid and Oliver Henshaw.
> 
> 
> Description
> -------
> 
> I found out that the Powerdevilprofilegenerator which is fired on first start of a KDE session which generates the profile and sets the defaults adds the DPMS action no matter if it is compiled or supported. That patch makes it only add it when it is compiled (HAVE_DPMS) and the action returns isSupported(). This will silence the most prominend warning about missing DPMS and having something in the config file which doesn't work is not good.
> Combined with the patch by Oliver Henshaw this will make PowerDevil not stick in your face on startup anymore (except for that Battery Low stuff I added :D which I am thinking of a fix)
> 
> 
> Diffs
> -----
> 
>   powerdevil/daemon/powerdevilprofilegenerator.cpp 4cdbe11 
> 
> Diff: http://git.reviewboard.kde.org/r/107503/diff/
> 
> 
> Testing
> -------
> 
> Deleted my powermanagementrc, Tested *without* DPMS, works flawlessly. Cannot test with since I do not have DPMS support.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20121128/395a6da0/attachment-0001.html>