Handle unsupported actions quietly

Review Request #107257 - Created Nov. 8, 2012 and updated

Information
Oliver Henshaw
kde-workspace
4.10
Reviewers
solid
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
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.

Issues

  • 5
  • 0
  • 0
  • 5
Description From Last Updated
So, if the action exists, you load it. And if it doesn't then you check if it is unsupported to ... Kai Uwe Broulik Kai Uwe Broulik
I would add a NoError = 0 at the beginning of the enum, since in case no error occurred we ... Dario Freddi Dario Freddi
For consistency, lastError() Dario Freddi Dario Freddi
This value should be initialized to 0 Dario Freddi Dario Freddi
It's a bit unclear to me why you need this, since you're advertising only the last error occurred. In fact, ... Dario Freddi Dario Freddi
Kai Uwe Broulik
Oliver Henshaw
Review request changed

Change Summary:

Record an appropriate error code during action initialisation. Add error codes for the other ways initialisation might fail.

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.

   
~  

Workaround by tracking which actions failed to initialise because they

~   were unsupported. Interested ActionPool::loadAction callers can then
~   ask whether failed action loads are due to unsupported actions.

  ~

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.

   
~  

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 unaffected.

  ~

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 complete fix might involve detecting whether the action is

  ~

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

Testing Done:

~  

Tested in VM with cirrus/vnc (dpms) and qxl/spice (non-dpms) graphics.

  ~

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.

Diff:

Revision 2 (+40 -6)

Show changes

Kai Uwe Broulik
Just FTR: The DPMS *always* exists in the config file, no matter if it is supported, compiled, or otherwise doesn't work. It is being added by the profile generator on first start along with all the other default settings. This means this message will always appear if the system does not DPMS for some reason.
Dario Freddi
Good start, but there's some stuff to take care of.
powerdevil/daemon/powerdevilactionpool.h (Diff revision 2)
 
 
 
 
 
 
I would add a NoError = 0 at the beginning of the enum, since in case no error occurred we need a value notifying that.
  1. 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?
powerdevil/daemon/powerdevilactionpool.h (Diff revision 2)
 
 
 
For consistency, lastError()
  1. I had this initially but, given the above, think error() is appropriate.
This value should be initialized to 0
  1. I'm hesitant because just like callers shouldn't use it stale, they shouldn't use it uninitialised. What do you think?
It's a bit unclear to me why you need this, since you're advertising only the last error occurred. In fact, you're just populating the hash without using it.
Oliver Henshaw

   
powerdevil/daemon/powerdevilactionpool.cpp (Diff revision 2)
 
 
 
m_initErrors is used here. Or did I misunderstand you?
Oliver Henshaw
At some point I come around to Dario's way of thinking and now think that the ErrorCode should be replaced with a StatusCode (that defaults to NothingToSeeHere or something). It might also be more appropriate to pass this an an optional parameter to ActionPool::loadAction(), rather than adding an ActionPool::status() method.

I'm not entirely sure how far I got with this, I need to check through my old branches.
Kai Uwe Broulik
Ping?
  1. Wednesday is Feature Freeze for 4.11!
  2. Still neglecting this in favour of nominally more important things. It's already on the feature list, so the relevant deadline is the Hard Feature Freeze (5th June). Besides I think that the majority of this is suitable for 4.10.x, so I mean to get it in before 4.10.4 tagging (May 30th).
Loading...