add touchpad section to Sugar CP

[walter]

OLPC has enabled the ability to switch between the resistive and capacitive touchpads. This patch adds a new section to the control panel to expose this to the user. It only appears on OLPC XO 1.0 CL1 hardware.

[walter]

Please refer to  http://lists.sugarlabs.org/archive/sugar-devel/2010-May/023974.html for a more detailed discussion.

[bernie]

The following hunk should be added to 0001-adding-touchpad-section-to-CP.patch:

diff --git a/data/icons/Makefile.am b/data/icons/Makefile.am
index a35643a..b65b34e 100644
--- a/data/icons/Makefile.am
+++ b/data/icons/Makefile.am
@@ -10,6 +10,7 @@ sugar_DATA =                        \
        module-modemconfiguration.svg   \
        module-network.svg              \
        module-power.svg                \
+       module-touchpad.svg             \
        module-updater.svg

 EXTRA_DIST = $(sugar_DATA)

Patch applied to F11-XO1-0.88 for testing...

[walter]

Oops. I had forgotten to move that file from my experimental branch to my fresh clone. I still need to improve my workflow. I have a couple of small code cleanups coming as well.

[walter]

consolidated artwork for tp section of cp

[walter]

New patches for consolidated artwork that uses XO colors;
Slight code clean up and xocolor glue.

[walter]

Here is what it looks like:

 http://wiki.sugarlabs.org/go/File:Touchpad-control-panel-selection.png

[bernie]

A nice addition would be showing the control panel icon only if the XO-1 touchpad is present.

[walter]

That should already be the case as per the if statement in the init.py file:

if path.exists('/sys/devices/platform/i8042/serio1/ptmode'):

CLASS = 'Touchpad'
ICON = 'module-touchpad'
TITLE = _('Touchpad')
KEYWORDS = touchpad?

[bernie]

The following hunk was also missing from the Sugar part of the patch:

a/configure.ac.orig b/configure.ac
--- sugar-0.88.0/configure.ac.orig      2010-05-19 21:16:39.000000000 -0400
+++ sugar-0.88.0/configure.ac   2010-05-19 21:17:03.000000000 -0400
@@ -59,6 +59,7 @@ extensions/cpsection/modemconfiguration/
 extensions/cpsection/Makefile
 extensions/cpsection/network/Makefile
 extensions/cpsection/power/Makefile
+extensions/cpsection/touchpad/Makefile
 extensions/cpsection/updater/backends/Makefile
 extensions/cpsection/updater/Makefile
 extensions/deviceicon/Makefile

[bernie]

Other missing hunk (for the sugar-artwork part):

--- sugar-artwork-0.88.0/icons/scalable/device/Makefile.am.orig 2010-05-20 03:35:54.000000000 -0400
+++ sugar-artwork-0.88.0/icons/scalable/device/Makefile.am      2010-05-20 03:36:22.000000000 -0400
@@ -38,6 +38,7 @@ icon_DATA =                           \
        battery-charging-100.svg        \
        camera-external.svg             \
        camera.svg                      \
+       capacitive.svg                  \
        computer.svg                    \
        computer-xo.svg                 \
        drive.svg                       \
@@ -77,6 +78,7 @@ icon_DATA =                           \
        network-wireless-connected-100.svg      \
        network-wireless-connected-100.icon     \
        printer.svg                     \
+       resistive.svg                   \
        speaker-000.svg                 \
        speaker-033.svg                 \
        speaker-066.svg                 \

[walter]

It would be good to have a page in the wiki somewhere to describe all of the extra places you need to look to get these configuration files in order. Makefile.am I usually remember, but configure.ac I had never known about.

[walter]

 http://wiki.sugarlabs.org/go/Activity_Team/Control_Panel

and

 http://wiki.sugarlabs.org/go/Features/Touchpad_control_panel_section

[walter]

new version that makes changes to ptmode immediately

[walter]

pgf wrote:

changes to the ptmode should be able to take affect immediately --
no restart should be required. and i think, given the nature
of this feature, that this is important.

my initial design outline took this into account, but maybe i wasn't
clear. so:

• the "get_touchpad()" method should ignore the flag file

entirely. it should read the ptmode node, and return
RESISTIVE if the value was a 1. the flag file should be
ignored in get_touchpad().

