After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 598820 - touchpad support overrides system defaults
touchpad support overrides system defaults
Status: RESOLVED DUPLICATE of bug 635486
Product: gnome-settings-daemon
Classification: Core
Component: mouse
2.28.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
: 589944 602094 617911 (view as bug list)
Depends on: 638973
Blocks:
 
 
Reported: 2009-10-18 06:47 UTC by Wendall Cada
Modified: 2011-03-21 14:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adds tap-to-click button settings. (5.07 KB, patch)
2010-01-06 16:26 UTC, Yuri Khan
none Details | Review
Adds tap-to-click button settings, with gconf schema update. (6.90 KB, patch)
2010-01-07 09:28 UTC, Yuri Khan
reviewed Details | Review
Adds tap-to-click button settings, v3. (6.92 KB, patch)
2010-01-13 17:40 UTC, Yuri Khan
needs-work Details | Review
Adds tap-to-click button settings, v4. (11.34 KB, patch)
2010-01-20 18:40 UTC, Yuri Khan
none Details | Review
Adds tap-to-click button settings, v5. (12.72 KB, patch)
2010-02-04 05:07 UTC, Yuri Khan
needs-work Details | Review
Adds tap-to-click button settings, v6. (13.69 KB, patch)
2010-02-17 18:02 UTC, Yuri Khan
none Details | Review
Adds tap-to-click button settings, v6a. (13.68 KB, patch)
2010-02-17 20:17 UTC, Yuri Khan
reviewed Details | Review
Adds tap-to-click button settings, v7. (13.63 KB, patch)
2010-02-20 14:59 UTC, Yuri Khan
needs-work Details | Review
Adds tap-to-click button settings, v8. (13.68 KB, patch)
2010-02-28 18:56 UTC, Yuri Khan
none Details | Review
Adds tap-to-click button settings, v8b (12.92 KB, patch)
2010-04-01 13:58 UTC, Tommaso Pasini
none Details | Review
Adds tap-to-click button settings, v9. (13.57 KB, patch)
2010-04-03 09:29 UTC, Yuri Khan
none Details | Review
Adds corner-click button settings, with the same logic. (9.02 KB, patch)
2010-04-03 09:33 UTC, Yuri Khan
none Details | Review
Adds tap-to-click button settings, rebased 2010-11-21. (14.00 KB, patch)
2010-11-21 14:15 UTC, Yuri Khan
none Details | Review
Adds corner-click button settings with the same logic, rebased 2010-11-21. (9.11 KB, patch)
2010-11-21 14:17 UTC, Yuri Khan
none Details | Review
Implement UI in gnome-control-center for the above two patches. (22.25 KB, patch)
2010-11-21 14:32 UTC, Yuri Khan
none Details | Review
[PATCH 1/4, rebased 2011-01-05] mouse: Extract get_device_button_mapping function. (3.22 KB, patch)
2011-01-05 07:47 UTC, Yuri Khan
none Details | Review
[PATCH 2/4] mouse: Fully take button mapping into account when setting tap-to-click. (7.50 KB, patch)
2011-01-05 07:48 UTC, Yuri Khan
none Details | Review
[PATCH 3/4, rebased 2011-01-05] mouse: Add tap-to-click button settings (9.78 KB, patch)
2011-01-05 07:49 UTC, Yuri Khan
none Details | Review
[PATCH 4/4, rebased 2011-01-05] mouse: Add corner-click button settings. (8.22 KB, patch)
2011-01-05 07:50 UTC, Yuri Khan
none Details | Review
gnome-control-center/mouse: Add tap-to-click button settings. (22.29 KB, patch)
2011-01-05 07:56 UTC, Yuri Khan
none Details | Review
[1/2] mouse: Add tap-to-click button settings (9.78 KB, patch)
2011-01-08 09:04 UTC, Yuri Khan
reviewed Details | Review
[2/2] mouse: Add corner-click button settings. (8.22 KB, patch)
2011-01-08 09:05 UTC, Yuri Khan
reviewed Details | Review
gnome-control-center/mouse: Add tap-to-click button settings. (22.29 KB, patch)
2011-01-08 09:09 UTC, Yuri Khan
reviewed Details | Review
gnome-control-center/mouse: Add tap-to-click button settings. (23.22 KB, patch)
2011-02-07 13:20 UTC, Yuri Khan
reviewed Details | Review

Description Wendall Cada 2009-10-18 06:47:04 UTC
Bug #578444 added support for touchpad settings. The implementation incorrectly assumes that 1/3/2 (Left/Right/Middle) button mapping is the default for touchpads. According to the synaptics man page, the default is 1/2/3 (Left/Middle/Right). For many distributions, 1/2/3 is the default setting in accordance with the synaptics manpage. 

