[Kde-hardware-devel] Review Request 123262: ddccontrol support for PowerDevil

[Lamarque Souza]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123262/#review78529
-----------------------------------------------------------



daemon/backends/upower/ddchelper.h (line 6)
<https://git.reviewboard.kde.org/r/123262/#comment53712>

    In kde-dev-scripts:relicensecheck.pl you stated that code you write can be LGPv2+, could you reflect that in this and the other files you are creating with this patch?



daemon/backends/upower/ddchelper.h (line 26)
<https://git.reviewboard.kde.org/r/123262/#comment53713>

    Please do not use "using" inside a header file. That can lead to wierd compiling errors.



daemon/backends/upower/ddchelper.cpp (line 56)
<https://git.reviewboard.kde.org/r/123262/#comment53714>

    This can lead to crash if pair is empty. At least add a Q_ASSERT(pair.size == 2) here.



daemon/backends/upower/ddchelper.cpp (line 79)
<https://git.reviewboard.kde.org/r/123262/#comment53715>

    same here.



daemon/backends/upower/ddchelper.cpp (line 82)
<https://git.reviewboard.kde.org/r/123262/#comment53716>

    You could add a Q_ASSERT(m_maximumBrightness > 0) here to to catch any problem when converting to int.



daemon/backends/upower/ddchelper.cpp (line 135)
<https://git.reviewboard.kde.org/r/123262/#comment53717>

    In QProcess doc: "Returns true if the process finished; otherwise returns false (if the operation timed out, if an error occurred, or if this QProcess is already finished).". "Already finished" here is the same as "not started yet", you should use if (!dccProcess.waitForStarted() || !dccProcess.waitForFinished()). I must also say that I really do not like synchronous calls in kded or in a plasmoid.


- Lamarque Souza


On April 5, 2015, 3:44 p.m., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123262/
> -----------------------------------------------------------
> 
> (Updated April 5, 2015, 3:44 p.m.)
> 
> 
> Review request for Solid.
> 
> 
> Repository: powerdevil
> 
> 
> Description
> -------
> 
> This allows PowerDevil to use the ddccontrol utility to manipulate brightness of external monitors through Display Data Channel.
> 
> It becomes the last link in the fallback chain; it first tries XRandR, then it tries the sysfs helper, and if all that fails it tries to use the ddccontrol utility.
> 
> dcccontrol unfortunately doesn't have machine-readable output so I have to take apart its probing output to get the device name and id of the backlight controls. Also it is pretty slow both initializing (kded startup, the helper saves the values and writes directly to the address returned by the initial probing) and setting brightness (that one doesn't block but it can take seconds until your monitor actually changes brightness)
> 
> 
> Diffs
> -----
> 
>   daemon/BackendConfig.cmake 295a8a2 
>   daemon/backends/upower/ddc_helper_actions.actions PRE-CREATION 
>   daemon/backends/upower/ddchelper.h PRE-CREATION 
>   daemon/backends/upower/ddchelper.cpp PRE-CREATION 
>   daemon/backends/upower/powerdevilupowerbackend.h 1c4dd59 
>   daemon/backends/upower/powerdevilupowerbackend.cpp 87b9cc7 
> 
> Diff: https://git.reviewboard.kde.org/r/123262/diff/
> 
> 
> Testing
> -------
> 
> It's working pretty nicely, I can adjust brightness of my desktop monitor through battery monitor and have it automatically dimmed after a timeout.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

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