Created attachment 298650 [details] [review]
disable two finger scroll toggle appropriately

Thanks for the bug report, there is patch...

Comment on attachment 298650 [details] [review]
disable two finger scroll toggle appropriately

Thanks for the very quick patch! :)

Created attachment 299007 [details] [review]
fix checking capabilites for multiple devices

synaptics_check_capabilities_x11 doesn't work currently for multiple devices correctly, because the toggle is insensitive if e.g. first one supports two finger scrolling and second one doesn't support it, which is wrong.

Created attachment 299008 [details] [review]
check libinput scroll methods under X11

Created attachment 299009 [details] [review]
check libinput tapping for X11

Also tapping should be checked for libinput devices...

We should fix it also for wayland, but not sure how to do that. We are using GsdDeviceManager in the code, which use GdkDeviceManager. There is also some wayland specific code (gdk_wayland_device_get_wl_*), but it seems to me it isn't much useful for this case. We need to list libinput devices probably in similar way as it is done for xdevices, but don't know how (libinput api documentation didn't help me a lot)...

Review of attachment 299007 [details] [review]:

"synaptics_check_capabilities_x11 doesn't work currently for multiple devices correctly"
should be:
"synaptics_check_capabilities_x11 doesn't currently work correctly for multiple devices"

Looks good otherwise.

Review of attachment 299008 [details] [review]:

::: panels/mouse/gnome-mouse-properties.c
@@ +157,3 @@
+			/* Property data is booleans for two-finger, edge, on-button scroll available. */
+
+			/* Enable two finger scrolling toggle if both two-finger scroll and edge scroll is supported. */

It's either two-finger scroll *or* edge scroll, not both:
http://who-t.blogspot.fr/2015/03/why-libinput-doesnt-support-edge.html

Review of attachment 299009 [details] [review]:

Looks good.

(In reply to Bastien Nocera from comment #8)
> Review of attachment 299008 [details] [review] [review]:
> 
> ::: panels/mouse/gnome-mouse-properties.c
> @@ +157,3 @@
> +			/* Property data is booleans for two-finger, edge, on-button scroll
> available. */
> +
> +			/* Enable two finger scrolling toggle if both two-finger scroll and edge
> scroll is supported. */
> 
> It's either two-finger scroll *or* edge scroll, not both:
> http://who-t.blogspot.fr/2015/03/why-libinput-doesnt-support-edge.html

"So our decision was to only provide edge scrolling on touchpads where it is required, i.e. those that cannot support two-finger scrolling, those with physical buttons. All other touchpads provide only two-finger scrolling."

"short answer: we don't have edge scrolling because it conflicts quite badly with software buttons and palm detection. so we only provide it on non-clickpads, i.e. devices with physical buttons."
https://bugs.freedesktop.org/show_bug.cgi?id=89381

So it seems touchpads with physical buttons can support both methods and it is only case when two_finger_scroll_toggle should be sensitive, isn't it?

However it is slightly complicated to set toggle activity if we accept the fact that one can have more then one touchpad (attachment 299007 [details] [review]) - what about inconsistent state if toggle is insensitive (to avoid creating some other variables)?

(In reply to Ondrej Holy from comment #10)
<snip>
> "So our decision was to only provide edge scrolling on touchpads where it is
> required, i.e. those that cannot support two-finger scrolling, those with
> physical buttons. All other touchpads provide only two-finger scrolling."
> 
> "short answer: we don't have edge scrolling because it conflicts quite badly
> with software buttons and palm detection. so we only provide it on
> non-clickpads, i.e. devices with physical buttons."
> https://bugs.freedesktop.org/show_bug.cgi?id=89381
> 
> So it seems touchpads with physical buttons can support both methods and it
> is only case when two_finger_scroll_toggle should be sensitive, isn't it?
> 
> However it is slightly complicated to set toggle activity if we accept the
> fact that one can have more then one touchpad (attachment 299007 [details] [review]
> [review]) - what about inconsistent state if toggle is insensitive (to avoid
> creating some other variables)?

We should show the configuration when at least one of the touchpads supports it, and we'd only apply the configuration to the touchpads that support that.

Created attachment 299107 [details] [review]
check libinput scroll methods under X11

Created attachment 299108 [details] [review]
check libinput tapping for X11

Rebased and added code to set toggle to be inactive if it isn't supported...

Review of attachment 299107 [details] [review]:

> mouse: check libinput scroll methods under X11

This isn't the right commit subject. A better one would be "Fix sensitivity of two-finger toggle with libinput"

> aproprietly

appropriately

> toggle activity

Hmm? "toggle sensitivity", you set it to what as well?

::: panels/mouse/gnome-mouse-properties.c
@@ +126,3 @@
 	devicelist = XListInputDevices (GDK_DISPLAY_XDISPLAY (gdk_display_get_default ()), &numdevices);
 	for (i = 0; i < numdevices; i++) {
+		if (!device_info_is_touchpad (&(devicelist[i])))

This should probably be in a separate commit.

@@ +150,3 @@
 				two_finger_scroll = TRUE;
 
+			/* All synaptics devices should support edge scrolling. */

I would do as with the libinput backend, only enable edge_scroll if two_finger_scroll isn't supported.

In any case, this needs syncing with gnome-settings-daemon's behaviour (eg. not set edge scroll if two_finger_scroll is supported).

@@ +179,3 @@
+	widget = WID ("two_finger_scroll_toggle");
+	gtk_widget_set_sensitive (widget, two_finger_scroll && edge_scroll);
+	if ((!two_finger_scroll && edge_scroll) || (two_finger_scroll && !edge_scroll))

if (two_finger_scroll != edge_scroll) ?

Review of attachment 299108 [details] [review]:

> mouse: check libinput tapping for X11

How about
mouse: Fix tap configuration availability with libinput
?

::: panels/mouse/gnome-mouse-properties.c
@@ +189,3 @@
+	gtk_widget_set_sensitive (widget, tap_to_click);
+	if (!tap_to_click)
+		gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), FALSE);

Why not:
gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), tap_to_click);
?

