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 745121 - backend-native: Make sure the pointer stays inside the stage
backend-native: Make sure the pointer stays inside the stage
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks: 745752
 
 
Reported: 2015-02-24 20:44 UTC by Rui Matos
Modified: 2015-03-09 13:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backend-native: Make sure the pointer stays inside the stage (1.37 KB, patch)
2015-02-24 20:44 UTC, Rui Matos
rejected Details | Review
backend-native: Ensure the pointer is visible on monitors-changed (2.84 KB, patch)
2015-02-25 16:14 UTC, Rui Matos
none Details | Review
backend-native: Ensure the pointer is visible on monitors-changed (2.87 KB, patch)
2015-02-26 13:18 UTC, Rui Matos
none Details | Review
backend-native: Ensure the pointer is visible on monitors-changed (2.62 KB, patch)
2015-03-09 13:25 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2015-02-24 20:44:32 UTC
Noticed while turning off monitors in the Displays panel.
Comment 1 Rui Matos 2015-02-24 20:44:36 UTC
Created attachment 297825 [details] [review]
backend-native: Make sure the pointer stays inside the stage

Clutter would do this for us if we didn't install our own constrain
function so we need to do it ourselves.

This becomes important when the stage size changes and suddenly the
pointer is outside the area covered by the monitors.
Comment 2 Jonas Ådahl 2015-02-25 00:47:58 UTC
Review of attachment 297825 [details] [review]:

::: src/backends/native/meta-backend-native.c
@@ +155,3 @@
+  stage = clutter_input_device_get_pointer_stage (device);
+  *x = CLAMP (*x, 0.f, clutter_actor_get_width (CLUTTER_ACTOR (stage)) - 1);
+  *y = CLAMP (*y, 0.f, clutter_actor_get_height (CLUTTER_ACTOR (stage)) - 1);

Shouldn't the monitor clamping already take care of this? If I understand it correctly, the stage size is set given the monitor configuration, i.e. clamping to the monitor effectively clamps to the stage as well. Under what situation is the stage smaller than the monitor configuration?
Comment 3 Rui Matos 2015-02-25 16:14:13 UTC
Created attachment 297895 [details] [review]
backend-native: Ensure the pointer is visible on monitors-changed

Otherwise the pointer might be "lost" outside the visible area. Note
that the constraining code only ensures the pointer doesn't leave the
visible area but if the pointer is already outside because the rug was
pulled under it then it doesn't do anything.
--

(In reply to Jonas Ådahl from comment #2)
> Shouldn't the monitor clamping already take care of this? If I
> understand it correctly, the stage size is set given the monitor
> configuration, i.e.  clamping to the monitor effectively clamps to
> the stage as well. Under what situation is the stage smaller than
> the monitor configuration?

This happens when we change the stage size. E.g start from a 1920x1080
stage and then switch it to a 1024x768 stage while the pointer is
stationary at the bottom right i.e. 1919,1079 . On the next motion
event, after the stage size change, the clutter device still has the
old coordinate and thus the current constrain code doesn't do anything
since it only constrains the pointer to *remain* inside the the
monitor covered area but in this case we're already outside of it.

I'm attaching another approach which I like more since it doesn't add
more code running on every motion event and ends up ensuring that the
pointer is nicely visible on top of the modal confirmation dialog that
gnome-shell shows after any mode setting operation.
Comment 4 Jonas Ådahl 2015-02-26 00:33:34 UTC
Review of attachment 297895 [details] [review]:

::: src/backends/native/meta-backend-native.c
@@ +230,3 @@
+  monitor_manager = meta_backend_get_monitor_manager (backend);
+  g_signal_connect (monitor_manager, "monitors-changed",
+                    G_CALLBACK (on_monitors_changed), backend);

Any point of disconnecting the signal when we terminate?
Comment 5 Rui Matos 2015-02-26 13:05:38 UTC
(In reply to Jonas Ådahl from comment #4)
> Review of attachment 297895 [details] [review] [review]:
> 
> ::: src/backends/native/meta-backend-native.c
> @@ +230,3 @@
> +  monitor_manager = meta_backend_get_monitor_manager (backend);
> +  g_signal_connect (monitor_manager, "monitors-changed",
> +                    G_CALLBACK (on_monitors_changed), backend);
> 
> Any point of disconnecting the signal when we terminate?

We could but the MetaBackend singleton is never really finalized and there's currently no reason to do so.
Comment 6 Rui Matos 2015-02-26 13:18:17 UTC
Created attachment 297975 [details] [review]
backend-native: Ensure the pointer is visible on monitors-changed

--

But yeah, let's do it properly with connect_object
Comment 7 Jonas Ådahl 2015-03-09 03:03:36 UTC
Review of attachment 297975 [details] [review]:

Looks good to me.
Comment 8 Marek Chalupa 2015-03-09 11:40:02 UTC
Review of attachment 297975 [details] [review]:

Otherwise looks good.

::: src/backends/native/meta-backend-native.c
@@ +193,3 @@
+    return;
+
+  monitor_manager = meta_monitor_manager_get ();

This is no-op statement, the monitor_manager is already set as a parametr

@@ +201,3 @@
+
+  /* warp the pointer to the primary monitor so it isn't lost */
+  for (i = 0; i < n_monitors; i++)

Wouldn't it be better to use meta_monitor_manager_get_primary_index() and thus get the primary index in constant time instead of looping over the monitors?
Comment 9 Rui Matos 2015-03-09 13:25:54 UTC
Created attachment 298877 [details] [review]
backend-native: Ensure the pointer is visible on monitors-changed

--

(In reply to Marek Chalupa from comment #8)
> ::: src/backends/native/meta-backend-native.c
> @@ +193,3 @@
> +    return;
> +
> +  monitor_manager = meta_monitor_manager_get ();
>
> This is no-op statement, the monitor_manager is already set as a
> parametr

Duh, of course

> @@ +201,3 @@
> +
> +  /* warp the pointer to the primary monitor so it isn't lost */
> +  for (i = 0; i < n_monitors; i++)

> Wouldn't it be better to use
> meta_monitor_manager_get_primary_index() and thus get the primary
> index in constant time instead of looping over the monitors?

Yeah, that's cleaner.
Comment 10 Rui Matos 2015-03-09 13:31:03 UTC
Attachment 298877 [details] pushed as 9a3b178 - backend-native: Ensure the pointer is visible on monitors-changed