This setting should not be hardcoded if it is not available as a gconf setting, and use whatever the xorg or hal settings are for this. Currently, this overrides the settings in hal and xorg, removing functionality from the underlying system, and removes functionality from the end user.
Comment 1 Tommaso Pasini 2009-10-19 12:11:22 UTC
The same happens on my system. I filed a long description on gentoo bugzilla (http://bugs.gentoo.org/show_bug.cgi?id=287230).
Comment 2 Gilles Dartiguelongue 2009-10-19 12:33:01 UTC
Let me add as I said on the gentoo bug that if g-s-d doesn't care about hal because everyone is trying to migrate to udev, please at least avoid half functional implementation of a feature that erases any sound default for not so uncommon touchpads. In my case, I lost ability to double or triple click (double/triple finger taps, but I don't want single finger tap as it is driving me crazy) on my macbook and somehow, double-finger scrolling doesn't want to re-enable through the interface (ok I had a strange "couldn't contact g-s-d" message even if the daemon was running so it may be a separate issue).

Bug #591696 seems to be mostly about the same I believe, might want to close one as duplicate of the other.
Comment 3 Peter Hutterer 2009-10-19 22:19:53 UTC
Reason for the 1/3/2 mapping is outlined in #591696.

A configuration interface to change the button mapping would generally be handy though.
Comment 4 Wendall Cada 2009-10-19 22:36:25 UTC
Based on several of the bugs I read before filing this report, I'd think that a configuration change would not be nice, but rather necessary. This patch breaks several things, including user trust by taking away functionality from the end user that previously existed. 

I think there was a huge underestimation in #591696 of the number of end users currently using multitouch, and a rather arrogant assumption of how people use multitouch.

This is a feature that without this code works fine, and steps should be taken to fix it. This is not a request for a feature that would be nice to have. I am unable to use my multitouch touchpad how I need to because this breaks it. I am also unable to just revert to my global settings, since this overrides the global settings.
Comment 5 Tommaso Pasini 2009-10-20 15:07:20 UTC
I understand the point in #591696; but I configured my touchpad differently and the default settings are worse than mine just because I chose what I think is best for me.

I'm not saying that my settings are the best for everyone, just that they shouldn't be overridden. Moreover, this is a regression: suppose I used circular scrolling, now it's disabled unless I open a terminal and use synclient.

I suggest that there should be some kind of check at the start of the session so that if there is no configuration (e.g. in new installations) the new defaults are applied; otherwise nothing overrides what is set by hal. Or, there could be a checkbox at the top of the capplet to let the user decide which method he prefers to configure its touchpad.
Comment 6 Jens Granseuer 2009-11-16 18:29:49 UTC
*** Bug 602094 has been marked as a duplicate of this bug. ***
Comment 7 Tom 2009-12-23 23:45:16 UTC
Yes, this is a problem.

I just spent way too long trying to figure out why my synaptics settings weren't taking effect in /etc/hal/fdi/policy/.  'Twas a wonderful use of my afternoon.
Comment 8 Peter Hutterer 2009-12-24 00:33:39 UTC
We have no way in GNOME to know whether a driver setting is a default setting or a user configuration. Not unless we know the driver defaults in GNOME as well, which isn't really feasible.
Comment 9 Yuri Khan 2010-01-06 14:25:20 UTC
Then there should be a way to configure g-s-d so that it does not force an arbitrarily selected default on the user. A gconf setting at least; GUI may come later.

My rationale: All touchpads I have seen (not many in fact) had hardware left and right buttons, but no middle button. When I need to right-click, I can use the hardware button, but for middle-clicking I want an easier gesture than the three-finger tap or a two-button click.
Comment 10 Yuri Khan 2010-01-06 16:26:36 UTC
Created attachment 150900 [details] [review]
Adds tap-to-click button settings.

In fact, here’s a patch. It adds gconf keys /desktop/gnome/peripherals/touchpad/tap_button_1, tap_button_2 and tap_button_3, of integer type. The meaning of values is the same as for TapButton1..3 in synaptics(4).
Comment 11 Tommaso Pasini 2010-01-06 16:56:00 UTC
The patch doesn't work here:
- the three gconf options don't appear
- tap-to-click (even a single finger) no longer works, no matter how I set tap_to_click in gconf-editor.
Comment 12 Yuri Khan 2010-01-07 09:28:27 UTC
Created attachment 150965 [details] [review]
Adds tap-to-click button settings, with gconf schema update.

(In reply to comment #11)
> The patch doesn't work here:
> - the three gconf options don't appear

That’s because I did not include the schema update. You could still add them manually.

> - tap-to-click (even a single finger) no longer works, no matter how I set
> tap_to_click in gconf-editor.

And this is because without the schema, the new values default to 0, which is interpreted as “disable”.

I attach the updated patch, now with schema update. I had to re-logon after ”make install” before gconf-editor showed me the new settings.
Comment 13 Tommaso Pasini 2010-01-07 09:53:48 UTC
It works, great job!
Comment 14 Peter Hutterer 2010-01-12 04:45:44 UTC
Review of attachment 150965 [details] [review]:

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Comment 15 Jens Granseuer 2010-01-12 17:54:18 UTC
Review of attachment 150965 [details] [review]:

::: plugins/mouse/gsd-mouse-manager.c
@@ +545,2 @@
 static int
+set_tap_to_click (gboolean state, guint tapButton1, guint tapButton2, guint tapButton3)

Small nit-pick: we don't use camelCase.
Comment 16 Yuri Khan 2010-01-13 17:40:53 UTC
Created attachment 151350 [details] [review]
Adds tap-to-click button settings, v3.

(In reply to comment #15)
> Small nit-pick: we don't use camelCase.

Fixed.
Comment 17 bigo72 2010-01-14 11:23:23 UTC
Ehi people! I made a deb to fix it!
Comment 19 bigo72 2010-01-16 08:59:18 UTC
No, please, don't say "thanks", not necessary, ok? .... Strange people ...
Comment 20 Jens Granseuer 2010-01-16 10:31:02 UTC
Comment on attachment 151350 [details] [review]
Adds tap-to-click button settings, v3.

Patch doesn't apply to master at the moment, unfortunately.
Comment 21 Jens Granseuer 2010-01-16 10:40:14 UTC
*** Bug 589944 has been marked as a duplicate of this bug. ***
Comment 22 Hiroyuki Ikezoe 2010-01-18 00:21:10 UTC
Review of attachment 151350 [details] [review]:

Though I am not a reviewer,

::: plugins/mouse/gsd-mouse-manager.c
@@ +825,3 @@
+                          gconf_client_get_int  (client, KEY_TAP_BUTTON_1, NULL),
+                          gconf_client_get_int  (client, KEY_TAP_BUTTON_2, NULL),
+                          gconf_client_get_int  (client, KEY_TAP_BUTTON_3, NULL));

I'd prefer that these values are got in set_tap_click() rather than scattering around.
Comment 23 Yuri Khan 2010-01-19 06:56:49 UTC
(In reply to comment #20)
> (From update of attachment 151350 [details] [review])
> Patch doesn't apply to master at the moment, unfortunately.

Oops. My patch, which was done against the version present in Ubuntu Karmic, seems to conflict with commit 46dee1da by Peter Hutterer, 2009-10-26 15:24:48 +1000, who reviewed my patch and didn’t say a word.

If I understand correctly, that commit is intended to solve a problem that occurs in the following conditions:
 * The pointing device has physical left and right buttons.
 * The user sets the Mouse Orientation option to (*) Left-handed.
In this case, the X button map has to be set to swap buttons 1 and 3. Since taps are mapped to physical buttons, they get swapped too. So, 1-finger tap would map to physical button 1 mapped to logical button 3, which is bad.

So, when mapping taps, we need to do it in a way that when the button map is applied, it ends up being the correct logical button. This in fact is not so easy as just ((left_handed) ? 3 : 1). Here’s a use case.

Suppose I modify the button map to something like 8 2 9 4 5 6 7 1 3 10 11 12 (via xorg.conf, hal fdi policies or invoking xinput set-button-map). This way, I map the physical left button to act like Back, and the right button to Forward. Then I would need to map 1-finger tap to the (non-existent) physical button 8, so that the button map would turn it to logical button 1.

If the modified button map does not contain 1 at all, the problem becomes unsolvable.

I will present an updated patch later. Meanwhile, please discuss.
Comment 24 Peter Hutterer 2010-01-19 08:03:11 UTC
(In reply to comment #23)
> (In reply to comment #20)
> Oops. My patch, which was done against the version present in Ubuntu Karmic,
> seems to conflict with commit 46dee1da by Peter Hutterer, 2009-10-26 15:24:48
> +1000, who reviewed my patch and didn’t say a word.

sorry for the conflict, I wasn't sure which one will get committed first and has to resolve the conflict. I guess I won ;)
 
> If I understand correctly, that commit is intended to solve a problem that
> occurs in the following conditions:
>  * The pointing device has physical left and right buttons.
>  * The user sets the Mouse Orientation option to (*) Left-handed.
> In this case, the X button map has to be set to swap buttons 1 and 3. Since
> taps are mapped to physical buttons, they get swapped too. So, 1-finger tap
> would map to physical button 1 mapped to logical button 3, which is bad.

not quite, that commit is intended to fix for touchpads that only have a single button (i.e. older apple laptops). in this case most left-handed users don't really want their single button to be a right button (I assume so, anyway).

For your patch, this is a bit harder. I think the best approach might just be to rely on the users to not actively apply button maps that don't make sense. It's easy enough to tell them how to configure this particular physical device.

so you can still say - look, if you want the phys. button to be 8, but tap_button1 to be 1, you have to adjust the button map accordingly.
Comment 25 Yuri Khan 2010-01-19 11:38:33 UTC
(In reply to comment #24)
> not quite, that commit is intended to fix for touchpads that only have a single
> button (i.e. older apple laptops). in this case most left-handed users don't
> really want their single button to be a right button (I assume so, anyway).

No, I’m not talking about your most recent commit 868060f2 (Don't allow left-handed setting for single-button touchpads); the conflict is with 46dee1da dated October 30 (mouse: allow left-handed setting for touchpads). At that time, Karmic had just been released, and I was waiting for another critical bug to be fixed before taking the plunge.

> For your patch, this is a bit harder. I think the best approach might just be
> to rely on the users to not actively apply button maps that don't make sense.

I disagree. That map I showed does in fact make a lot of sense — it allows me to use my two physical buttons for browser Back/Forward while retaining left/middle/right logical buttons on 1/2/3-finger taps.

Tentatively, I am planning to use the button search technique from configure_button_layout: If the user wants n-finger tap to be logical button k, I search the button map for the index j where the value is k and put j+1 into data[3+n], where data is the buffer to assign to the Synaptics Tap Action property.

This will most probably also eliminate the need to pass the left_handed parameter to set_tap_to_click.
Comment 26 Yuri Khan 2010-01-20 18:40:00 UTC
Created attachment 151860 [details] [review]
Adds tap-to-click button settings, v4.

Here’s an updated patch.

As a side note, I suspect the current master will not compile without HAVE_X11_EXTENSIONS_XINPUT_H. I did not attempt to address this issue.
Comment 27 Peter Hutterer 2010-01-22 03:55:17 UTC
(In reply to comment #25)
> No, I’m not talking about your most recent commit 868060f2 (Don't allow
> left-handed setting for single-button touchpads); the conflict is with 46dee1da
> dated October 30 (mouse: allow left-handed setting for touchpads). At that
> time, Karmic had just been released, and I was waiting for another critical bug
> to be fixed before taking the plunge.
> 
> > For your patch, this is a bit harder. I think the best approach might just be
> > to rely on the users to not actively apply button maps that don't make sense.

sorry, mixed up the commits, had too many windows open.

> I disagree. That map I showed does in fact make a lot of sense — it allows me
> to use my two physical buttons for browser Back/Forward while retaining
> left/middle/right logical buttons on 1/2/3-finger taps.

Your map makes sense, though my comment referred to a button map that doesn't include a button 1. This is where it becomes tricky and thus we have to rely on users not to do that.

> Tentatively, I am planning to use the button search technique from
> configure_button_layout: If the user wants n-finger tap to be logical button k,
> I search the button map for the index j where the value is k and put j+1 into
> data[3+n], where data is the buffer to assign to the Synaptics Tap Action
> property.
> 
> This will most probably also eliminate the need to pass the left_handed
> parameter to set_tap_to_click.

This certainly makes sense and should fix the issue. The other option is to hardcode certain things into the touchpad handling. I haven't seen touchpads with more than three phys. buttons, so we can assume that if g-s-d sets the tap-actions to buttons 8/9/10 and then adjusts the button map for these three, we can simply ignore the tapping for left/right handed as well while retaining the ability to use any logical button as we want to.
Does that make sense?
Comment 28 Yuri Khan 2010-01-22 06:03:13 UTC
(In reply to comment #27)
> my comment referred to a button map that doesn't include a button 1.
> This is where it becomes tricky and thus we have to rely on users 
> not to do that.

Agree.

> This certainly makes sense and should fix the issue. The other option is to
> hardcode certain things into the touchpad handling. I haven't seen touchpads
> with more than three phys. buttons, so we can assume that if g-s-d sets the
> tap-actions to buttons 8/9/10 and then adjusts the button map for these three,
> we can simply ignore the tapping for left/right handed as well while retaining
> the ability to use any logical button as we want to.
> Does that make sense?

It would and actually would simplify things a lot. BUT, they say X does not allow more than one physical button to map to the same logical button. And many will want just that — both 1-finger tap and left hardware button to map to logical button 1.
Comment 29 Peter Hutterer 2010-01-23 00:54:01 UTC
(In reply to comment #28) 
> It would and actually would simplify things a lot. BUT, they say X does not
> allow more than one physical button to map to the same logical button. And many
> will want just that — both 1-finger tap and left hardware button to map to
> logical button 1.

this is only true for the core protocol request, not for the XInput one that we use here. you're pretty much free to map as you want, just try out "xinput --set-button-map 'device name' 1 1 1 1 1"
Comment 30 Yuri Khan 2010-01-23 12:16:03 UTC
(In reply to comment #29)
> this is only true for the core protocol request, not for the XInput one that we
> use here. you're pretty much free to map as you want, just try out "xinput
> --set-button-map 'device name' 1 1 1 1 1"

Tempting. But environmentally unfriendly.

In my current configuration, the touchpad claims to have 12 buttons. At first glance this seems a lot, but let’s see how many gestures compete for them.

 * Left and right hardware buttons, commonly mapped to 1 and 3.
 * Left+right hardware buttons, emulating the middle button, mapped to 2.
 * Vertical edge scrolling and/or vertical two-finger scrolling and/or circular scrolling and/or scroll up/down buttons, generating events 4 and 5.
 * Horizontal edge scrolling and/or horizontal two-finger scrolling and/or scroll left/right buttons, generating 6 and 7.
 * Corner taps, at top left, top right, bottom left and bottom right. Can be mapped to any of the 12 virtual physical buttons through the "Synaptics Tap Action" property.
 * Finger taps, with 1, 2 and 3 fingers. Also mappable to any of the 12 buttons via "Synaptics Tap Action".
 * Clicking the left hardware button while tapping with 1, 2 or 3 fingers. Can be mapped to any of the 12 buttons via "Synaptics Click Action" property.

So, there are 17 gestures that can generate button events, 10 of them can be mapped to any button; but there are only 5 unused buttons. This means we cannot assign a separate button to each customizable gesture. And if we do it for three of ten gestures, someone later will curse us for hogging the scarce resource.
Comment 31 Yuri Khan 2010-02-04 05:07:26 UTC
Created attachment 152988 [details] [review]
Adds tap-to-click button settings, v5.

Fixed one coding style issue.

I decided against hard-coding high-numbered unused buttons for taps. Instead, this patch uses the button search technique described in comment 25.

Please review.
Comment 32 Peter Hutterer 2010-02-17 06:22:31 UTC
Review of attachment 152988 [details] [review]:

First of all, thank you for hanging in there and for providing another bug. I'm really sorry for the long delay, things are rather busy here and too much had to go on back burner. Please find my comments below:

+      <key>/schemas/desktop/gnome/peripherals/touchpad/tap_button_1</key>

Is it just me or are the schemas in there twice? Is this intended?

+        <short>Button assigned to one finger tap</short>
I _think_ this should be one-finger tap (hyphenated), same goes for two-finger and three-finger.

+static int      update_tap_to_click           ();                                                  

this should be update_tap_to_click(void)

@@ -321,11 +325,8 @@ set_xinput_devices_left_handed (gboolean left_handed)
this hunk makes the check for single button useless since you don't do anything with the return value anymore. Why not remove the check?

+        gint tap_physical_1 = tap_button_1, tap_physical_2 = tap_button_2, tap_physical_3 = tap_button_3;
I'd prefer this to be split up over three lines to ease readability. so in the form
gint tap_physical_1 = tap_button_1,
     tap_physical_2 = tap_button_2,
     tap_physical_3 = tap_button_3;


The XGetDeviceButtonMapping code seems duplicated from set_xinput_devices_left_handed. This should possibly be moved into a helper function to avoid getting out of sync. Maybe a follow-up patch?

       /* Set RLM mapping for 1/2/3 fingers*/
this line should be removed with this patch since it's not accurate anymore.

+      for (j = 0; j < n_buttons; j++)     
this block should be commented with the approach you've picked. It took me a while to figure out what you're doing there and since the approach is somewhat unusual, a comment would help.
Comment 33 Yuri Khan 2010-02-17 18:02:29 UTC
Created attachment 154059 [details] [review]
Adds tap-to-click button settings, v6.

(In reply to comment #32)
> Review of attachment 152988 [details] [review]:

(skipped and applied the rest of suggestions)

> @@ -321,11 +325,8 @@ set_xinput_devices_left_handed (gboolean left_handed)
> this hunk makes the check for single button useless since you don't do anything
> with the return value anymore. Why not remove the check?

No, there’s an “if (single_button) continue;” two lines down there, which skips a configure_button_layout call.
Comment 34 Yuri Khan 2010-02-17 20:17:58 UTC
Created attachment 154078 [details] [review]
Adds tap-to-click button settings, v6a.

No changes, should have configured my git earlier.
Comment 35 Peter Hutterer 2010-02-18 06:50:22 UTC
Review of attachment 154078 [details] [review]:

thank you!
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Comment 36 Jens Granseuer 2010-02-20 09:46:24 UTC
Review of attachment 154078 [details] [review]:

Just a small niggle left. Since we've been in feature freeze for quite a while this won't make it in for 2.30 (unless you touchpad users deem it important enough that one of you applies for a freeze exemption which I'd be ok with, see http://live.gnome.org/ReleasePlanning/RequestingFreezeBreaks). We're still in the string change announcement period so we'd just violate the feature freeze here.

::: plugins/mouse/gsd-mouse-manager.c
@@ +617,3 @@
 
+        buttons = g_new (guchar, buttons_capacity);
+        if (!buttons)

This is unnecessary. glib will bomb when allocation fails, and nothing else ever checks for allocation errors from g_*, either.
Comment 37 Yuri Khan 2010-02-20 14:59:20 UTC
Created attachment 154270 [details] [review]
Adds tap-to-click button settings, v7.

Removed g_new null check, as per Jens Granseuer’s suggestion.

I am applying for a feature freeze break, as I strongly consider this bug to be a usability regression and want the fix in GNOME 2.30 and Ubuntu Lucid.
Comment 38 Yuri Khan 2010-02-28 17:40:20 UTC
Review of attachment 154270 [details] [review]:

::: plugins/mouse/gsd-mouse-manager.c
@@ +301,3 @@
+{
+        gint n_buttons = XGetDeviceButtonMapping (GDK_DISPLAY (), device,
+                                                  buttons,

Forgot to pass 'device' as parameter. Will not compile.
Comment 39 Yuri Khan 2010-02-28 18:56:36 UTC
Created attachment 154905 [details] [review]
Adds tap-to-click button settings, v8.

This one actually compiles.
Comment 40 Peter Hutterer 2010-02-28 23:58:02 UTC
wait. you submitted a patch that didn't compile? (and requested a feature freeze break) 
how did you test the patch?
Comment 41 Yuri Khan 2010-03-01 06:47:30 UTC
I stand ashamed and humiliated. This will teach me to test *every* revision of the patches I do.

I will now go build test packages for Ubuntu, try them myself and have them tried by somebody else.
Comment 42 Filippo Argiolas 2010-03-01 07:20:02 UTC
Hi, I'm not totally against Yuri's approach here and I'd really like to have this fixed better sooner than later since my physical left button is almost broken and doing middle clicks with three fingers is such a pain...
But, wouldn't it be easier to follow what I suggested in the duplicated bug? i.e. save current mappings (from xorg configuration) at g-s-d startup, do the RL swapping if enabled in preferences, and restore saved settings if touchpad is disabled and re-enabled.

This would not require any freeze break and there would be all the time from now until 2.30.2 to implement it if you are busy... It would also avoid adding yet another entry point for touchpad mapping configuration. Xorg as far as I know is also moving to a saner configuration system and we would be just ignoring it adding our custom gconf based one.
Comment 43 Tommaso Pasini 2010-04-01 13:58:14 UTC
Created attachment 157697 [details] [review]
Adds tap-to-click button settings, v8b

The v8-patch works fine against gnome-settings-daemon-2.29 but fails against 2.30. I noticed a few #ifdef have been added to gsd-mouse-manager.c and those are the reason the patch fails.

I tried to update the patch so that I could restore the behaviour on my system. FWIW, it works here, so I'm sharing it just in case you need it.
Comment 44 Yuri Khan 2010-04-01 17:33:54 UTC
Now that 2.30 is out and the freeze is(?) over, I intend to re-evaluate my patch once again and eventually get it committed. I do not want to rush as I got burnt last time.

I wonder if there are any objections to managing corner taps along with regular finger taps. Ubuntu maps right top and right bottom to middle and right button by default, and they fall into the same trap of not taking the button map into account. If GNOME did all the heavy lifting of reversing the button map and providing customization, Ubuntu would only need to change the schema defaults.
Comment 45 Yuri Khan 2010-04-03 09:29:37 UTC
Created attachment 157792 [details] [review]
Adds tap-to-click button settings, v9.

Rebased against current master.

Also, fixed a bug introduced when factoring out get_device_button_mapping: buttons array should be passed by pointer, since it is potentially reallocated. (That would not manifest until synaptics button count grew above 16.)

The initial values for tap_physical_* variables changed to 0 -- after all, there is not much we can do if the button map does not contain the desired logical button. Better disable than assign a different button.
Comment 46 Yuri Khan 2010-04-03 09:33:09 UTC
Created attachment 157793 [details] [review]
Adds corner-click button settings, with the same logic.

Follow-up patch that extends the same logic to corner clicking.

Should we have a separate master switch for corner clicking? A separate checkbox in gnome-mouse-properties? A GUI for configuring the tap and corner to button mapping?
Comment 47 Tommaso Pasini 2010-04-03 14:13:09 UTC
The patch for corner clicks works, but there needs to be a way to configure the area: maybe it's just my touchpad, but the sensitive area happens to be too tiny and half times the click is recognised as a left one.

I think there should be a GUI option to enable corner clicks, while the size of the area should be hidden inside gconf.

Even better, I'd love a checkbox at the top that, if checked, disables any touchpad configuration by gnome-settings-daemon and lets the user fine-tune every single setting via hal.
Comment 48 Yuri Khan 2010-04-03 17:52:30 UTC
(In reply to comment #47)
> The patch for corner clicks works, but there needs to be a way to configure the
> area: maybe it's just my touchpad, but the sensitive area happens to be too
> tiny and half times the click is recognised as a left one.

Why not configure the area at xorg/hal/udev level? It's likely to be per-machine anyway. Button mapping, on the other hand, is a matter of personal preference so should be per-user, which is why I went the gconf way in the first place.
Comment 49 Jens Granseuer 2010-05-06 15:08:40 UTC
*** Bug 617911 has been marked as a duplicate of this bug. ***
Comment 50 Yuri Khan 2010-10-10 16:47:57 UTC
Another code freeze has come and gone. I have been testing my patches on Ubuntu users (Lucid and Maverick alphas/betas) and the feedback has been positive (https://launchpad.net/bugs/563276), so I would really like to get the patches incorporated into GNOME.

I have rebased them to current master and tested that they compile. The resulting binary is not going to work due to bugs in gsettingsification, though (see 631502). Ironing them out will require another rebase (involving merge conflicts) on my part, so waiting.
Comment 51 Yuri Khan 2010-11-21 14:15:50 UTC
Created attachment 174950 [details] [review]
Adds tap-to-click button settings, rebased 2010-11-21.

Ok, I have installed an early pre-alpha of Ubuntu Natty that has all the required packages to build and run the current master.

Here are the newly rebased patches. Please review.
Comment 52 Yuri Khan 2010-11-21 14:17:01 UTC
Created attachment 174951 [details] [review]
Adds corner-click button settings with the same logic, rebased 2010-11-21.
Comment 53 Yuri Khan 2010-11-21 14:32:57 UTC
Created attachment 174958 [details] [review]
Implement UI in gnome-control-center for the above two patches.
Comment 54 Margarita Manterola 2011-01-02 19:45:19 UTC
Hi!

It's been more than a year since this bug got reported, and it still hasn't been applied.

What's preventing this from getting applied?  Please do not let yet another gnome version with this nasty bug in it.
Comment 55 Yuri Khan 2011-01-03 15:52:47 UTC
The patch is waiting for someone knowledgeable to review it. I am alive and available for discussion and have some free time for development (until 2011-01-10, after which the New Year holidays end and my time will be more limited).
Comment 56 Peter Hutterer 2011-01-04 07:12:09 UTC
Review of attachment 174950 [details] [review]:

in set_tap_to_click(): can you add a comment for why j+1 is necessary (button
numbering starts at 1). It took me a while to figure out what this is doing
here, and tbh the comment confused me a bit more. How about something like
this: "Synaptics maps taps to physical buttons, X then maps physical buttons
to logical buttons. Search for a physical button with the desired logical
button mapping and make tapping emit that physical button code."
Since it's comments, you can be more verbose than this :)

+update_tap_to_click ()
→ +update_tap_to_click (void) if I'm not mistaken.

this is bikeshedding, but maybe using a "one-finger-tap", etc naming instead
of tap-button-1? It seems more self-explanatory? I don't think tap button is
a good naming in the driver, but there we're stuck with it.

patch looks good, though I haven't tested it.
Comment 57 Peter Hutterer 2011-01-04 07:14:01 UTC
Review of attachment 174951 [details] [review]:

same comments as for the other one. "corner-tap-rt" maybe instead of "tap-button-rt"?
Comment 58 Wendall Cada 2011-01-04 18:21:00 UTC
Thanks Yuri for the patch.

Considering that every reference in documentation refers to these as 1/2/3, the naming in the patch makes sense. However, whatever it takes to unbreak this feature, name it colors of the rainbow for all I care. ;)
Comment 59 Yuri Khan 2011-01-05 07:47:14 UTC
Created attachment 177550 [details] [review]
[PATCH 1/4, rebased 2011-01-05] mouse: Extract get_device_button_mapping function.

I have restructured the patch so that it (hopefully) is easier to understand.

The second patch specifically fixes the problem hinted at in comment #23. Should I take it out into a separate bug?
Comment 60 Yuri Khan 2011-01-05 07:48:12 UTC
Created attachment 177551 [details] [review]
[PATCH 2/4] mouse: Fully take button mapping into account when setting tap-to-click.
Comment 61 Yuri Khan 2011-01-05 07:49:18 UTC
Created attachment 177552 [details] [review]
[PATCH 3/4, rebased 2011-01-05] mouse: Add tap-to-click button settings
Comment 62 Yuri Khan 2011-01-05 07:50:18 UTC
Created attachment 177553 [details] [review]
[PATCH 4/4, rebased 2011-01-05] mouse: Add corner-click button settings.
Comment 63 Yuri Khan 2011-01-05 07:56:13 UTC
Created attachment 177554 [details] [review]
gnome-control-center/mouse: Add tap-to-click button settings.

I have tested these patches by applying them to gnome-settings-daemon 2.91.6.1-0ubuntu1~build1 and gnome-control-center 1:2.91.4-0ubuntu1~build1 from the https://launchpad.net/~gnome3-team/+archive/gnome3 repository, and they work for me (as far as those versions can be considered working).
Comment 64 Yuri Khan 2011-01-08 09:04:26 UTC
Created attachment 177811 [details] [review]
[1/2] mouse: Add tap-to-click button settings

The patches that deal with the button mapping have been moved out as a separate bug 638973. Now please let’s get them committed.
Comment 65 Yuri Khan 2011-01-08 09:05:12 UTC
Created attachment 177812 [details] [review]
[2/2] mouse: Add corner-click button settings.
Comment 66 Yuri Khan 2011-01-08 09:09:02 UTC
Created attachment 177813 [details] [review]
gnome-control-center/mouse: Add tap-to-click button settings.
Comment 67 Yuri Khan 2011-01-16 16:37:24 UTC
Please, what else can I do to get these two bugs resolved? I really want to get the patches in before the nearest feature freeze.
Comment 68 Peter Hutterer 2011-01-18 22:20:04 UTC
Review of attachment 177811 [details] [review]:

I see the reference to find_button() in both patches but can't find the definition for it. Did this go missing?
Comment 69 Yuri Khan 2011-01-19 04:23:51 UTC
(In reply to comment #68)
> I see the reference to find_button() in both patches but can't find the
> definition for it. Did this go missing?

I moved it to bug 638973 and marked it as blocking this one. The patches in this bug deal with customizability, the patches over there deal with correct button map handling.
Comment 70 Peter Hutterer 2011-01-31 05:26:35 UTC
Review of attachment 177811 [details] [review]:

Not sure if the .convert part is really needed but someone with more gnome
experience can answer that I expect. Other than that:

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Comment 71 Peter Hutterer 2011-01-31 05:29:19 UTC
Review of attachment 177812 [details] [review]:

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

(same comment regarding .convert)
Comment 72 Peter Hutterer 2011-01-31 05:33:57 UTC
Review of attachment 177813 [details] [review]:

Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

looks sensible enough but I haven't tested it. In regards to the button
combo - is there a preference whether this should be done in the .ui file
directly instead of in code?
Comment 73 Yuri Khan 2011-01-31 16:24:25 UTC
(In reply to comment #70)
> Review of attachment 177811 [details] [review]:
> 
> Not sure if the .convert part is really needed but someone with more gnome
> experience can answer that I expect. Other than that:

The .convert part is not technically necessary but is a convenience for users who have used these patches before gsettingsification.

(In reply to comment #72)
> Review of attachment 177813 [details] [review]:
> 
> In regards to the button combo - is there a preference whether this should 
> be done in the .ui file directly instead of in code?

You mean, populating the combo boxes declaratively rather than programmatically? (Is that possible?) I copied the technique from nearby code (former Mouse | Accessibility tab); it got deleted somewhere on the way though.
Comment 74 Peter Hutterer 2011-01-31 21:58:38 UTC
(In reply to comment #73)
> You mean, populating the combo boxes declaratively rather than
> programmatically? (Is that possible?) I copied the technique from nearby code
> (former Mouse | Accessibility tab); it got deleted somewhere on the way though.

don't take this as gospel but you can see an example here:
http://cgit.freedesktop.org/~whot/gnome-control-center/tree/panels/wacom/gnome-wacom-properties.ui?h=wacom#n802

seems to work for me, and that way everything UI related is in the .ui file, nothing in code.
Comment 75 Yuri Khan 2011-02-01 08:14:50 UTC
(In reply to comment #74)
> (In reply to comment #73)
> > You mean, populating the combo boxes declaratively rather than
> > programmatically? (Is that possible?)

> don't take this as gospel but you can see an example here:
> http://cgit.freedesktop.org/~whot/gnome-control-center/tree/panels/wacom/gnome-wacom-properties.ui?h=wacom#n802

Nice. Would it be possible to reference that list store, or do I need to duplicate it?
Comment 76 Peter Hutterer 2011-02-02 05:40:26 UTC
probably duplicate it. the list stores aren't shared between plugins unless you pass the GtkBuilder around
Comment 77 Yuri Khan 2011-02-07 13:20:01 UTC
Created attachment 180295 [details] [review]
gnome-control-center/mouse: Add tap-to-click button settings.

Reorganized patch to populate the combo boxes from the .ui file rather than programmatically.
Comment 78 Peter Hutterer 2011-02-10 00:37:51 UTC
Review of attachment 180295 [details] [review]:

Still looks sane to me :)

Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
Comment 79 Bastien Nocera 2011-03-21 14:14:42 UTC
In bug 635486, you'll find a new feature in the gnome-settings-daemon mouse plugin. The mouse plugin can now launch a script on startup, for each device, and thereafter for each added or removed devices.

You will then be able to come up with any kind of crazy customisations you fancy for your touchpads, even if not supported by gnome-settings-daemon or gnome-control-center. And gnome-settings-daemon will not touch your devices any more when the correct return value is returned from the scripts.

Any problems with the interaction, on the gnome-settings-daemon side, please report as new bugs.

I will not take any of the patches attached to this bug, as the mouse panel is to undergo re-design for GNOME 3.2, and the functionality those patches add haven't been vouched for design-wise.

*** This bug has been marked as a duplicate of bug 635486 ***