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.

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?

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.

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?

(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.

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

Review of attachment 297975 [details] [review]:

Looks good to me.

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?

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.

Attachment 298877 [details] pushed as 9a3b178 - backend-native: Ensure the pointer is visible on monitors-changed