Opened 9 years ago

Closed 9 years ago

Last modified 6 years ago

#2006 closed enhancement (fixed)

add touchpad section to Sugar CP

Reported by: walter Owned by: tomeu
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Unspecified
Severity: Unspecified Keywords: r+
Cc: Distribution/OS: OLPC
Bug Status: Unconfirmed

Description

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.

Attachments (18)

0001-adding-touchpad-section-to-CP.patch (14.3 KB) - added by walter 9 years ago.
0001-icons-for-touchpad-panel.patch (3.9 KB) - added by walter 9 years ago.
consolidated artwork for tp section of cp
0001-touchpad-section-for-control-panel.patch (15.2 KB) - added by walter 9 years ago.
new version that makes changes to ptmode immediately
0001-file-exisits-check.patch (877 bytes) - added by walter 9 years ago.
check to see if file exists before trying to remove it
0001-typo-switch-to-os.remove.patch (1.1 KB) - added by walter 9 years ago.
fixed typo, switched to os.remove
0001-code-cleanup-as-per-Sascha-s-feedback.patch (7.9 KB) - added by walter 9 years ago.
module-touchpad.svg (2.1 KB) - added by walter 9 years ago.
a new icon for the CP
0001-adding-touchpad-icons.patch (4.0 KB) - added by walter 9 years ago.
new device icons for touchpad
0001-touchpad-icons.patch (766 bytes) - added by walter 9 years ago.
update to Makefile.am
0001-touchpad-extension.patch (5.3 KB) - added by walter 9 years ago.
FRAME device extension for touchpad
0001-touchpad-device-on-frame.patch (5.4 KB) - added by walter 9 years ago.
final version of the patch
0001-no-spaces.patch (711 bytes) - added by walter 9 years ago.
Makefile.am with tabs instead of spaces
0001-no-hardwired-paths.patch (2.5 KB) - added by walter 9 years ago.
no hardwired file names
touchpad.py (4.8 KB) - added by walter 9 years ago.
final version of the patch
0001-add-touchpad-device-to-Frame.patch (5.9 KB) - added by walter 9 years ago.
final version of the patch
0001-icons-used-for-touchpad-device.patch (4.5 KB) - added by walter 9 years ago.
final version of the patch
0001-touchpad-with-finger-mode-default.patch (5.0 KB) - added by walter 9 years ago.
defaults to capacitive on boot
0001-add-touchpad-device-to-Frame-for-switching-between-c.patch (5.7 KB) - added by walter 9 years ago.
new version of the patch with Tomeu's suggestions incorporated

Download all attachments as: .zip

Change History (55)

Changed 9 years ago by walter

comment:1 Changed 9 years ago by walter

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

comment:2 Changed 9 years ago by 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...

comment:3 Changed 9 years ago by 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.

Changed 9 years ago by walter

consolidated artwork for tp section of cp

comment:4 Changed 9 years ago by walter

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

comment:6 Changed 9 years ago by bernie

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

comment:7 Changed 9 years ago by 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?

comment:8 Changed 9 years ago by 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

comment:9 Changed 9 years ago by 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                 \

comment:10 follow-up: Changed 9 years ago by 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.

Changed 9 years ago by walter

new version that makes changes to ptmode immediately

comment:12 Changed 9 years ago by 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.

Changed 9 years ago by walter

check to see if file exists before trying to remove it

comment:13 Changed 9 years ago by walter

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

comment:14 Changed 9 years ago by 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

comment:15 in reply to: ↑ 10 Changed 9 years ago by 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.

comment:16 Changed 9 years ago by bernie

The control panel icon has incorrect colors.

comment:17 Changed 9 years ago by 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.

Changed 9 years ago by walter

fixed typo, switched to os.remove

comment:18 Changed 9 years ago by walter

Thanks for catching the typo...

comment:19 Changed 9 years ago by 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.

comment:20 Changed 9 years ago by tomeu

  • Milestone changed from Unspecified by Release Team to 0.90

comment:21 follow-up: Changed 9 years ago by 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.)

comment:22 in reply to: ↑ 21 Changed 9 years ago by 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?

comment:23 Changed 9 years ago by 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.

comment:24 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

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.

comment:25 Changed 9 years ago by 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.

Changed 9 years ago by walter

a new icon for the CP

comment:26 Changed 9 years ago by 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.

Changed 9 years ago by walter

new device icons for touchpad

Changed 9 years ago by walter

update to Makefile.am

Changed 9 years ago by walter

FRAME device extension for touchpad

comment:27 Changed 9 years ago by 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.

Changed 9 years ago by walter

final version of the patch

comment:28 Changed 9 years ago by walter

  • Keywords r? added; control panel r! removed

comment:29 Changed 9 years ago by walter

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

comment:30 Changed 9 years ago by 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).

Changed 9 years ago by walter

Makefile.am with tabs instead of spaces

Changed 9 years ago by walter

no hardwired file names

Changed 9 years ago by walter

final version of the patch

comment:31 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

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

Changed 9 years ago by walter

final version of the patch

Changed 9 years ago by walter

final version of the patch

Changed 9 years ago by walter

defaults to capacitive on boot

comment:32 Changed 9 years ago by 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

comment:33 Changed 9 years ago by 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

Changed 9 years ago by walter

new version of the patch with Tomeu's suggestions incorporated

comment:34 Changed 9 years ago by walter

  • Keywords r? added; r! removed

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.

comment:35 Changed 9 years ago by tomeu

  • Keywords r+ added; r? removed

+ 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!

comment:37 Changed 6 years ago by dnarvaez

  • Milestone 0.90 deleted

Milestone 0.90 deleted

Note: See TracTickets for help on using tickets.