[Sugar-devel] [PATCH] touchpad section for Sugar Control Panel

[Sascha Silbe]

Excerpts from Walter Bender's message of Tue May 18 20:08:58 +0000 2010:
> I've attached two patches: (1) a new touchpad section for the control
> panel and (2) new icons for sugar-artwork used by the control panel.
Thanks for working on this!
While at least some users will probably want a faster way (read: a hotkey)
to do this, a Control Panel section is more discoverable and a hotkey
would be better implemented in olpc-kbdshim rather than Sugar.

> We still need a script t be added to olpc-utils that is executed at
> boot time to: (1) change the mode of
> /sys/devices/platform/i8042/serio1/ptmode to 666; and (2) write a 1 to
> that node if a flag file exists ("/home/olpc/.olpc-pentablet-mode").
I'd prefer that code to live in olpc-kbdshim as olpc-utils is specific to
the OLPC builds.

[extensions/cpsection/touchpad/model.py]
> def get_touchpad():
>     """Get the touchpad mode."""
>     if path.exists('/home/olpc/.olpc-pentablet-mode'):
Again, this won't work on non-OLPC builds. For this reason I can't test
your patch, sorry.

> def set_touchpad(touchpad):
>     """Set the touchpad mode."""
>     if touchpad == _CAPACITIVE:
>         system("rm /home/olpc/.olpc-pentablet-mode")
Please use os.remove() instead.

> def print_touchpad():
>     """Print the future touchpad mode."""
>     if get_touchpad == _CAPACITIVE:
>         print _('Touchpad will be set to finger mode on next reboot.')
Typo: get_touchpad(). But I don't see print_touchpad() used anyway, so
you can just drop it. If you'd like to output this message, please use
logging.info() instead.

[extensions/cpsection/touchpad/view.py]
> _CAPACITIVE = 0
> _RESISTIVE = 1
Please define these constants in a single place only (they are defined in
extensions/cpsection/touchpad/model.py, too). Actually I'd prefer the use
of literal strings instead, that way you don't need to define constants.

> class TouchpadEventIcon(gtk.EventBox):
I suppose TouchpadEventIcon is meant to be used by TouchpadPicker / Touchpad
exclusively. In that case it should be made "private", i.e. the name
should be prefixed with an underscore ("_TouchpadPicker").

>     """A subclass of the Sugar Event Icon"""
Is it? The code says otherwise. ;)
In general, a comment saying "A subclass of X" isn't useful and can be
left out. Of course a more meaningful description would be even better.

>     def __init__(self, **kwargs):
>         """Create an extra-large, clickable icon."""    
If rephrased slightly, this is a good candidate for the TouchpadEventIcon
docstring.

> class TouchpadPicker(TouchpadEventIcon):
>     """A class for the touchpad selection buttons"""
Like above, "A class for" is redundant. And what's the difference between
TouchpadEventIcon and TouchpadPicker?

> class Touchpad(SectionView):
[...]
>         self._touchpad_label = gtk.HBox(spacing=style.DEFAULT_SPACING)
>         self._touchpad_box = gtk.HBox(spacing=style.DEFAULT_SPACING)
>         self._touchpad_alert_box = gtk.HBox(spacing=style.DEFAULT_SPACING)
>         self._touchpad_alert = None
You can leave out "touchpad_" from these names as it's clear from the
context. As the names are now shorter, you can also rename
self._touchpad_label to self._label_box as it only contains the box holding
the label, not the label itself (which will be created later in
_setup_touchpad()).

>     def _setup_touchpad(self):                
This would probably better be named _setup_widgets(), as it creates a set
of widgets, whereas from the name I'd have expected it to (physically)
configure the touchpad mode.

>     def undo(self):
>         """Undo any changes."""
>         for widget, handler in self._handlers:
>             widget.disconnect(handler)
Do we need to disconnect the handlers manually? Won't PyGTK handle this
for us during garbage collection?
If PyGTK handles it, you can drop self._handlers (and simplify setup()).
Otherwise we should do it in any case, not just in undo().


I've ignored any "alert"-related code as others have already pointed out
this panel should change the mode at run-time, rather than requiring a
reboot.


PS: Please give "git send-email" a try. It sends the patches in a way that
makes them easier to review.

Sascha
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
Url : http://lists.sugarlabs.org/archive/sugar-devel/attachments/20100525/e680b552/attachment.pgp