On Tue, May 25, 2010 at 7:05 PM, Walter Bender <walter.ben...@gmail.com> wrote: > On Tue, May 25, 2010 at 8:50 AM, Sascha Silbe > <sascha-ml-ui-sugar-de...@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@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 _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel