GNOME Bugzilla – Bug 589944
gnome-settings-daemon uses hardcoded tapping settings
Last modified: 2010-01-16 10:40:14 UTC
Why mouse plugin of g-s-d uses hardcoded Synaptics tap action? Shouldn't it be taken from gconf at least? * Set RLM mapping for 1/2/3 fingers*/ data[4] = (state) ? 1 : 0; data[5] = (state) ? 3 : 0; data[6] = (state) ? 2 : 0; I don't like this settings. I prefer RML for 1/2/3 fingers setting. I would like to change it via gconf or some other way different from changing sources and recompiling. Other information:
I strongly disagree recent synaptics developers decision of setting tapping defaults to TapButton2=3 and TapButton3=2 since I'm a long time linux user and I'm accustomed to the previous TapButton2=2 and TapButton3=3 setup. I definitely use middle click (copy and paste) more often than right click and I want it easily accessible (3 fingers tap is harder to get right). I suppose they had good reasons for that choice, so I don't complain and usually revert it from xorg.conf to my beloved behaviour but now that's not possible anymore since gsd blindly resets my settings to its hardcoded one as soon as it starts. Proposed patch following.
Created attachment 147717 [details] [review] mouse: honor current tap button mappings Don't hardcode tapping button mapping when activating tap_to_click. Honor current driver settings instead and just swap 1 and 2-finger tapping for left handed configuration
Ok, the proposed patch doesn't work if you disable tapping and want to reenable it afterwards since data[4,5,6] are all zero. Maybe g-s-d should save current settings when it starts and restore to saved ones when you want to reenable them or just use the hardcoded defaults if previous data[4,5,6] are zero.
Agreed; it's better not to have a default RLM mapping if it's going to stomp on user customizations. The synaptics RML default works, something like Filippo proposes would also work, but silently throwing out user customizations is broken.
Created attachment 150938 [details] [review] A workaround A workaround, not tested yet. * Save current settings and restore them as Filippo said in comment#3. * Also save current settings when a device is connected to. * Also save current settings just before disable tap-to-click (i.e. set all values to zero) to avoid breaking the settings which was changed by xinput after tap-to-click was enabled by gnome-control-center. I think this fix works in most cases.
attachment 150938 [details] [review] also fixes bug 600128.
Review of attachment 150938 [details] [review]: +typedef struct _TapActions +{ + char *values; + int nitems; +} TapActions; Given that the TapActions are always size 7, wouldn't it be easier to just do it as a uchar[7] and use sizeof instead of nitems? +enable_tap_to_click (GsdMouseManager *manager, XDevice *device, const char *device_name) looking at this function, it seems it can be split up in a better way (and potentially merged with disable_tap_to_click). You can move the init of the tap buttons into a separate function for example. + XDeviceInfo *devicelist = XListInputDevices (GDK_DISPLAY (), &numdevices); + XDevice * device; inconsistency in the coding style, there's at least one more issue with a space lacking before a (. Please re-check. + * collect_tap_actions_values() is called not only when + * gsd starts but also device is connected, so the device + * which has connected at gsd start-up time should be ignored here. the "at" is confusing me, should that be "before" or "after"? +collect_tap_actions_values (GsdMouseManager *manager) i think this should be enclosed in #ifdef HAVE_X11_EXTENSIONS_XINPUT_H. it might simply be outside the patch context though, please check. prop = XInternAtom (GDK_DISPLAY (), "Synaptics Tap Action", False); - if (!prop) return 0; whitespace hunk, just leave the code as-is. + data[1] = 3; /* button 3 at right bottom corner */ huh?
(In reply to comment #7) > +typedef struct _TapActions > +{ > + char *values; > + int nitems; > +} TapActions; > > Given that the TapActions are always size 7, wouldn't it be easier to just do > it as a uchar[7] and use sizeof instead of nitems? If the size assumes always 7, it can be easier than before. TapActions will not be needed. But I don't think uchar[7] can be used anywhere. Because those values are stored in hash table to handle multiple devices, so those must be dynamic allocated. I will try to remove TapActions. > +enable_tap_to_click (GsdMouseManager *manager, XDevice *device, const char > *device_name) > > looking at this function, it seems it can be split up in a better way (and > potentially merged with disable_tap_to_click). You can move the init of the tap > buttons into a separate function for example. I am not convinced that such code will be better. But anyway, I will try to do so. > + XDeviceInfo *devicelist = XListInputDevices (GDK_DISPLAY (), > &numdevices); > + XDevice * device; > > inconsistency in the coding style, there's at least one more issue with a space > lacking before a (. Please re-check. I'll fix those. > + * collect_tap_actions_values() is called not only > when > + * gsd starts but also device is connected, so the > device > + * which has connected at gsd start-up time should be > ignored here. > > the "at" is confusing me, should that be "before" or "after"? I meant "before". > +collect_tap_actions_values (GsdMouseManager *manager) > > i think this should be enclosed in #ifdef HAVE_X11_EXTENSIONS_XINPUT_H. it > might simply be outside the patch context though, please check. I don't really get the HAVE_X11_EXTENSIONS_XINPUT_H role. I think that is for XDevicePresenceNotifyEvent, isn't it? If not (the macro means as it is), all the other features related to touchpad should be also enclosed. > + data[1] = 3; /* button 3 at right bottom corner */ > > huh? I misunderstood that this option is a driver's default value since my laptop has its value (maybe Ubuntu does so). I'll remove this. Thank you for your review and helpful advices.
Created attachment 151019 [details] [review] Revised patch This patch is a bit shorter. Changes from attachment 150938 [details] [review]. * Remove TapActions struct. * Use unsigned char[7] and sizeof to set values (i.e. XChangeDeviceProperty). * Remove enable/disable_tap_to_click functions. Only storing current tap action values splits away from set_tap_to_click() and collect_tap_actions_values() also uses the storing function. * Fix comment in collect_tap_actions_values(). (at -> before) * Fix some style issues. * Remove right bottom corner action value. I left the HAVE_X11_EXTENSIONS_XINPUT_H issue. I tested this patch now. It works fine at least one touchpad device. I have not tested with multiple touchpad devices yet.
(In reply to comment #8) > > i think this should be enclosed in #ifdef HAVE_X11_EXTENSIONS_XINPUT_H. it > > might simply be outside the patch context though, please check. > > I don't really get the HAVE_X11_EXTENSIONS_XINPUT_H role. I think that is for > XDevicePresenceNotifyEvent, isn't it? > If not (the macro means as it is), all the other features related to touchpad > should be also enclosed. this is to enable building on systems without libXi installed. but you're right, it looks a bit broken at the moment - if it were undefined the source couldn't compile AFAICT. probably better in a follow-up patch.
Review of attachment 151019 [details] [review]: Thanks, looking a bit better and I only have two style issues to comment on. +store_current_tap_actions_data I'd prefer if the if (rc == Success) condition was split up and the xfree merged. as in if (rc == Success) { if (other checks) { g_hash_table_insert ... } XFree(data); } that's personal preference though. + set_tap_to_click the if (enable) block should probably be moved into a separate function so that set_tap_to_click can be reduced to for (... all devices ) { if (device is touchpad) { if (enable) restore_tap_actions_data(); else store_tap_actions_data() XChangeDeviceProperty() ... } } (I hope this makes sense...) Finally, I'd recommend that you pass in the size of the array into tap_actions_values_all_zero so that it works in a more generic manner. just in case the tap actions need to be changed lateron there's only one place to change the size 7. Having said that, I do now see the benefit of your previous TapAction approach so if you preferred that approach I'm happy with it too.
(In reply to comment #11) > +store_current_tap_actions_data > > I'd prefer if the if (rc == Success) condition was split up and the xfree > merged. as in > if (rc == Success) { > if (other checks) { > g_hash_table_insert ... > } > XFree(data); > } I will do so. > + set_tap_to_click > the if (enable) block should probably be moved into a separate function so that > set_tap_to_click can be reduced to > > for (... all devices ) { > if (device is touchpad) { > if (enable) > restore_tap_actions_data(); > else > store_tap_actions_data() > XChangeDeviceProperty() > ... > } > } > > (I hope this makes sense...) > > Finally, I'd recommend that you pass in the size of the array into > tap_actions_values_all_zero so that it works in a more generic manner. just in > case the tap actions need to be changed lateron there's only one place to > change the size 7. Having said that, I do now see the benefit of your previous > TapAction approach so if you preferred that approach I'm happy with it too. Ah, I am sorry, I misunderstood about what you said in comment #7. You mean that TapActions has just uchar[7] value like this? #define N_TAP_ACTIONS 7 typedef struct _TapActions { unsigned char values[N_TAP_ACTIONS]; } TapActions; I also think this is the best.
(In reply to comment #12) > > Finally, I'd recommend that you pass in the size of the array into > > tap_actions_values_all_zero so that it works in a more generic manner. just in > > case the tap actions need to be changed lateron there's only one place to > > change the size 7. Having said that, I do now see the benefit of your previous > > TapAction approach so if you preferred that approach I'm happy with it too. > > Ah, I am sorry, I misunderstood about what you said in comment #7. You mean > that TapActions has just uchar[7] value like this? > > #define N_TAP_ACTIONS 7 > typedef struct _TapActions > { > unsigned char values[N_TAP_ACTIONS]; > } TapActions; > > I also think this is the best. Or you mean my first approach is good to me now? having nitems is nice?
(In reply to comment #13) > Or you mean my first approach is good to me now? having nitems is nice? yes, with using nitems. I think it makes sense to have it in case the property is extended in the future.
Created attachment 151072 [details] [review] Update patch This patch revived TapActions structure with nitems and use the structure to avoid the use of magic number "7" as far as possible.
*** Bug 591696 has been marked as a duplicate of this bug. ***
I think bug 598820 is related if not a duplicate. There, I propose a different patch. To compare: * Hiroyuki’s patch makes g-s-d only turn tap-to-click on and off, leaving the details in the hands of whatever configures TapButton1..3 before g-s-d (xorg.conf, hal fdi policy, synaptics defaults etc). Advanced users can reconfigure those defaults and will need to restart X or force device reconnection. * With my patch, g-s-d takes over TapButton# settings. The user can then reconfigure them on the fly in gconf-editor. Later, someone might write a patch to add a GUI for these settings.
> * With my patch, g-s-d takes over TapButton# settings. The user can then > reconfigure them on the fly in gconf-editor. Later, someone might write a patch > to add a GUI for these settings. If this is true, then I like Yuri's approach better.
*** This bug has been marked as a duplicate of bug 598820 ***