• the "set_touchpad()" method should write 1/0 to the ptmode

node, and also create/destroy the flag file, as
appropriate.

• the control panel should lose the "Changes require restart"

text.

btw, in addition to allowing immediate switching, this scheme
allows seamless operation with a gnome applet which does the same
thing, if we were ever to create such an applet.

---

I also implemented an "undo" that restores the previous state if the user exits the panel by hitting the X.

Tested on an OLPC XO-1 CL1 where I sudo chmod +w ptmode.

[walter]

check to see if file exists before trying to remove it

[walter]

Added a patch to check if the flag file exists before trying to remove it. Also added testing instructions on the wiki page.

[tabitha]

I tried to follow the "How To Test" instructions on  http://wiki.sugarlabs.org/go/Features/Touchpad_control_panel_section

There was no ptmode file in /sys:

[olpc@xo-10-A4-F0 5F61-9B8A]$ sudo find /sys -name ptmode
[olpc@xo-10-A4-F0 5F61-9B8A]$

There was no devices directory in /usr/share/icons/sugar/scaleable, there was a "device" directory

There was no extensions directory in /usr/share/sugar

XO CL1
Martin's build os802b6
Sugar 0.82.1

[bernie]

Replying to walter:

It would be good to have a page in the wiki somewhere to describe all of the extra places you need to look to get these configuration files in order. Makefile.am I usually remember, but configure.ac I had never known about.

Automake does not support wildcards for philosophical reasons.

We could work around the problem by creating ad-hoc install rules like we do for the po/ directory) or switch to a setup.py based build system like Michael Stone did in his Sugar fork.

[bernie]

The control panel icon has incorrect colors.

[bernie]

There's a typo in 0001-file-exisits-check.patch: s/exisits/exists/

Also, it would be better to do os.remove(path) than calling system("rm ...") to do it.

[walter]

fixed typo, switched to os.remove

[walter]

Thanks for catching the typo...

[walter]

I made a number of small changes based upon Sascha's feedback (see attachment). I didn't change the location of the flag file as this change has to be coordinated with PGF's patch. Regarding a keyboard shortcut, while there is nothing wrong with including one, the purpose of this patch is to provide a work-around for the flaky touchpads on the XO-1.0 CL1 hardware. It is not clear that children will be switching back and forth but on rare occasions. Note that this feature is designed to only appear on that hardware, since it is only relevant there.

[walter]

