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 771576 - Triple click on title bar is needed to maximize xWayland apps window when using touchpad
Triple click on title bar is needed to maximize xWayland apps window when usi...
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
3.21.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2016-09-17 06:11 UTC by Strangiato
Modified: 2017-03-24 19:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
clutter: Update double click timeout on evdev (3.83 KB, patch)
2017-03-20 20:20 UTC, Armin K.
none Details | Review
clutter: Update double click timeout on evdev (3.71 KB, patch)
2017-03-21 11:02 UTC, Armin K.
none Details | Review
input-settings: Set double click timeout from gsettings (4.36 KB, patch)
2017-03-21 14:59 UTC, Armin K.
none Details | Review
input-settings: Set double click timeout from gsettings (2.70 KB, patch)
2017-03-23 18:08 UTC, Armin K.
rejected Details | Review
input-settings: Set double click timeout from gsettings (2.70 KB, patch)
2017-03-23 18:12 UTC, Armin Krezović
committed Details | Review

Description Strangiato 2016-09-17 06:11:44 UTC
I an running 3.22 beta 2 on Arch.

steps to reproduce
open any gtk2/qt4/qt5 app
use your touchpad to do double click (use "tap to click" instead left/right touchpad buttons) on title bar to maximize the window
nothing happens, I need do triple click using "tap to click" to maximize the window.
I can maximixe the window with double click when I use touchpad left button.

Under X11 I can maximize the window with double click using "tap to click".
Comment 1 Armin K. 2017-03-20 20:20:06 UTC
Created attachment 348359 [details] [review]
clutter: Update double click timeout on evdev
Comment 2 Carlos Garnacho 2017-03-20 23:23:19 UTC
Review of attachment 348359 [details] [review]:

Thanks for the fix! Looks good IMO, just minor nitpicks, I deem this accepted-commit-now after these are fixed.

::: clutter/clutter/evdev/clutter-device-manager-evdev.c
@@ +2044,3 @@
+    return;
+
+  g_object_freeze_notify (G_OBJECT (settings));

No need to freeze/thaw, we're just modifying one property and just once, we can't coalesce that any further.

@@ +2207,3 @@
 
+  if (priv->gsd_settings)
+    g_object_unref (priv->gsd_settings);

