Review of attachment 301125 [details] [review]:

This would break ensure_touchpad_active() because that function relies on get_disabled_devices() which asks X for a list of disabled devices and by disabling touchpads via this synaptics property would still make them enabled from X's point of view. So, ensure_touchpad_active() needs to be changed to determine that touchpads are disabled by looking at this synaptics property too.

Created attachment 301984 [details] [review]
mouse: Do not disable touchpad buttons

Thanks for the review! I've modified get_disabled_devices to return also devices with positive value of "Synaptics Off" property. This slightly reduces usability of the get_disabled_devices function, but the function is used only in ensure_touchpad_active currently...

Review of attachment 301984 [details] [review]:

Looks fine to me otherwise.

::: plugins/common/gsd-input-helper.c
@@ +583,3 @@
 get_disabled_devices (GdkDeviceManager *manager)
 {
+        GdkDisplay *display = gdk_display_get_default ();

Don't assign in the declaration please, do this at the top of the function.

@@ +610,3 @@
                 device = gdk_x11_device_manager_lookup (manager, device_info[i].id);
+                if (device != NULL) {
+                        prop = gdk_x11_get_xatom_by_name ("Synaptics Off");

You should do this outside the loop.

(In reply to Ondrej Holy from comment #2)
> Thanks for the review! I've modified get_disabled_devices to return also
> devices with positive value of "Synaptics Off" property. This slightly
> reduces usability of the get_disabled_devices function, but the function is
> used only in ensure_touchpad_active currently...

1. Since the function isn't used by anything else we should rename it (get_disabled_touchpads?) to make its purpose clear

2. Since we are not disabling touchpads at the X level anymore, it doesn't make sense to return disabled devices from this function. For instance if I disable a keyboard device with 'xinput disable <keyboard-device-id>' we would then try to enable it with the synaptics property which wouldn't obviously work.

Basically I think this should all be made specific to touchpads and rely solely on the synaptics prop.

Thanks for the reviews! I realized that the patch currently interfering with syndaemon, because syndaemon isn't terminated properly and use "Synaptics Off" property also... will fix it.

(In reply to Rui Matos from comment #4)
> (In reply to Ondrej Holy from comment #2)
> 2. Since we are not disabling touchpads at the X level anymore, it doesn't
> make sense to return disabled devices from this function. For instance if I
> disable a keyboard device with 'xinput disable <keyboard-device-id>' we
> would then try to enable it with the synaptics property which wouldn't
> obviously work.

I think we should list all the disabled devices also as we do before, but we should enable them also, so to call both set_device_enabled and set_touchpad_device_enabled. It will be useful for people who has disabled touchpad and update to g-s-d with this patch, otherwise they won't be able to enable their touchpad using g-s-d after the update... don't you think?

Also it would be still good to make patch also for g-c-c to show the disabled touchpad at the X level - Bug 747502.

Comment on attachment 301984 [details] [review]
mouse: Do not disable touchpad buttons

What Rui said.

Created attachment 302140 [details] [review]
mouse: Do not disable touchpad buttons

Comments applied. Also removed some redundant calls to enable/disable touchpad and syndaemon is terminated if the touchpads are disabled...

Review of attachment 302140 [details] [review]:

With these comments addressed, a-c-n

::: plugins/common/gsd-input-helper.c
@@ +601,2 @@
         for (i = 0; i < n_devices; i++) {
+                gdk_error_trap_push ();

The error trap push/pop should be done outside the loop

::: plugins/mouse/gsd-mouse-manager.c
@@ +552,3 @@
 set_disable_w_typing (GsdMouseManager *manager, gboolean state)
 {
+        if (state && touchpad_is_present () && get_touchpad_enabled (manager)) {

If you do what I suggest below this added check wouldn't be needed

@@ +1217,2 @@
         if (g_str_equal (key, KEY_SEND_EVENTS)) {
+                set_disable_w_typing (manager, TRUE);

We should move all the calls to set_disable_w_typing() into ensure_touchpad_active() instead since they are always called together.

Comment on attachment 302140 [details] [review]
mouse: Do not disable touchpad buttons

commit b23917f0a279aba4599cdc7a5b34055f3d8975ba

*** Bug 747502 has been marked as a duplicate of this bug. ***

*** Bug 749269 has been marked as a duplicate of this bug. ***

It would be nice to add fix also for gnome-3-14, unfortunately the patch isn't applicable on gnome-3-14, because the code was changed a lot (added _DISABLED_ON_EXTERNAL_MOUSE, missing _DISABLE_W_TYPING...). So reopening...

Created attachment 303764 [details] [review]
mouse: Do not disable touchpad buttons (gnome-3-14)

There is simplified patch for gnome-3-14.

Review of attachment 303764 [details] [review]:

otherwise looks fine

::: plugins/mouse/gsd-mouse-manager.c
@@ +1069,3 @@
+        if (g_str_equal (key, KEY_TOUCHPAD_ENABLED)) {
+                set_disable_w_typing (manager, g_settings_get_boolean (manager->priv->touchpad_settings, KEY_TOUCHPAD_DISABLE_W_TYPING));
+        }

no need for curly braces for a single statement

@@ -1234,3 @@
         ensure_touchpad_active (manager);
 
-        if (g_settings_get_boolean (manager->priv->touchpad_settings, KEY_TOUCHPAD_ENABLED)) {

we should still do this on initialization IMO, we just need to use the full device list instead of only the disabled ones

Review of attachment 303764 [details] [review]:

::: plugins/mouse/gsd-mouse-manager.c
@@ -1240,3 @@
-
-                        device_id = GPOINTER_TO_INT (l->data);
-                        set_touchpad_enabled (device_id);

now that I think about it, we should ensure that touchpads are enabled *or* disabled according to the setting on initialization.

this code here was just incomplete in that regard since it didn't ensure that touchpads were disabled if the setting said so

Thanks for review!

(In reply to Rui Matos from comment #14)
> @@ -1234,3 @@
>          ensure_touchpad_active (manager);
>  
> -        if (g_settings_get_boolean (manager->priv->touchpad_settings,
> KEY_TOUCHPAD_ENABLED)) {
> 
> we should still do this on initialization IMO, we just need to use the full
> device list instead of only the disabled ones

There is such loop few lines above...

(Removed loop was there before to enable disabled devices, because they are not returned from gdk_device_manager_list_devices in the above loop).

(In reply to Rui Matos from comment #15)
> Review of attachment 303764 [details] [review] [review]:
> 
> ::: plugins/mouse/gsd-mouse-manager.c
> @@ -1240,3 @@
> -
> -                        device_id = GPOINTER_TO_INT (l->data);
> -                        set_touchpad_enabled (device_id);
> 
> now that I think about it, we should ensure that touchpads are enabled *or*
> disabled according to the setting on initialization.
> 
> this code here was just incomplete in that regard since it didn't ensure
> that touchpads were disabled if the setting said so

...the above loop calls set_mouse_settings() and there is set_touchpad_disabled(). The touchpads are there just disabled, but the devices should be enabled by default, so it should be enough, do you agree?

(In reply to Ondrej Holy from comment #16)
> ...the above loop calls set_mouse_settings() and there is
> set_touchpad_disabled(). The touchpads are there just disabled, but the
> devices should be enabled by default, so it should be enough, do you agree?

No, I think we shouldn't rely on existing state in the X server and instead be explicit so that we don't end up in an inconsistent state where we think devices are enabled but in reality they aren't however unlikely that might be.

Created attachment 303824 [details] [review]
mouse: Do not disable touchpad buttons (gnome-3-14)

Ok, removed brackets and enable touchpads explicitly :-)

Review of attachment 303824 [details] [review]:

push it

Comment on attachment 303824 [details] [review]
mouse: Do not disable touchpad buttons (gnome-3-14)

commit 7a4c41dc709bab94f684efcfc04325797d432912

Thanks for review!

This change affects the "toggle touchpad" media key on the Lenovo Yoga 900. The keypress event is sent through the legacy keyboard device, and syndaemon interprets it as typing, so it changes the "Synaptics Off" property back to 0 after 1s (checked with xinput watch-props). Simply changing the order between changing the "disable while typing" setting and the "Synaptics Off" property makes syndaemon die earlier and not interfere with the "toggle touchpad" media key.

I'm attaching a patch that fixes the problem. I was developed on top of a slightly modified 3.18.3, but the changes there should not affect this. From the comments here this probably also affects the 3.14 and 3.16 branches, but it looks like this code was removed on 3.20.

Let me know if any extra info is needed.

Created attachment 327049 [details] [review]
mouse: Change disable while typing before state

(In reply to João Paulo Rechi Vita from comment #22)
> This change affects the "toggle touchpad" media key on the Lenovo Yoga 900.
> The keypress event is sent through the legacy keyboard device, and syndaemon
> interprets it as typing, so it changes the "Synaptics Off" property back to
> 0 after 1s (checked with xinput watch-props). Simply changing the order
> between changing the "disable while typing" setting and the "Synaptics Off"
> property makes syndaemon die earlier and not interfere with the "toggle
> touchpad" media key.
> 
> I'm attaching a patch that fixes the problem. I was developed on top of a
> slightly modified 3.18.3, but the changes there should not affect this. From
> the comments here this probably also affects the 3.14 and 3.16 branches, but
> it looks like this code was removed on 3.20.
> 
> Let me know if any extra info is needed.

That code doesn't exist anymore, the code above was only used if you didn't use the libinput Xorg driver. File a new bug if you're still interested in that, but it's likely not going to change.

I understand that it does not exist on master anymore, I just commented and attached a patch here because I thought you would be interested in picking this fix for the stable branches affected by this problem (we'll carry this downstrean at Endless anyway). If you are interested, I'm happy to file a separate bug (I guess you want the version to specify 3.18, right?). Please let me know, as your previous comment suggests the opposite. Also, I apologize if this was reported on the wrong place, I'm not very familiar with GNOME's contribution process.

(In reply to João Paulo Rechi Vita from comment #25)
> I understand that it does not exist on master anymore, I just commented and
> attached a patch here because I thought you would be interested in picking
> this fix for the stable branches affected by this problem (we'll carry this
> downstrean at Endless anyway).

I'd advise you to remove the synaptics driver and use the libinput Xorg driver instead. This is already what was recommended on Linux, and the synaptics code was only there for non-Linux platforms.

> If you are interested, I'm happy to file a
> separate bug (I guess you want the version to specify 3.18, right?). Please
> let me know, as your previous comment suggests the opposite. Also, I
> apologize if this was reported on the wrong place, I'm not very familiar
> with GNOME's contribution process.

We prefer separate bugs, especially in this case where the bug was already fixed for 3.14 and later.