In some sense, this feature is more important to backport to 0.84 than to include in 0.90, since 0.84 is what will be used in the field on older machines. (Alas, it won't work with 0.82, which may be deployed in Peru.)

[tomeu]

Replying to walter:

In some sense, this feature is more important to backport to 0.84 than to include in 0.90, since 0.84 is what will be used in the field on older machines. (Alas, it won't work with 0.82, which may be deployed in Peru.)

But you cannot backport it if it's not in a later release, right? Otherwise, it's adding feature to a past stable release. Is that what is requested?

[walter]

Good point!! I see no reason not to include it in future releases; presumably these will eventually find their way to the old hardware.

[tomeu]

Btw, what's the reason for submitting it for inclusion upstream if it's only relevant for a single hw platform? The extension mechanism exists precisely to make easier for downstreams to extend Sugar without having to go through all the hops to get the code upstream (and also to avoid having upstream maintain some code that is only relevant to a subset of its users).

I know XO 1.0 is our largest user base, but I don't see the point of having it upstream, given that we have the extensions mechanism.

[walter]

This is a good point. It is also not clear that the CP is the proper place for this. Bernie has suggested the device section of the frame. Investigating.

[walter]

a new icon for the CP

[walter]

Just attached a new icon for the CP on the off chance we end up going that route. (It is strictly black and white like the others). I will continue to pursue the device-on-the-frame route in any case.

[walter]

new device icons for touchpad

[walter]

update to Makefile.am

[walter]

FRAME device extension for touchpad

[walter]

Attached are patches for adding the touchpad selector to the Frame. This extension is only applicable to the XO 1.0 CL1 hardware. It supercedes the control panel patches previously included with this ticket.

[walter]

final version of the patch

[walter]

See  http://wiki.sugarlabs.org/go/Features/Touchpad_control_panel_section for more details, including illustrations of the feature.

[bernie]

In 0001-touchpad-device-on-frame.patch, the Makefile bits are indented with spaces, not tabs. Make traditionally demands tabs (but gnu make is smarter).

[walter]

Makefile.am with tabs instead of spaces

[walter]

no hardwired file names

[walter]

final version of the patch

[tomeu]

If Marco has already approved this patch, just push it. For the future, please attach only whole patches.

[walter]

final version of the patch

[walter]

final version of the patch

[walter]

defaults to capacitive on boot

[walter]

This version will default to capacitive mode on boot. Because we are no longer preserving mode between boots, there is no longer any need to maintain the FLAG_FILE nor to check for it at boot.

 http://bugs.sugarlabs.org/attachment/ticket/2006/0001-touchpad-with-finger-mode-default.patch

Tested on 258py

[tomeu]

+TOUCHPAD_MODES = ['capacitive', 'resistive']

I would go with TOUCHPAD_MODE_CAPACITIVE and TOUCHPAD_MODE_RESISTIVE instead, then people who read the code don't need to translate from indexes to values.

+        """ Create the touchpad palette and display it on Frame. """

Looks like the palette is not actually created here? What about "Create the icon that represents the touchpad"?

+        """ On create, set the current mode. """

Don't see anything about the mode in the implementation of this method. What about "Create a palette for this icon, gets called by the Sugar framework when a palette needs to be displayed"?

+        """ On button release, switch modes. """

Docstrings should explain the functionality, not how it is achieved. Otherwise we have to make changes twice: once in the code and once in the docs. In fact, I guess that's why the previous comments don't match what the code does. Not having docstrings is bad, but having misleading docstrings is even worst. I recommend to look at the docstrings in the Python std library, such as  http://docs.python.org/library/logging.html.

If you really need to add some explanation of what the code does because it's too complex to understand with a quick read, consider making the code more obvious (split long lines, add intermediate variables, split out more functions, etc.). In cases when that is not possible (for example performance critical snippets, obscure APIs are used, ...) use inline comments instead of docstrings for the implementation explanations.

+    """ Query the current state of the touchpad and update the display. """

That's not what I would expect a class called ResourcePalette do. What about "Palette attached to the device icon that represents the touchpad"?

+        """ On mouse click, toggle the mode. """

This method has nothing about mouse clicks. I would leave only "Toggle the touchpad mode".

+    """ Touchpad palette only appears when the device exisits. """

This doesn't describe the functionality of setup(). "Initialize the device icon, called by the shell when initializing the frame".

+def read_touchpad_mode():

Should be private.

+def write_touchpad_mode(touchpad):

Why have a function that just calls another one?

+    """ Write to node path, catching exception is there is a problem """

Implementation detail. """ Write the touchpad mode to the node path. """ sounds better.

+        print e

Better use logging.exception

[walter]

new version of the patch with Tomeu's suggestions incorporated

[walter]

 http://bugs.sugarlabs.org/attachment/ticket/2006/0001-add-touchpad-device-to-Frame-for-switching-between-c.patch

I believe I have taken care of all of the changes recommended by Tomeu. Please review again.

[tomeu]

+ TOUCHPAD_MODES[1]: _('stylus')}

I guess s/TOUCHPAD_MODES[1]/TOUCHPAD_MODE_RESISTIVE ?

+ """ Toggle the touchpad mode; called by touchpad icon
+ button-release-event handler. """

I think we shouldn't be so specific about who calls what because otherwise we are going to end up with unsync'ed docstrings, specially for public methods.

+ # Set the initial touchpad device value to capacitive mode (finger)
+ _write_touchpad_mode(TOUCHPAD_MODE_CAPACITIVE)

Isn't this comment redundant? Once you get the intent of your code to be clear, you don't need inline comments any more ;)

Please push with this changes, thanks!

[walter]

Committed with these patches:

 http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/950f451aa2b8cc52b52448c2653b219c8b4d91ae

 http://git.sugarlabs.org/projects/sugar-artwork/repos/walters-sugarartwork-clone/commits/cc5a6fbfe9a2eff57d34a8c55f8b47ebc3f3b3a9