(In reply to Bastien Nocera from comment #15)
> Review of attachment 299108 [details] [review] [review]:
> 
> > mouse: check libinput tapping for X11
> 
> How about
> mouse: Fix tap configuration availability with libinput
> ?

ok

> ::: panels/mouse/gnome-mouse-properties.c
> @@ +189,3 @@
> +	gtk_widget_set_sensitive (widget, tap_to_click);
> +	if (!tap_to_click)
> +		gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), FALSE);
> 
> Why not:
> gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), tap_to_click);
> ?

Because this code only checks if tapping is supported by hardware (but tapping can be enabled, or disabled). So we can set active = FALSE if tap_to_click == FALSE, because there isn't any device which supports tapping. However we can't set active = TRUE if tap_to_click == TRUE, because we only know that there are devices which supports tapping, but it depends on user preference if the toggle is active or not (if tapping is enabled, or disabled). So if tapping is supported, the toggle state comes from:
g_settings_bind (d->touchpad_settings, "tap-to-click", WID ("tap_to_click_toggle"), "active", G_SETTINGS_BIND_DEFAULT);

(In reply to Bastien Nocera from comment #14)
> Review of attachment 299107 [details] [review] [review]:
> 
> > mouse: check libinput scroll methods under X11
> 
> This isn't the right commit subject. A better one would be "Fix sensitivity
> of two-finger toggle with libinput"

ok

> > aproprietly
> 
> appropriately

ok

> > toggle activity
> 
> Hmm? "toggle sensitivity", you set it to what as well?

With toggle activity I mean:
gtk_toggle_button_set_active 

> ::: panels/mouse/gnome-mouse-properties.c
> @@ +126,3 @@
>  	devicelist = XListInputDevices (GDK_DISPLAY_XDISPLAY
> (gdk_display_get_default ()), &numdevices);
>  	for (i = 0; i < numdevices; i++) {
> +		if (!device_info_is_touchpad (&(devicelist[i])))
> 
> This should probably be in a separate commit.

Ok if you want, but this isn't necessary for existing code, because only touchpads have "Synaptics Capabilities" property. We need this for the libinput patches, because the libinput properties are common also for other devices...

> @@ +150,3 @@
>  				two_finger_scroll = TRUE;
>  
> +			/* All synaptics devices should support edge scrolling. */
> 
> I would do as with the libinput backend, only enable edge_scroll if
> two_finger_scroll isn't supported.

Not sure if your comment belongs to this statement. This statement isn't executed if libinput is used, because "Synaptics Capabilities" doesn't exists for libinput devices. So edge_scroll is not set to TRUE by default for libinput devices...

Also edge_scroll = TRUE doesn't mean in this context that we enable edge scrolling, it just means that edge scrolling is supported by hardware.

What do you mean with libinput backend? libinput code in mutter? libinput library? I don't see such code you mentioned anywhere... But the code bellow does what you says:

if (two_finger_scroll != edge_scroll)
gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), two_finger_scroll);

Enable edge scrolling if two finger isn't supported. It also enables two finger scrolling if edge isn't supported, but this is also valid, don't you think? 

> In any case, this needs syncing with gnome-settings-daemon's behaviour (eg.
> not set edge scroll if two_finger_scroll is supported).

After reading your comment I realized that we shouldn't use gtk_toggle_button_set_active at all probably in g-c-c and let g-s-d/mutter to set the value itself... We should just set gtk_toggle_button_set_sensitivity if particular features are not available. Do you agree? 

