GNOME Bugzilla – Bug 771576
Triple click on title bar is needed to maximize xWayland apps window when using touchpad
Last modified: 2017-03-24 19:51:42 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".
Created attachment 348359 [details] [review] clutter: Update double click timeout on evdev
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.
Can't we just put this in mutter? We now have three instead of two places we do input settings.
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.
(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 ().
Created attachment 348393 [details] [review] clutter: Update double click timeout on evdev
(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.
(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.
Created attachment 348418 [details] [review] input-settings: Set double click timeout from gsettings
(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 :)
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.
(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.
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?
(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.
(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.
(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.
Created attachment 348596 [details] [review] input-settings: Set double click timeout from gsettings
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.
Review of attachment 348596 [details] [review]: I hereby confirm that the patch uploaded in Comment 18 comes from me using a different account.
Review of attachment 348598 [details] [review]: Looks good to me.
The patch looks good indeed, pushed on behalf of Armin. Attachment 348598 [details] pushed as a77da35 - input-settings: Set double click timeout from gsettings