[Bugs] #2006 UNSP: add touchpad section to Sugar CP

[Sugar Labs Bugs]

#2006: add touchpad section to Sugar CP
------------------------------------------+---------------------------------
    Reporter:  walter                     |          Owner:  tomeu      
        Type:  enhancement                |         Status:  new        
    Priority:  Unspecified by Maintainer  |      Milestone:  0.90       
   Component:  sugar                      |        Version:  Unspecified
    Severity:  Unspecified                |       Keywords:  r!         
Distribution:  OLPC                       |   Status_field:  Unconfirmed
------------------------------------------+---------------------------------

Comment(by tomeu):

 {{{
 +TOUCHPAD_MODES = ['capacitive', 'resistive']
 }}}

 I would go with TOUCHPAD_MODE_CAPACITIVE and TOUCHPAD_MODE_RESISTIVE
 instead, then people who read the code don't need to translate from
 indexes to values.

 {{{
 +        """ Create the touchpad palette and display it on Frame. """
 }}}

 Looks like the palette is not actually created here? What about "Create
 the icon that represents the touchpad"?

 {{{
 +        """ On create, set the current mode. """
 }}}

 Don't see anything about the mode in the implementation of this method.
 What about "Create a palette for this icon, gets called by the Sugar
 framework when a palette needs to be displayed"?

 {{{
 +        """ On button release, switch modes. """
 }}}

 Docstrings should explain the functionality, not how it is achieved.
 Otherwise we have to make changes twice: once in the code and once in the
 docs. In fact, I guess that's why the previous comments don't match what
 the code does. Not having docstrings is bad, but having misleading
 docstrings is even worst. I recommend to look at the docstrings in the
 Python std library, such as http://docs.python.org/library/logging.html.

 If you really need to add some explanation of what the code does because
 it's too complex to understand with a quick read, consider making the code
 more obvious (split long lines, add intermediate variables, split out more
 functions, etc.). In cases when that is not possible (for example
 performance critical snippets, obscure APIs are used, ...) use inline
 comments instead of docstrings for the implementation explanations.

 {{{
 +    """ Query the current state of the touchpad and update the display.
 """
 }}}

 That's not what I would expect a class called ResourcePalette do. What
 about "Palette attached to the device icon that represents the touchpad"?

 {{{
 +        """ On mouse click, toggle the mode. """
 }}}

 This method has nothing about mouse clicks. I would leave only "Toggle the
 touchpad mode".

 {{{
 +    """ Touchpad palette only appears when the device exisits. """
 }}}

 This doesn't describe the functionality of setup(). "Initialize the device
 icon, called by the shell when initializing the frame".

 {{{
 +def read_touchpad_mode():
 }}}

 Should be private.

 {{{
 +def write_touchpad_mode(touchpad):
 }}}

 Why have a function that just calls another one?

 {{{
 +    """ Write to node path, catching exception is there is a problem """
 }}}

 Implementation detail. """ Write the touchpad mode to the node path. """
 sounds better.

 {{{
 +        print e
 }}}

 Better use logging.exception

-- 
Ticket URL: <http://bugs.sugarlabs.org/ticket/2006#comment:33>
Sugar Labs <http://sugarlabs.org/>
Sugar Labs bug tracking system