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 Bender Sugar Labs http://www.sugarlabs.org _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel