On Mon, Jul 26, 2010 at 11:50 AM, David Farning <dfarn...@gmail.com> wrote: > 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 >
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 _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel