GNOME Bugzilla – Bug 712775
take over cursor visibility handling from gsd cursor plugin
Last modified: 2015-03-13 20:07:34 UTC
The cursor plugin in gnome-settings-daemon is currently still doing direct XFixes calls to hide or show the mouse pointer when the device goes idle. This functionality should probably just be done in mutter - after all, it is already keeping track of idletimes... This is also needed to make the functionality transparently available on both X and Wayland.
See also bug 709475. I guess mutter could take over from g-s-d's cursor plugin to track the last used device and gnome-shell would hide the cursor/enable the OSK if it's a touchscreen.
I'm attaching some patches that take care of cursor visibility. As for OSK, I'm a bit more undecided at the moment. It is entirely non functional on wayland yet, so it might be a bit too soon to show it there. OTOH, a temporary check might be added on gnome-shell so it's only displayed on X11, and that would allow us to remove the cursor plugin from g-s-d entirely.
Created attachment 296618 [details] [review] core: Add meta_display_update_current_pointer() This function can be used to trigger changes depending on the device type that is currently emitting the events. So far, it is used to switch cursor visibility on/off on touchscreen interaction. When the cursor is updated, this is emitted through MetaDisplay:pointing-device-updated.
Created attachment 296619 [details] [review] core: Update cursor visibility on display events
Created attachment 296620 [details] [review] backends/x11: Call meta_display_update_last_pointer() from XI_DeviceChanged On X11, calling this function on meta_display_handle_events() will not catch mouse events happening over clients, so poke directly in the backend for XI_DeviceChanged events, which mutter will get on device switches.
Created attachment 296621 [details] [review] core: Update cursor visibility based on device availability This is done on startup, and anytime the last interacted pointing device is removed. Devices being added don't update cursor visibility, we wait for the user interacting with those instead.
Review of attachment 296618 [details] [review]: My first gut feeling is that this should be in MetaBackend, not MetaDisplay, but I'm not so sure. I think it should be. ::: src/core/display.c @@ +133,3 @@ + PROP_FOCUS_WINDOW, + PROP_POINTER You don't ever add / use this property? Was it an attempt to use notify::pointer instead of the signal?
Review of attachment 296619 [details] [review]: Looks good.
Review of attachment 296620 [details] [review]: ::: src/backends/x11/meta-backend-x11.c @@ +255,3 @@ + if (event->type == GenericEvent && + event->xcookie.extension == priv->xinput_opcode) + handle_device_change (x11, (XIEvent *) event->xcookie.data); I think this should be handle_input_event, which optionally dispatches to handle_device_change or maybe_spoof_event_as_stage_event.
Review of attachment 296621 [details] [review]: ::: src/core/display.c @@ +560,3 @@ + devices = clutter_device_manager_peek_devices (device_manager); + + while (devices) for (l = devices; l; l = l->next) @@ +566,3 @@ + + devices = devices->next; + type = clutter_input_device_get_device_type (device); You an just put this in one line, below the get_device_mode: ClutterInputDeviceType type = clutter_input_device_get_device_type (device);
(In reply to Jasper St. Pierre from comment #7) > Review of attachment 296618 [details] [review] [review]: > > My first gut feeling is that this should be in MetaBackend, not MetaDisplay, > but I'm not so sure. I think it should be. I mainly went this way because of https://bugzilla.gnome.org/show_bug.cgi?id=733631#c19 :). It is true that we're anyway forced to check for this in backend-dependent ways anyway. > > ::: src/core/display.c > @@ +133,3 @@ > > + PROP_FOCUS_WINDOW, > + PROP_POINTER > > You don't ever add / use this property? Was it an attempt to use > notify::pointer instead of the signal? Oops, I at first thought about making the last device a property and notifying about it, that's a remaining of this. (In reply to Jasper St. Pierre from comment #9) > Review of attachment 296620 [details] [review] [review]: > > ::: src/backends/x11/meta-backend-x11.c > @@ +255,3 @@ > + if (event->type == GenericEvent && > + event->xcookie.extension == priv->xinput_opcode) > + handle_device_change (x11, (XIEvent *) event->xcookie.data); > > I think this should be handle_input_event, which optionally dispatches to > handle_device_change or maybe_spoof_event_as_stage_event. I mainly kept this separate as I'm not sure should be affected by bypass_clutter, might be unlikely for XI_DeviceChanged to be caught though.
Created attachment 298998 [details] [review] backend: Add meta_backend_update_last_device() This function can be used to trigger changes depending on the device type that is currently emitting the events. So far, it is used to switch cursor visibility on/off on touchscreen interaction.
Created attachment 298999 [details] [review] core: Update cursor visibility on display events
Created attachment 299000 [details] [review] backends/x11: Call meta_backend_update_last_device() from XI_DeviceChanged On X11, calling this function on meta_display_handle_events() will not catch mouse events happening over clients, so poke directly in the backend for XI_DeviceChanged events, which mutter will get on device switches. The code has been slightly refactored so we deal with XIEvents at a single handle_input_event() function.
Created attachment 299001 [details] [review] backend: Update cursor visibility based on device availability This is done on startup, and anytime the last interacted pointing device is removed. Devices being added don't update cursor visibility, we wait for the user interacting with those instead.
Review of attachment 298998 [details] [review]: ::: src/backends/meta-backend.c @@ +420,3 @@ + { + case CLUTTER_POINTER_DEVICE: + case CLUTTER_TOUCHPAD_DEVICE: That's different from what g-s-d does. g-s-d hides the cursor only when the last used device is a touchscreen. so it should be: meta_cursor_tracker_set_pointer_visible (cursor_tracker, device_type != CLUTTER_TOUCHSCREEN_DEVICE); Note that you're hiding the cursor when using a tablet, or a joystick, which is probably unwanted.
Review of attachment 298998 [details] [review]: ::: src/backends/meta-backend-private.h @@ +108,3 @@ struct xkb_keymap * meta_backend_get_keymap (MetaBackend *backend); +void meta_backend_update_last_device (MetaBackend *backend, I would also call this "last_pointer_device", so that we could create a new property for when to show/hide the OSK. If the last used device is a keyboard, you'd want to hide the OSK.
Review of attachment 299001 [details] [review]: The old behaviour was that the pointer was hidden until the mouse was moved. So to make this better, you should check whether there's a touchscreen, and hide the cursor only in this case. But you don't want to show a cursor when there's a touchscreen, even when there's a mouse.
Created attachment 299018 [details] [review] backend: Update cursor visibility based on device availability On startup, the cursor is kept hidden if there's any touchscreen available. If the device that was last interacted is removed, we check on available mice though, so we don't possibly hide the pointer if there are further connected mice. Devices being added don't update cursor visibility, we wait for the user interacting with those instead.
Review of attachment 298998 [details] [review]: ::: src/backends/meta-backend-private.h @@ +108,3 @@ struct xkb_keymap * meta_backend_get_keymap (MetaBackend *backend); +void meta_backend_update_last_device (MetaBackend *backend, Maybe this function name is good enough though, never mind.
Created attachment 299022 [details] [review] backend: Add meta_backend_update_last_device() This function can be used to trigger changes depending on the device type that is currently emitting the events. So far, it is used to switch cursor visibility on/off on touchscreen interaction. A "last-device-updated" signal has also been added, in order allow hooking other behavior changes (eg. OSK) when the last device changes.
Created attachment 299210 [details] [review] backend: Add meta_backend_update_last_device() This function can be used to trigger changes depending on the device type that is currently emitting the events. So far, it is used to switch cursor visibility on/off on touchscreen interaction. A "last-device-updated" signal has also been added, in order allow hooking other behavior changes (eg. OSK) when the last device changes.
fwiw, i think the mouse pointer should be hidden any time the user goes idle or uses a non-mouse input device, and it should get shown any time the mouse gets moved. I mean, why would we want to keep showing it even if the user isn't using it? (i realize this is different behavior than gnome-settings-daemon)
Bastien, what's the advantage of showing the pointer so aggressively (other than maintaining status quo) ?
(In reply to Ray Strode [halfline] from comment #23) > fwiw, i think the mouse pointer should be hidden any time the user goes idle > or uses a non-mouse input device, and it should get shown any time the mouse > gets moved. I mean, why would we want to keep showing it even if the user > isn't using it? (i realize this is different behavior than > gnome-settings-daemon) One corner case I can think of is scrolling, if the pointer is hidden you can't see promptly where it applies, only after you started. You can of course wiggle the mouse before, but looks like something to grow accustomed to. Also, seems a bit hard to implement for x11 :(, at least without listening to XIRaw* events... otherwise you just don't know about motion events in the big holes that app windows are to mutter.
Isn't that what DEVICE IDLETIME alarms are for?
(In reply to Ray Strode [halfline] from comment #23) > fwiw, i think the mouse pointer should be hidden any time the user goes idle > or uses a non-mouse input device, and it should get shown any time the mouse > gets moved. I mean, why would we want to keep showing it even if the user > isn't using it? (i realize this is different behavior than > gnome-settings-daemon) What do you mean "when the user goes idle"? No, it should still be shown if the last used device is something other than a touchscreen. It should however be hidden when the fade to black from gnome-shell starts, but that's another bug.
In any case, I think we should start with implementing the same design as in gnome-settings-daemon, and enhance on top of that. I'm sure the designers would like to get involved in any of those changes.
(In reply to Carlos Garnacho from comment #25) > One corner case I can think of is scrolling, if the pointer is hidden you > can't see promptly where it applies, only after you started. You can of > course wiggle the mouse before, but looks like something to grow accustomed > to. Not sure I follow, in order to get to a document to scroll it you necessarily need to move the mouse right? oh, are you thinking alt-tab ? > Also, seems a bit hard to implement for x11 :(, at least without listening > to XIRaw* events... otherwise you just don't know about motion events in the > big holes that app windows are to mutter. We actually already can do it in X using XSync IDLETIME counters (which we already watch) like Jasper mentioned. > What do you mean "when the user goes idle"? I mean the pointer is a tool to help someone while they're using the mouse, if they aren't actively using the mouse, the pointer is just something that gets in the way. > No, it should still be shown if the last used device is something other > than a touchscreen. Why? > It should > however be hidden when the fade to black from gnome-shell starts, but that's > another bug. I'm arguing it should be hidden anytime it isn't being useful. (In reply to Bastien Nocera from comment #28) > In any case, I think we should start with implementing the same design as in > gnome-settings-daemon, and enhance on top of that. Fair enough, though, I think I argued the same thing I'm arguing now when gnome-settings-daemon initially gained the code: https://bugzilla.gnome.org/show_bug.cgi?id=666121#c4 > I'm sure the designers would like to get involved in any of those changes. Well, mccann agreed with me in 2012: "When I am using my computer with a mouse I expect there to be a mouse pointer visible. When I am not interacting with my computer with a mouse I don't expect there to be a pointer visible." https://bugzilla.gnome.org/show_bug.cgi?id=687791#c0 But would be good to talk to Allan and get his take.
(In reply to Ray Strode [halfline] from comment #29) > (In reply to Carlos Garnacho from comment #25) > > One corner case I can think of is scrolling, if the pointer is hidden you > > can't see promptly where it applies, only after you started. You can of > > course wiggle the mouse before, but looks like something to grow accustomed > > to. > Not sure I follow, in order to get to a document to scroll it you > necessarily need to move the mouse right? oh, are you thinking alt-tab ? I meant, you interact with the mouse, let's say you switch workspace to a fullscreen gnome-terminal, then the idle hides the pointer, and you're not sure right away anymore on the mouse position, and whether it's on the vte widget, or its on the notebook tabs and it will switch you away. > > > Also, seems a bit hard to implement for x11 :(, at least without listening > > to XIRaw* events... otherwise you just don't know about motion events in the > > big holes that app windows are to mutter. > We actually already can do it in X using XSync IDLETIME counters (which we > already watch) like Jasper mentioned. Right, that didn't cross my mind :)
(In reply to Carlos Garnacho from comment #30) > I meant, you interact with the mouse, let's say you switch workspace to a > fullscreen gnome-terminal, then the idle hides the pointer, and you're not > sure right away anymore on the mouse position, and whether it's on the vte > widget, or its on the notebook tabs and it will switch you away. Ah okay, I think I see what you're saying now. I feel like that situation's not really going to be a problem in practice. At least it's hard for me to imagine someone letting their session go idle, then later drawing attention back to the screen, carefully putting their hand back on the mouse (without moving it!) and then using scroll wheel, only then to think, "wait, is the mouse pointer where it's supposed to be?". Even so, feels like the inconvenience imposed in this case is much smaller than the net win of not having a pointer hanging around.
I'm with Bastien on this one: no need to have the pointer 'go idle' before the rest of the session.
(In reply to Ray Strode [halfline] from comment #29) <snip> > > I'm sure the designers would like to get involved in any of those changes. > Well, mccann agreed with me in 2012: > > "When I am using my computer with a mouse I expect there to be a mouse > pointer visible. When I am not interacting with my computer with a mouse I > don't expect there to be a pointer visible." > > https://bugzilla.gnome.org/show_bug.cgi?id=687791#c0 I'm pretty sure he meant: "When I'm interacting with my computer with a touchscreen I don't expect there to be a pointer visible" which I agree with. I'm fairly certain he didn't mean that if you use the keyboard instead of the mouse it should get hidden...
Review of attachment 299018 [details] [review]: ::: src/backends/meta-backend.c @@ +179,3 @@ destroy_device_monitor (backend, device_id); + + if (backend->current_device_id == device_id) This assumes that touchscreens aren't removable, right? Maybe add a comment about that @@ +183,3 @@ + MetaCursorTracker *cursor_tracker = meta_cursor_tracker_get_for_screen (NULL); + meta_cursor_tracker_set_pointer_visible (cursor_tracker, + check_has_mice (device_manager)); we should only set the pointer invisible if we have a touchscreen @@ +241,3 @@ + cursor_tracker = meta_cursor_tracker_get_for_screen (NULL); + meta_cursor_tracker_set_pointer_visible (cursor_tracker, has_touchscreen); !has_touchscreen ?
Review of attachment 299210 [details] [review]: good after typo fix ::: src/backends/meta-backend.c @@ +426,3 @@ + switch (device_type) + { + case CLUTTER_POINTER_TOUCHSCREEN: CLUTTER_TOUCHSCREEN_DEVICE ?
Review of attachment 298999 [details] [review]: ::: src/core/events.c @@ +181,3 @@ #endif + source = clutter_event_get_source_device (event); This might return NULL which will issue some warnings, so maybe protect against that here?
Review of attachment 299000 [details] [review]: lgtm
Created attachment 299352 [details] [review] backend: Update cursor visibility based on device availability On startup, the cursor is kept hidden if there's any touchscreen available. If the device that was last interacted is removed, we check on available mice though, so we don't possibly hide the pointer if there are further connected mice. Devices being added don't update cursor visibility, we wait for the user interacting with those instead.
Review of attachment 299352 [details] [review]: ::: src/backends/meta-backend.c @@ +176,3 @@ + { + if (clutter_input_device_get_device_mode (device) != CLUTTER_INPUT_MODE_MASTER && + clutter_input_device_get_device_type (device) == CLUTTER_TOUCHSCREEN_DEVICE) if (device_is_slave_touchscreen ()) ? @@ +212,3 @@ + { + meta_cursor_tracker_set_pointer_visible (cursor_tracker, + check_has_pointing_device (device_manager)); I still think this should be: check_has_pointing_device() || !check_has_trouchscreen()
All patches have been pushed after amending the last suggestions Attachment 298999 [details] pushed as a30ca3e - core: Update cursor visibility on display events Attachment 299000 [details] pushed as af9d8f1 - backends/x11: Call meta_backend_update_last_device() from XI_DeviceChanged Attachment 299210 [details] pushed as 9e3bac0 - backend: Add meta_backend_update_last_device() Attachment 299352 [details] pushed as 3471ef3 - backend: Update cursor visibility based on device availability