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

David Farning dfarning at gmail.com
Mon Jul 26 11:50:22 EDT 2010


On Tue, May 25, 2010 at 7:05 PM, Walter Bender <walter.bender at gmail.com> wrote:
> On Tue, May 25, 2010 at 8:50 AM, Sascha Silbe
> <sascha-ml-ui-sugar-devel at silbe.org> wrote:
>> 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/
>>
>> _______________________________________________
>> Sugar-devel mailing list
>> Sugar-devel at lists.sugarlabs.org
>> http://lists.sugarlabs.org/listinfo/sugar-devel
>>
>>
>
> Thanks. Great feedback... I'll learn Python one of these days :)
>
> -walter

Walter,

I you get a chance can you take another look at the patch.  PY is
carrying it in their local tree but it has not been accepted in sugar.

david


More information about the Sugar-devel mailing list