> @@ +179,3 @@
> +	widget = WID ("two_finger_scroll_toggle");
> +	gtk_widget_set_sensitive (widget, two_finger_scroll && edge_scroll);
> +	if ((!two_finger_scroll && edge_scroll) || (two_finger_scroll &&
> !edge_scroll))
> 
> if (two_finger_scroll != edge_scroll) ?

ok :-D

Comment on attachment 299108 [details] [review]
check libinput tapping for X11

Sorry for the confusion, this tap-to-click patch is totally odd, finally I understand why you gave such questions in your review for this patch...

However the current tap-to-click code is already odd, because following code was added by commit c34734dd87debd2428e2d3208bf18c308a5c7352 with comment "Touchpads that don't have physical buttons mustn't disable tapping - set to
on by default and disable checkbox.":

+			if (!data[0] || !data[1] || !data[2]) {
+				gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (WID ("tap_to_click_toggle")), TRUE);
+				gtk_widget_set_sensitive (WID ("tap_to_click_toggle"), FALSE);
+			}

And data[0] doesn't really tell anything about tapping, but about availability of hardware buttons. Unfortunately the line to set toggle activity was removed later by commit c34734dd87debd2428e2d3208bf18c308a5c7352. The default tap-to-click value is FALSE in the schema, so the code is avoiding to enable tap-to-click for touchpads without hardware buttons (which is totally opposite behavioral). 

So I think it would be best to remove this tap-to-click code at all since there weren't any bug reports about it though it was wrong over 4 years... Also I think that such code should be placed directly e.g. in g-s-d, or mutter if it is necessary, because we have to change the values immediately after the device is connected or user is logged in, and not after the dialog is opened...

(In reply to Ondrej Holy from comment #18)
> Comment on attachment 299108 [details] [review] [review]
> check libinput tapping for X11
> 
> Sorry for the confusion, this tap-to-click patch is totally odd, finally I
> understand why you gave such questions in your review for this patch...
> 
> However the current tap-to-click code is already odd, because following code
> was added by commit c34734dd87debd2428e2d3208bf18c308a5c7352 with comment
> "Touchpads that don't have physical buttons mustn't disable tapping - set to
> on by default and disable checkbox.":
> 
> +			if (!data[0] || !data[1] || !data[2]) {
> +				gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (WID
> ("tap_to_click_toggle")), TRUE);
> +				gtk_widget_set_sensitive (WID ("tap_to_click_toggle"), FALSE);
> +			}
> 
> And data[0] doesn't really tell anything about tapping, but about
> availability of hardware buttons. Unfortunately the line to set toggle
> activity was removed later by commit
> c34734dd87debd2428e2d3208bf18c308a5c7352. The default tap-to-click value is
> FALSE in the schema, so the code is avoiding to enable tap-to-click for
> touchpads without hardware buttons (which is totally opposite behavioral). 
> 
> So I think it would be best to remove this tap-to-click code at all since
> there weren't any bug reports about it though it was wrong over 4 years...
> Also I think that such code should be placed directly e.g. in g-s-d, or
> mutter if it is necessary, because we have to change the values immediately
> after the device is connected or user is logged in, and not after the dialog
> is opened...

Right now, we're looking at fixing this problem for yesterday, the 3.16.1 release. Any other discussion is out of scope for now, and we'll revisit the wisdom of this code when we're cleaning up this code for 3.18.

We're not discussing removing a settings in the middle of a stable release.

Created attachment 301603 [details] [review]
mouse: Allow enabling tap-to-click if there aren't hw buttons

Created attachment 301604 [details] [review]
mouse: Fix checking capabilities for multiple devices

Created attachment 301605 [details] [review]
mouse: Check synaptics capabilities only for touchpad

Created attachment 301606 [details] [review]
mouse: Fix sensitivity of two-finger toggle with libinput

Review of attachment 301603 [details] [review]:

This isn't quite correct, data[0] exposes whether there are physical buttons, and in commit a5bf115175ea52c5cdc184b3844d06bdb7ad775c we also enabled that button's value if the touchpad didn't have a physical button.

We should still disable the checkbox if none of the touchpads have physical buttons (in g-c-c), and always enable it (in g-s-d) if the touchpad doesn't have physical buttons.

Review of attachment 301604 [details] [review]:

Please link to the gnome-settings-daemon code that forces it on if the device only supports edge-scrolling.

Review of attachment 301605 [details] [review]:

Does it actually cause problems that it does? This seems invasive to add to 3.16.x

Review of attachment 301606 [details] [review]:

> Many libinput devices doesn't

"don't", it's plural.

> see http://who-t.blogspot.cz/2015/03/why-libinput-doesnt-support-edge.html.

