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/
signature.asc
Description: PGP signature
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel