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

[Walter Bender]

On Mon, Jul 26, 2010 at 11:50 AM, David Farning <dfarning at gmail.com> wrote:
> 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
>

Not sure what else I can do. I've incorporated all of the suggestion
from Marco et al. Just awaiting final review.

-walter


-- 
Walter Bender
Sugar Labs
http://www.sugarlabs.org