Remove the "." there, it might cause parsers to add a broken link

Rest looks good.

Thanks for the reviews, following patches should fix your comments.

(In reply to Bastien Nocera from comment #26)
> Review of attachment 301605 [details] [review] [review]:
> 
> Does it actually cause problems that it does? This seems invasive to add to
> 3.16.x

You are right, this isn't necessary.

Created attachment 301620 [details] [review]
mouse: Fix checking capabilities for multiple devices

Created attachment 301621 [details] [review]
mouse: Fix sensitivity of two-finger toggle with libinput

There is one new problem, that tap_to_click_toggle isn't sensitive with libinput. Trying to find a way, how to detect hw buttons also with libinput if possible...

Review of attachment 301620 [details] [review]:

Looks good otherwise.

::: panels/mouse/gnome-mouse-properties.c
@@ +142,3 @@
+				tap_to_click = TRUE;
+
+			if (data[3])

You shouldn't remove the comment, and probably should add one for the check above.

Review of attachment 301621 [details] [review]:

Looks good.

Created attachment 301631 [details] [review]
mouse: Fix tap-to-click toggle sensitivity with libinput

Allow changing tap-to-click with libinput if "libinput Tapping Enabled" property exists. Seems there isn't any property to detect buttons as for X11. Peter doesn't responding to me on irc currently.

Review of attachment 301631 [details] [review]:

Sure.

Created attachment 301632 [details] [review]
mouse: Fix scroll methods settings for multiple devices (g-s-d)

Created attachment 301633 [details] [review]
mouse: Enable tap-to-click on devices without hw buttons (g-s-d)

Review of attachment 301632 [details] [review]:

Don't use dconf in the message, we never use dconf, we use GSettings (which use dconf as a backend).

"value" <- "stored configuration value"

Otherwise we're not really sure which value you're talking about.

Looks fine otherwise.

Comment on attachment 301620 [details] [review]
mouse: Fix checking capabilities for multiple devices

commit 2f8453438825d6862e08025446c10a2a4485362f

Comment on attachment 301621 [details] [review]
mouse: Fix sensitivity of two-finger toggle with libinput

commit 155d440623c1af67f3594c7508d9b7f44c83dae6

Comment on attachment 301631 [details] [review]
mouse: Fix tap-to-click toggle sensitivity with libinput

commit b1f3f3e458ccbafd9254185afb2ecaf1ce220686

Review of attachment 301633 [details] [review]:

Looks good.

::: plugins/mouse/gsd-mouse-manager.c
@@ +634,3 @@
+        if (rc == Success && type == XA_INTEGER && format == 8 && nitems >= 1) {
+                if (!(data[0])) {
+                        g_warning ("No hardware buttons, enabling tap to click on %s", gdk_device_get_name (device));

g_debug(), it's not a programming, or OS setup error not to have hardware buttons.

Comment on attachment 301632 [details] [review]
mouse: Fix scroll methods settings for multiple devices (g-s-d)

commit 73f70e19c2315fb1d6db3bfec52613c20c2ab0b8

Comment on attachment 301633 [details] [review]
mouse: Enable tap-to-click on devices without hw buttons (g-s-d)

commit 0f41e196509c7c30f48516fa00e3396db565d5e8

Thanks for the reviews, all patches pushed with applied comments.

I will check potential code changes for mutter to apply settings for multiple devices and will file bug for mutter if necessary.

Also I will open another bug for g-c-c to support this configuration also under wayland.

(In reply to Ondrej Holy from comment #45)
> Thanks for the reviews, all patches pushed with applied comments.
> 
> I will check potential code changes for mutter to apply settings for
> multiple devices and will file bug for mutter if necessary.

Please mention in this bug whether that's already working, or post the new bug ID here, thanks.

> Also I will open another bug for g-c-c to support this configuration also
> under wayland.

Yeah, we need a way to enumerate the devices and their capabilities under wayland. Mutter, can probably export a couple of variables to tell us which sections and options to show.

(In reply to Bastien Nocera from comment #46)
> (In reply to Ondrej Holy from comment #45)
> > Thanks for the reviews, all patches pushed with applied comments.
> > 
> > I will check potential code changes for mutter to apply settings for
> > multiple devices and will file bug for mutter if necessary.
> 
> Please mention in this bug whether that's already working, or post the new
> bug ID here, thanks.

See Bug 747931 for mutter.

(In reply to Bastien Nocera from comment #46)
> (In reply to Ondrej Holy from comment #45)
> > Also I will open another bug for g-c-c to support this configuration also
> > under wayland.
> 
> Yeah, we need a way to enumerate the devices and their capabilities under
> wayland. Mutter, can probably export a couple of variables to tell us which
> sections and options to show.

Bug 748031