[Kde-hardware-devel] Review Request: Handle unsupported actions quietly

[Oliver Henshaw]

> On Nov. 28, 2012, 10:54 a.m., Dario Freddi wrote:
> > powerdevil/daemon/powerdevilactionpool.h, line 61
> > <http://git.reviewboard.kde.org/r/107257/diff/2/?file=95657#file95657line61>
> >
> >     This value should be initialized to 0

I'm hesitant because just like callers shouldn't use it stale, they shouldn't use it uninitialised. What do you think?


> On Nov. 28, 2012, 10:54 a.m., Dario Freddi wrote:
> > powerdevil/daemon/powerdevilactionpool.h, lines 54-55
> > <http://git.reviewboard.kde.org/r/107257/diff/2/?file=95657#file95657line54>
> >
> >     For consistency, lastError()

I had this initially but, given the above, think error() is appropriate.


> On Nov. 28, 2012, 10:54 a.m., Dario Freddi wrote:
> > powerdevil/daemon/powerdevilactionpool.h, lines 37-41
> > <http://git.reviewboard.kde.org/r/107257/diff/2/?file=95657#file95657line37>
> >
> >     I would add a NoError = 0 at the beginning of the enum, since in case no error occurred we need a value notifying that.

I'm not so sure. I used kservice and kpluginloader as models for the error handling. One has an error() method and one passes the error in the arguments, but in both the error string could be stale if no error occured. Since callers shouldn't be checking the error code/string unless the returned pointer is null, I don't see any point in providing a NoError code.

Maybe I should document this in the header file?


- Oliver


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


On Nov. 21, 2012, 7:20 p.m., Oliver Henshaw wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107257/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2012, 7:20 p.m.)
> 
> 
> Review request for Solid.
> 
> 
> Description
> -------
> 
> Handle unsupported actions quietly
> 
> Attempting to load a configured action on a machine where it is not
> supported (e.g. DPMS when the display doesn't support it or it is not
> compiled in) fails and brings up a notification, something that is
> particularly intrusive during login.
> 
> Provide a method for ActionPool::loadAction callers to ask why the load
> failed. PowerDevil::Core::loadProfile uses this check to decide whether
> to warn to stderr rather than notifying the user of a misconfiguration.
> Other loadAction callers are unchanged.
> 
> Action loading failure may be due to an error during initialisation or
> simply because no such action exits. In the former case the error must
> be recorded so that it is available to pass on to later loadAction
> callers.
> 
> NB: A more complete fix might involve detecting whether the action is
> supportable when loading the action configuration and/or in the profile
> generator. However that may not turn out to be a feasible approach.
> 
> BUG: 302846
> 
> 
> Diffs
> -----
> 
>   powerdevil/daemon/powerdevilactionpool.h 8a94eacc8ef2c2aead8cb075cbc80b783c1aeb4c 
>   powerdevil/daemon/powerdevilactionpool.cpp a9950f174fe184b8faa54c54fc00654984c65b3f 
>   powerdevil/daemon/powerdevilcore.cpp 2dcdbc62236d5c1fae384fdb9111825a2ebf5204 
> 
> Diff: http://git.reviewboard.kde.org/r/107257/diff/
> 
> 
> Testing
> -------
> 
> Tested in VM with cirrus/vnc (dpms) and qxl/spice (non-dpms) graphics. Tested the NoAction and LoadFailed cases act as expected. Tested nothing horrible happens when disabling and re-enabling powerdevil in kded Services Manager.
> 
> 
> Thanks,
> 
> Oliver Henshaw
> 
>

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