we can just use g_clear_object() here.
Comment 3 Jonas Ådahl 2017-03-21 00:02:18 UTC
Can't we just put this in mutter? We now have three instead of two places we do input settings.
Comment 4 Jonas Ådahl 2017-03-21 00:04:57 UTC
Sorry, with mutter I meant meta-input-settings*, and with three I mean four (clutter-settings-x11, meta-input-settings, meta-input-settings-x11, meta-input-settings-native), and I think we should move towards meta-input-settings* instead of introducing yet another place.
Comment 5 Armin K. 2017-03-21 10:49:16 UTC
(In reply to Jonas Ådahl from comment #4)
> Sorry, with mutter I meant meta-input-settings*, and with three I mean four
> (clutter-settings-x11, meta-input-settings, meta-input-settings-x11,
> meta-input-settings-native), and I think we should move towards
> meta-input-settings* instead of introducing yet another place.

Unfortunately, ClutterSettings is private to clutter. There's no public version of clutter_settings_set_property_internal ().
Comment 6 Armin K. 2017-03-21 11:02:51 UTC
Created attachment 348393 [details] [review]
clutter: Update double click timeout on evdev
Comment 7 Armin K. 2017-03-21 11:04:21 UTC
(In reply to Carlos Garnacho from comment #2)
> Review of attachment 348359 [details] [review] [review]:
> 
> Thanks for the fix! Looks good IMO, just minor nitpicks, I deem this
> accepted-commit-now after these are fixed.
> 
> ::: clutter/clutter/evdev/clutter-device-manager-evdev.c
> @@ +2044,3 @@
> +    return;
> +
> +  g_object_freeze_notify (G_OBJECT (settings));
> 
> No need to freeze/thaw, we're just modifying one property and just once, we
> can't coalesce that any further.
> 
> @@ +2207,3 @@
>  
> +  if (priv->gsd_settings)
> +    g_object_unref (priv->gsd_settings);
> 
> we can just use g_clear_object() here.

Thanks for the feedback. I've added an updated version to address outlined issues.
Comment 8 Jonas Ådahl 2017-03-21 13:51:45 UTC
(In reply to Armin K. from comment #5)
> (In reply to Jonas Ådahl from comment #4)
> > Sorry, with mutter I meant meta-input-settings*, and with three I mean four
> > (clutter-settings-x11, meta-input-settings, meta-input-settings-x11,
> > meta-input-settings-native), and I think we should move towards
> > meta-input-settings* instead of introducing yet another place.
> 
> Unfortunately, ClutterSettings is private to clutter. There's no public
> version of clutter_settings_set_property_internal ().

We have pretty much erased the border of private vs not between clutter and mutter, and clutter backends might even migrate from clutter into mutter to put backends in one place instead of spreading them out. For example half the clutter stage backend is already in MetaStage* and friends.

As for setting the property, a

ClutterSettings *clutter_settings = clutter_settings_get_default ();

g_object_set (G_OBJECT (clutter_settings), "double-click-timeout", timeout);

would already work from mutter.
Comment 9 Armin K. 2017-03-21 14:59:46 UTC
Created attachment 348418 [details] [review]
input-settings: Set double click timeout from gsettings
Comment 10 Armin K. 2017-03-21 15:00:16 UTC
(In reply to Jonas Ådahl from comment #8)
> (In reply to Armin K. from comment #5)
> > (In reply to Jonas Ådahl from comment #4)
> > > Sorry, with mutter I meant meta-input-settings*, and with three I mean four
> > > (clutter-settings-x11, meta-input-settings, meta-input-settings-x11,
> > > meta-input-settings-native), and I think we should move towards
> > > meta-input-settings* instead of introducing yet another place.
> > 
> > Unfortunately, ClutterSettings is private to clutter. There's no public
> > version of clutter_settings_set_property_internal ().
> 
> We have pretty much erased the border of private vs not between clutter and
> mutter, and clutter backends might even migrate from clutter into mutter to
> put backends in one place instead of spreading them out. For example half
> the clutter stage backend is already in MetaStage* and friends.
> 
> As for setting the property, a
> 
> ClutterSettings *clutter_settings = clutter_settings_get_default ();
> 
> g_object_set (G_OBJECT (clutter_settings), "double-click-timeout", timeout);
> 
> would already work from mutter.

Thanks for the g_object_set () hint. I'm still new to the GLib/GObject world, so I have a lot to learn :)
Comment 11 Jonas Ådahl 2017-03-22 02:25:32 UTC
Review of attachment 348418 [details] [review]:

Looks correct, comments are mostly about making things consistent with the other setting management code.

FWIW, this removes the listening on the XSetting for double-click timeout, which I believe is reasonable here because this version of clutter will not be used as an application toolkit, and mutter-as-a-compositor should just get its setting from one place. I'd like to get a second opinion on this though before landing.

::: src/backends/meta-input-settings.c
@@ +238,3 @@
+
+  if (!priv->gsd_settings)
+    return;

No need for this, without it we would already have crashed elsewhere.

@@ +1490,3 @@
+                        G_CALLBACK (meta_input_settings_changed_cb), settings);
+
+    }

This could be shortened to 

priv->gsd_settings = g_settings_new ("org.gnome.settings-daemon.peripherals.mouse");
g_signal_connect (priv->gsd_settings, "changed",
                  G_CALLBACK (meta_input_settings_changed_cb), settings);

as is done with the other settings schemas.

@@ +1492,3 @@
+    }
+
+  update_double_click_timeout (settings);

Move this to meta_input_settings_constructed() with the other equivalent calls.
Comment 12 Armin K. 2017-03-22 09:42:51 UTC
(In reply to Jonas Ådahl from comment #11)
> Review of attachment 348418 [details] [review] [review]:
> 
> Looks correct, comments are mostly about making things consistent with the
> other setting management code.
> 

Speaking of consistency, the code was written to be consistent with:

https://git.gnome.org/browse/mutter/tree/src/wayland/meta-wayland-keyboard.c#n610

I suppose whoever wrote the code above wanted to avoid hard runtime dep on g-s-d. I'd like to do the same.

However, if you still insist on consistency, I suppose it should be made consistent everywhere.
Comment 13 Florian Müllner 2017-03-22 09:56:27 UTC
Review of attachment 348418 [details] [review]:

::: src/backends/meta-input-settings.c
@@ +1488,3 @@
+      g_settings_schema_unref (schema);
+      g_signal_connect (priv->gsd_settings, "changed",
+                        G_CALLBACK (meta_input_settings_changed_cb), settings);

Can't we use

  g_settings_bind (priv->gsd_settings, "double-click",
                   clutter_settings_get_default(), "double-click-time",
                   G_SETTINGS_BIND_GET);

and do without update_double_click_timeout() altogether?
Comment 14 Jonas Ådahl 2017-03-23 05:58:49 UTC
(In reply to Florian Müllner from comment #13)
> Review of attachment 348418 [details] [review] [review]:
> 
> ::: src/backends/meta-input-settings.c
> @@ +1488,3 @@
> +      g_settings_schema_unref (schema);
> +      g_signal_connect (priv->gsd_settings, "changed",
> +                        G_CALLBACK (meta_input_settings_changed_cb),
> settings);
> 
> Can't we use
> 
>   g_settings_bind (priv->gsd_settings, "double-click",
>                    clutter_settings_get_default(), "double-click-time",
>                    G_SETTINGS_BIND_GET);
> 
> and do without update_double_click_timeout() altogether?

Even better.
Comment 15 Jonas Ådahl 2017-03-23 06:36:17 UTC
(In reply to Armin K. from comment #12)
> (In reply to Jonas Ådahl from comment #11)
> > Review of attachment 348418 [details] [review] [review] [review]:
> > 
> > Looks correct, comments are mostly about making things consistent with the
> > other setting management code.
> > 
> 
> Speaking of consistency, the code was written to be consistent with:
> 
> https://git.gnome.org/browse/mutter/tree/src/wayland/meta-wayland-keyboard.
> c#n610
> 
> I suppose whoever wrote the code above wanted to avoid hard runtime dep on
> g-s-d. I'd like to do the same.
> 
> However, if you still insist on consistency, I suppose it should be made
> consistent everywhere.

Yes, please make things consistent, especially within the same file.
Comment 16 Armin K. 2017-03-23 18:08:00 UTC
(In reply to Florian Müllner from comment #13)
> Review of attachment 348418 [details] [review] [review]:
> 
> ::: src/backends/meta-input-settings.c
> @@ +1488,3 @@
> +      g_settings_schema_unref (schema);
> +      g_signal_connect (priv->gsd_settings, "changed",
> +                        G_CALLBACK (meta_input_settings_changed_cb),
> settings);
> 
> Can't we use
> 
>   g_settings_bind (priv->gsd_settings, "double-click",
>                    clutter_settings_get_default(), "double-click-time",
>                    G_SETTINGS_BIND_GET);
> 
> and do without update_double_click_timeout() altogether?

Whoa, this is awesome! Thank you for the hint.
Comment 17 Armin K. 2017-03-23 18:08:19 UTC
Created attachment 348596 [details] [review]
input-settings: Set double click timeout from gsettings
Comment 18 Armin Krezović 2017-03-23 18:12:12 UTC
Created attachment 348598 [details] [review]
input-settings: Set double click timeout from gsettings

Reuploading again with the correct name and e-mail address, which will be used for GSoC.
Comment 19 Armin K. 2017-03-23 18:14:34 UTC
Review of attachment 348596 [details] [review]:

I hereby confirm that the patch uploaded in Comment 18 comes from me using a different account.
Comment 20 Jonas Ådahl 2017-03-24 14:27:56 UTC
Review of attachment 348598 [details] [review]:

Looks good to me.
Comment 21 Carlos Garnacho 2017-03-24 19:51:37 UTC
The patch looks good indeed, pushed on behalf of Armin.

Attachment 348598 [details] pushed as a77da35 - input-settings: Set double click timeout from gsettings