<html><body bgcolor="#FFFFFF"><div><span class="Apple-style-span" style="-webkit-tap-highlight-color: rgba(26, 26, 26, 0.296875); -webkit-composition-fill-color: rgba(175, 192, 227, 0.230469); -webkit-composition-frame-color: rgba(77, 128, 180, 0.230469); ">On 8 Jul 2010, at 17:23, Walter Bender <<a href="mailto:walter.bender@gmail.com">walter.bender@gmail.com</a>> wrote:</span><br></div><div><br></div><div></div><blockquote type="cite"><div><span>---</span><br><span> extensions/deviceicon/touchpad.py | 152 +++++++++++++++++++++++++++++++++++++</span><br><span> 1 files changed, 152 insertions(+), 0 deletions(-)</span><br><span> create mode 100644 extensions/deviceicon/touchpad.py</span><br><span></span><br><span>diff --git a/extensions/deviceicon/touchpad.py</span><br><span>b/extensions/deviceicon/touchpad.py</span><br><span>new file mode 100644</span><br><span>index 0000000..a182d9c</span><br><span>--- /dev/null</span><br><span>+++ b/extensions/deviceicon/touchpad.py</span><br><span>@@ -0,0 +1,152 @@</span><br><span>+# Copyright (C) 2010, Walter Bender, Sugar Labs</span><br><span>+#</span><br><span>+# This program is free software; you can redistribute it and/or modify</span><br><span>+# it under the terms of the GNU General Public License as published by</span><br><span>+# the Free Software Foundation; either version 2 of the License, or</span><br><span>+# (at your option) any later version.</span><br><span>+#</span><br><span>+# This program is distributed in the hope that it will be useful,</span><br><span>+# but WITHOUT ANY WARRANTY; without even the implied warranty of</span><br><span>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the</span><br><span>+# GNU General Public License for more details.</span><br><span>+#</span><br><span>+# You should have received a copy of the GNU General Public License</span><br><span>+# along with this program; if not, write to the Free Software</span><br><span>+# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA</span><br><span>+</span><br><span>+</span><br><span>+from gettext import gettext as _</span><br><span>+</span><br><span>+import gtk</span><br><span>+import gconf</span><br><span>+from os import system, path, remove</span><font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#0023A3"><br></font></font></div></blockquote><div><br></div><div>The os import should be in the first block. I wouldn't import the methods, prepending os. to the actual calls makes it clearer what the methods are doing.</div><div><br></div><br><blockquote type="cite"><div><span>+from sugar.graphics.tray import TrayIcon</span><br><span>+from sugar.graphics.xocolor import XoColor</span><br><span>+from sugar.graphics.palette import Palette</span><br><span>+from jarabe.frame.frameinvoker import FrameWidgetInvoker</span><br><span>+</span><br></div></blockquote><div><br></div><div>The jarabe import should be in a separate block.</div><div><br></div><br><blockquote type="cite"><div><span>+_STATUS_TEXT = {'capacitive': _('finger'), 'resistive': _('stylus')}</span><br><span>+_CAPACITIVE_ICON_NAME = 'touchpad-capacitive'</span><br><span>+_RESISTIVE_ICON_NAME = 'touchpad-resistive'</span><br><span>+_FLAG_PATH = '/home/olpc/.olpc-pentablet-mode'</span><br><span>+_NODE_PATH = '/sys/devices/platform/i8042/serio1/ptmode'</span><br></div></blockquote><div><br></div><div>I think we don't need to mark these has private, the plugins code is not imported by other modules, so private/public doesn't matter.</div><br><blockquote type="cite"><div><span>+</span><br><span>+</span><br></div></blockquote><div><br></div><div>One less new line :) I'm sure everyone has been missing my nitpicks!</div><br><blockquote type="cite"><div><span>+class DeviceView(TrayIcon):</span><br><span>+ """ Manage the touchpad mode from the device palette on the Frame """</span><br><span>+</span><br><span>+ FRAME_POSITION_RELATIVE = 500</span><br><span>+</span><br><span>+ def __init__(self):</span><br><span>+ """ Create the touchpad palette and display it on Frame """</span><br><span>+</span><br><span>+ # Only appears when the device exisits</span><br><span>+ if not path.exists(_NODE_PATH):</span><br><span>+ return</span><br><span>+</span><br></div></blockquote><div><br></div><div>Aborting constructions looks wrong, better to move the check to the setup method.</div><div><br></div><br><blockquote type="cite"><div><span>+ if get_touchpad() == 'resistive':</span><br><span>+ icon_name = _RESISTIVE_ICON_NAME</span><br><span>+ else:</span><br><span>+ icon_name = _CAPACITIVE_ICON_NAME</span><br></div></blockquote><div><br></div><div>It would be nice to use a dictionary like you did with the label.</div><br><blockquote type="cite"><div><span>+</span><br><span>+ client = gconf.client_get_default()</span><br><span>+ color = XoColor(client.get_string('/desktop/sugar/user/color'))</span><br><span>+ TrayIcon.__init__(self, icon_name=icon_name, xo_color=</span><br></div></blockquote><div><br></div><div>Splitting the blocks with a new line here would be more readable.</div><br><blockquote type="cite"><div><span>+ self.set_palette_invoker(FrameWidgetInvoker(self))</span><br><span>+ self.connect('button-release-event', self.__button_release_event_cb)</span><br><span>+ self.connect('expose-event', self.__expose_event_cb)</span><br><span>+</span><br><span>+ def create_palette(self):</span><br><span>+ """ On create, set the current mode """</span><br><span>+ self.palette = ResourcePalette(_('My touchpad'), self.icon)</span><br><span>+ self.palette.set_group_id('frame')</span><br><span>+ return self.palette</span><br><span>+</span><br><span>+ def __button_release_event_cb(self, widget, event):</span><br><span>+ """ On button release, switch modes """</span><br><span>+ self.palette.toggle_mode()</span><br><span>+ return True</span><br><span>+</span><br><span>+ def __expose_event_cb(self, *args):</span><br><span>+ pass</span><br><br></div></blockquote><div><br></div><div>Leftover? Seems unnecessary to connect the signal since you are just passing then.</div><div><br></div><br><blockquote type="cite"><div><span>+</span><br><span>+class ResourcePalette(Palette):</span><br><span>+ """ Query the current state of the touchpad and update the display """</span><br><span>+</span><br><span>+ def __init__(self, primary_text, icon):</span><br><span>+ """ Get the status """</span><br><span>+ Palette.__init__(self, label=primary_text)</span><br><span>+</span><br><span>+ self._icon = icon</span><br><span>+</span><br><span>+ self.connect('popup', self.__popup_cb)</span><br><span>+ self.connect('popdown', self.__popdown_cb)</span><br><span>+</span><br><span>+ self.updating = False</span><br><span>+</span><br><span>+ vbox = gtk.VBox()</span><br><span>+ self.set_content(vbox)</span><br><span>+</span><br><span>+ self._status_text = gtk.Label()</span><br><span>+ vbox.pack_start(self._status_text, padding=10)</span><br><span>+ self._status_text.show()</span><br><span>+</span><br><span>+ vbox.show()</span><br><span>+</span><br><span>+ self._mode = get_touchpad()</span><br><span>+ self.set_mode_graphics()</span><br><span>+</span><br><span>+ def set_mode_graphics(self):</span></div></blockquote><div><br></div><div>Looks like this can be marked as private? update would be more appropriate then set since you are not passing a mode.</div><br><blockquote type="cite"><div><span>+ """ Set the label and icon based on the current mode """</span><br><span>+ self._status_text.set_label(_STATUS_TEXT[self._mode])</span><br><span>+ if self._mode == 'resistive':</span><br><span>+ self._icon.props.icon_name = _RESISTIVE_ICON_NAME</span><br><span>+ else:</span><br><span>+ self._icon.props.icon_name = _CAPACITIVE_ICON_NAME</span><br><span>+</span><br><span>+ def toggle_mode(self):</span><br><span>+ """ On mouse click, toggle the mode """</span><br><span>+ if self._mode == 'capacitive':</span><br><span>+ self._mode = 'resistive'</span><br><span>+ else:</span><br><span>+ self._mode = 'capacitive'</span><br><span>+</span><br><span>+ set_touchpad(self._mode)</span><br></div></blockquote><div><br></div><div>I would write the new mode before assigning it to the object property so that if something goes wrong and we trace back, state doesn't go out of sync.</div><br><blockquote type="cite"><div><span>+ self.set_mode_graphics()</span><br><span>+</span><br><span>+ def __popup_cb(self, gobject):</span><br><span>+ self.updating = True</span><br><span>+</span><br><span>+ def __popdown_cb(self, gobject):</span><br><span>+ self.updating = False</span><br><br></div></blockquote><div><br></div><div>What does the updating property do? Is it a Palette thing?</div><br><blockquote type="cite"><div><span>+</span><br><span>+def setup(tray):</span><br><span>+ tray.add_device(DeviceView())</span><br><span>+</span><br><span>+</span><br><span>+def get_touchpad():</span><br></div></blockquote><div><br></div><div>I think naming it read_touchpad_mode would be more clear</div><br><blockquote type="cite"><div><span>+ """ Get the touchpad mode. """</span><br><span>+ _file_handle = open(_NODE_PATH, "r")</span><br><span>+ _text = _file_handle.read()</span><br><span>+ _file_handle.close()</span><font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#0023A3"><br></font></font></div></blockquote><div><br></div><div><br></div>It seems unnecessary to mark these variables as private?<br><div><br></div><br><blockquote type="cite"><div><span>+ if _text[0] == '1':</span><br><span>+ return 'resistive'</span><br><span>+ else:</span><br><span>+ return 'capacitive'</span><font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#0023A3"><br></font></font></div></blockquote><div><br></div><div>It would be nicer to use an enumeration for these instead of raw strings.</div><br><blockquote type="cite"><div><span>+</span><br><span>+def set_touchpad(touchpad):</span><br></div></blockquote><div><br></div><div>Suggest write_touchpad_mode</div><br><blockquote type="cite"><div><span>+ """ Set the touchpad mode. """</span><br><span>+ if touchpad == 'capacitive':</span><br><span>+ if path.exists(_FLAG_PATH):</span><br><span>+ remove(_FLAG_PATH)</span><br></div></blockquote><div><br></div><div>Why do we need this flag?</div><br><blockquote type="cite"><div><span>+ system("echo 0 > %s" % (_NODE_PATH))</span><br></div></blockquote><div><br></div><div>It would be better to use python to write instead of forking echo.</div><div><br></div><br><blockquote type="cite"><div><span>+ else:</span><br><span>+ _file_handle = open(_FLAG_PATH, "w")</span><br><span>+ _file_handle.close()</span><br></div></blockquote><div><br></div><div>Marking variable as private seems unnecessary.</div><br><blockquote type="cite"><div><span>+ system("echo 1 > %s" % (_NODE_PATH))</span><br></div></blockquote><div><br></div><div>Same as above.</div><br><blockquote type="cite"><div><span>+ return</span><br></div></blockquote><div><br></div><div>I guess this is a leftover, we are exiting the function anyway.</div><br><div><br></div></body></html>