GNOME Bugzilla – Bug 759309
Add GdkSeat
Last modified: 2015-12-21 20:02:04 UTC
I'm attaching some patches that add a GdkSeat object to GDK. A GdkSeat represents a collection of devices belonging to a single user, in this aspect it differs with GdkDeviceManager in that the latter recollects all available master/slave devices, the relation between those being something secondary. You can conceptually have multiple seats each with its own pointer/keyboard focus, etc. The main operation moved to this object domain is grabs, the traditional way to perform grabs in gtk+ to first grab the pointer and then the keyboard doesn't completely scale if there's more foci than those (eg. touchscreen or tablets in wayland, those will have a standalone focus, and pointer cursor for the latter), yet we want the grab to allow interaction with all those while ignoring other seats. Also, gdk_seat_grab() allows for showing a window and grabbing "atomically", so we can do both operations in windowing dependent order. I've improved somewhat the current behavior on wayland though, so we print a big warning recommending gdk_seat_grab() and fallback to using the default seat, something similar could be done for the cases where we don't have a parent/transient_for window for the popup we're trying to show. Some caveats: - gdk_seat_grab() has 8 arguments, a bit too much. I was suggested on irc to make it a gdk_seat_grab_full() and having a simple one for the most common cases, but that would leave it with 6 arguments, so it wasn't a too big win. TBH I think we could cut down owner_events (default to TRUE), and recommend additionally doing a gtk+ grab if owner_events=FALSE behavior is desired. But that still leaves us with 7 vs 5 arguments. - I haven't implemented this yet for other backends than x11/wayland. I implemented the "traditional" functionality in GdkSeatDefault (which is what X11 uses), so it could be used by other backends too, might be actually easier for the missing ones since AFAIR we don't get devices added/removed at runtime on those. - In X11 specifically, the dependency between GdkSeat and GdkDeviceManager should be inverted in the future, we need splitting the GdkEventTranslator functionality out of it first, although I don't think GdkSeat is the place for this interface, as it's no longer a singleton.
Created attachment 317130 [details] [review] gdk: Add GdkSeat
Created attachment 317131 [details] [review] gdk: Add GdkSeatDefault
Created attachment 317132 [details] [review] GdkDevice: Add GdkSeat property and getter
Created attachment 317133 [details] [review] GdkDisplay: Add GdkSeat getters
Created attachment 317134 [details] [review] GdkEvent: Add GdkSeat getter and internal setter
Created attachment 317135 [details] [review] x11: Use GdkSeatDefault to implement GdkSeat
Created attachment 317136 [details] [review] GtkButton: Use gdk_seat_grab()
Created attachment 317137 [details] [review] GtkCellRendererAccel: Use gdk_seat_grab()
Created attachment 317138 [details] [review] GtkMenu: Use gdk_seat_grab()
Created attachment 317139 [details] [review] GtkComboBox: Use gdk_seat_grab()
Created attachment 317140 [details] [review] GtkEntryCompletion: Use gdk_seat_grab()
Created attachment 317141 [details] [review] GtkNotebook: Use gdk_seat_grab()
Created attachment 317142 [details] [review] GtkTreeView: Use gdk_seat_grab()
Created attachment 317143 [details] [review] wayland: Add GdkSeat implementation GdkWaylandDeviceData conceptually gathers the data that belongs to a seat, so it's been renamed (although the old typedef stays, plenty of refactoring is due here...). The methods in GdkSeatClass have also been implemented, the most remarkable is ::grab, which ensures the grab is performed on all the relevant "master" devices.
Created attachment 317144 [details] [review] GdkDisplay: Add ::seat-added/removed signals These will be emitted as seats come and go.
Created attachment 317145 [details] [review] wayland: Make gdk_wayland_device_get_focus() work on touch So we can figure out the focus for the master device.
Created attachment 317146 [details] [review] gdk: Manage GDK_TOUCH_CANCEL events on gdk_windowing_got_event() These events must get active/implicit grabs undone, and can be done on client-side code.
Created attachment 317147 [details] [review] wayland: Unset "pointer emulating" touch on wl_touch.cancel And emit the corresnponding leave event on its master pointer.
Created attachment 317148 [details] [review] wayland: Emit cancelled on touchpoint used on window dragging/moving This allows GDK to unset the grab itself. Also, make sure we unset the "pointer emulating" touch on the device if this is the pointer emulating sequence.
Created attachment 317149 [details] [review] gdk: Deprecate GdkDeviceManager and gdk_device_grab/ungrab() GdkSeat is now the preferred way to deal with input devices and grabs.
Created attachment 317150 [details] [review] wayland: Replace deprecated functions We can just gdk_seat_ungrab() here.
Created attachment 317151 [details] [review] wayland: Improve creation of windowing surface roles We no longer need a grabbed seat, instead we'll just use the default seat if this happens, not without first warning and recommending gdk_seat_grab() for the operation.
Review of attachment 317130 [details] [review]: Mostly looks good to me. ::: gdk/gdkseat.c @@ +114,3 @@ + * The ::device-added signal is emitted either when a new master + * pointer is created, or when a slave (Hardware) input device + * is plugged in. I am confused about this documentation - isn't there a single master pointer per seat ? @@ +204,3 @@ + * this is %NULL then the normal cursors are used for + * @window and its descendants, and the cursor for @window is used + * elsewhere. What is the recommended way to update the cursor while grabbed ? @@ +219,3 @@ + * all other "pointing" capabilities (eg. %GDK_SEAT_CAPABILITY_TOUCH) should + * be grabbed too, so the user is able to interact with all of those while + * the grab holds. Could introduce a shorthand for this to make it harder for apps to get this wrong ? Like #define GDK_SEAT_CAPABILITY_POINTING (GDK_SEAT_CAPABILITY_POINTER_ONLY|GDK_SEAT_CAPABILITY_TOUCH_ONLY) @@ +233,3 @@ + * If you set up anything at the time you take the grab that needs to be + * cleaned up when the grab ends, you should handle the #GdkEventGrabBroken + * events that are emitted when the grab ends unvoluntarily. We could continue to move away from events, and add a GdkSeatGrabCleanupFunc. But that would be argument # 9... @@ +291,3 @@ + * Returns: (transfer container) (element-type GdkSeat): A list of #GdkDevices. The list + * must be freed with g_list_free(), the elements are owned + * by GDK and must not be freed. Missing the occasional Since annotation in the docs @@ +312,3 @@ + * + * Returns: (transfer none) (nullable): a master #GdkDevice with pointer + ( capabilities. This memory is owned by GTK+ and must not be memory sounds odd here - either object or device would be better
Review of attachment 317131 [details] [review]: ::: gdk/gdkseatdefault.c @@ +108,3 @@ + + if (prepare_func && !gdk_window_is_visible (window)) + (prepare_func) (seat, window, prepare_func_data); Don't you think it is better to call prepare_func unconditionally ? @@ +114,3 @@ + g_critical ("Window %p has not been made visible in GdkSeatGrabPrepareFunc", + window); + return GDK_GRAB_FAILED; Maybe GDK_GRAB_NOT_VIEWABLE would be more appropriate here, given that we continue to return GdkGrabStatus, not a simple boolean @@ +119,3 @@ + G_GNUC_BEGIN_IGNORE_DEPRECATIONS; + + if (capabilities & ~GDK_SEAT_CAPABILITY_KEYBOARD) Might be more future-proof to make this explicit POINTER|TOUCH, instead of relying on the negation
Review of attachment 317132 [details] [review]: ::: gdk/gdkdevice.c @@ +1914,3 @@ + return; + + device->seat = seat; How does the memory management work here ? I assume - seats are forever - devices never switch seats ?
Review of attachment 317133 [details] [review]: ::: gdk/gdkdisplay.c @@ +2366,3 @@ + * + * Returns: (transfer none): the default seat. + **/ More missing Since annotations. ::: gdk/gdkdisplayprivate.h @@ +241,3 @@ + GdkSeat * (*get_default_seat) (GdkDisplay *display); + Hmm, so the list of seats is kept in the frontend, but the default seat is determined by vfunc. Wouldn't you expect the list to come from the backend as well ?
Review of attachment 317134 [details] [review]: ::: gdk/gdkevents.c @@ +2327,3 @@ + **/ +GdkSeat * +gdk_event_get_seat (const GdkEvent *event) This commit is missing the public header addition
Review of attachment 317135 [details] [review]: ::: gdk/x11/gdkdevicemanager-xi2.c @@ +1498,3 @@ + device = g_hash_table_lookup (device_manager->id_table, + GUINT_TO_POINTER (xev->deviceid)); + gdk_event_set_device (event, device); Converting to gdk_event_set_device() could be pulled out as a separate fix, possibly.
Review of attachment 317136 [details] [review]: ok
Review of attachment 317137 [details] [review]: ok
Review of attachment 317138 [details] [review]: ok. Isn't this where we would want to use the prepare func to show the window ?
Review of attachment 317139 [details] [review]: ok
Review of attachment 317140 [details] [review]: ok
Review of attachment 317141 [details] [review]: ::: gtk/gtknotebook.c @@ +3142,3 @@ + GtkNotebookPrivate *priv = notebook->priv; + + gdk_window_show (priv->drag_window); Should this be gdk_window_show (window) to be more obviously correct ?
Review of attachment 317142 [details] [review]: ok
Review of attachment 317143 [details] [review]: ok
Review of attachment 317144 [details] [review]: ::: gdk/gdkdisplay.c @@ +215,3 @@ + * + * The ::seat-added signal is emitted whenever a new seat is made + * known to the windowing system. Copy paste error here
Review of attachment 317145 [details] [review]: ::: gdk/wayland/gdkdevice-wayland.c @@ +489,3 @@ gdk_wayland_device_get_focus (GdkDevice *device) { + GdkWaylandSeat *wayland_seat = GDK_WAYLAND_DEVICE (device)->device; device->device ? should this be device->seat?
Review of attachment 317146 [details] [review]: ok
Review of attachment 317147 [details] [review]: ok
Review of attachment 317148 [details] [review]: ok
Review of attachment 317149 [details] [review]: ok
Review of attachment 317150 [details] [review]: ok
Review of attachment 317151 [details] [review]: ok
(In reply to Matthias Clasen from comment #23) > Review of attachment 317130 [details] [review] [review]: > > Mostly looks good to me. > > ::: gdk/gdkseat.c > @@ +114,3 @@ > + * The ::device-added signal is emitted either when a new master > + * pointer is created, or when a slave (Hardware) input device > + * is plugged in. > > I am confused about this documentation - isn't there a single master pointer > per seat ? Oops right, copied that too straight away from GdkDeviceManager > @@ +219,3 @@ > + * all other "pointing" capabilities (eg. %GDK_SEAT_CAPABILITY_TOUCH) should > + * be grabbed too, so the user is able to interact with all of those while > + * the grab holds. > > Could introduce a shorthand for this to make it harder for apps to get this > wrong ? > Like > > #define GDK_SEAT_CAPABILITY_POINTING > (GDK_SEAT_CAPABILITY_POINTER_ONLY|GDK_SEAT_CAPABILITY_TOUCH_ONLY) I felt tempted to do this, although there's still tablets missing. I am just adding that enum value and the shorthand, there's places where we can honor that after all. > > @@ +233,3 @@ > + * If you set up anything at the time you take the grab that needs to be > + * cleaned up when the grab ends, you should handle the #GdkEventGrabBroken > + * events that are emitted when the grab ends unvoluntarily. > > We could continue to move away from events, and add a > GdkSeatGrabCleanupFunc. But that would be argument # 9... As I think of it, in the future GdkEvent still makes sense to me as a a standalone message that gets passed around. I'd like it to be a lot more more opaque than it is now, there's however places like this where we need something to give back to the windowing to let it figure out the right time/serial/whatnot, at that point I think it'd be ok to have gtk_gesture_get_last_event() as the only way to let applications transfer this "current state" from the gesture to the windowing action. > > @@ +291,3 @@ > + * Returns: (transfer container) (element-type GdkSeat): A list of > #GdkDevices. The list > + * must be freed with g_list_free(), the elements are owned > + * by GDK and must not be freed. > > Missing the occasional Since annotation in the docs Oops, added > > @@ +312,3 @@ > + * > + * Returns: (transfer none) (nullable): a master #GdkDevice with pointer > + ( capabilities. This memory is owned by GTK+ and must not be > > memory sounds odd here - either object or device would be better Agreed, changed that to "object" (In reply to Matthias Clasen from comment #24) > Review of attachment 317131 [details] [review] [review]: > > ::: gdk/gdkseatdefault.c > @@ +108,3 @@ > + > + if (prepare_func && !gdk_window_is_visible (window)) > + (prepare_func) (seat, window, prepare_func_data); > > Don't you think it is better to call prepare_func unconditionally ? Right, would be more consistent. > > @@ +114,3 @@ > + g_critical ("Window %p has not been made visible in > GdkSeatGrabPrepareFunc", > + window); > + return GDK_GRAB_FAILED; > > Maybe GDK_GRAB_NOT_VIEWABLE would be more appropriate here, given that we > continue to return GdkGrabStatus, not a simple boolean Agreed. > > @@ +119,3 @@ > + G_GNUC_BEGIN_IGNORE_DEPRECATIONS; > + > + if (capabilities & ~GDK_SEAT_CAPABILITY_KEYBOARD) > > Might be more future-proof to make this explicit POINTER|TOUCH, instead of > relying on the negation Right, I wanted to make room for tablet stylus, but that's a too wide shot, making it more specific now. (In reply to Matthias Clasen from comment #25) > Review of attachment 317132 [details] [review] [review]: > > ::: gdk/gdkdevice.c > @@ +1914,3 @@ > + return; > + > + device->seat = seat; > > How does the memory management work here ? I assume > > - seats are forever > - devices never switch seats > > ? Not quite... seats may come and go, in x11 a "seat" is a master pointer/keyboard pair, so "xinput create-master foo" would be creating a new GdkSeat, in wayland you can possibly get new seat globals announced and removed at runtime, although I think no compositor gets there yet. As for devices switching seats, it can only happen on X11 (eg. xinput reattach) I trust in that case on the ref held by the GdkDeviceManager while moving the device between seats, same for floating devices (that do not belong to any seat) Even though the memory management is juggle-y, I do consider the GdkDevices to belong to the seat. Everywhere else than x11 the devices are "virtual" and will be tear down with the seat, and on X11 removing a master device pair will result in the devices being left floating, so belonging only to the GdkDeviceManager. In the future, when there's no GdkDeviceManager to take care of floating devices, I think we might just destroy/create devices indeed, although somewhere in X11 there would still need to be a global list of devices. (In reply to Matthias Clasen from comment #26) > Review of attachment 317133 [details] [review] [review]: > > ::: gdk/gdkdisplay.c > @@ +2366,3 @@ > + * > + * Returns: (transfer none): the default seat. > + **/ > > More missing Since annotations. > > ::: gdk/gdkdisplayprivate.h > @@ +241,3 @@ > > + GdkSeat * (*get_default_seat) (GdkDisplay > *display); > + > > Hmm, so the list of seats is kept in the frontend, but the default seat is > determined by vfunc. > Wouldn't you expect the list to come from the backend as well ? Could make sense for consistence... (In reply to Matthias Clasen from comment #27) > Review of attachment 317134 [details] [review] [review]: > > ::: gdk/gdkevents.c > @@ +2327,3 @@ > + **/ > +GdkSeat * > +gdk_event_get_seat (const GdkEvent *event) > > This commit is missing the public header addition Oops. (In reply to Matthias Clasen from comment #28) > Review of attachment 317135 [details] [review] [review]: > > ::: gdk/x11/gdkdevicemanager-xi2.c > @@ +1498,3 @@ > + device = g_hash_table_lookup (device_manager->id_table, > + GUINT_TO_POINTER (xev->deviceid)); > + gdk_event_set_device (event, device); > > Converting to gdk_event_set_device() could be pulled out as a separate fix, > possibly. Right, I agree. (In reply to Matthias Clasen from comment #37) > Review of attachment 317144 [details] [review] [review]: > > ::: gdk/gdkdisplay.c > @@ +215,3 @@ > + * > + * The ::seat-added signal is emitted whenever a new seat is made > + * known to the windowing system. > > Copy paste error here Indeed :) (In reply to Matthias Clasen from comment #38) > Review of attachment 317145 [details] [review] [review]: > > ::: gdk/wayland/gdkdevice-wayland.c > @@ +489,3 @@ > gdk_wayland_device_get_focus (GdkDevice *device) > { > + GdkWaylandSeat *wayland_seat = GDK_WAYLAND_DEVICE (device)->device; > > device->device ? should this be device->seat? There's plenty of cleanups due there :(. GdkWaylandDevice->device used to be a GdkWaylandDeviceData, which now is really GdkWaylandSeat, so it's all actually fine there. All occurrences to GdkWaylandDeviceData as "device" or "wd" should change, just that there's a truckload of these, I didn't want to make this patch too unobvious.
Pushed the whole bunch after addressing the issues spotted here. Attachment 317130 [details] pushed as 8d68f59 - gdk: Add GdkSeat Attachment 317131 [details] pushed as 0472c08 - gdk: Add GdkSeatDefault Attachment 317132 [details] pushed as d24f63e - GdkDevice: Add GdkSeat property and getter Attachment 317133 [details] pushed as 6f4edc0 - GdkDisplay: Add GdkSeat getters Attachment 317134 [details] pushed as da6d527 - GdkEvent: Add GdkSeat getter and internal setter Attachment 317135 [details] pushed as d236fd7 - x11: Use GdkSeatDefault to implement GdkSeat Attachment 317136 [details] pushed as e4eeec2 - GtkButton: Use gdk_seat_grab() Attachment 317137 [details] pushed as 5cbbb90 - GtkCellRendererAccel: Use gdk_seat_grab() Attachment 317138 [details] pushed as d54f208 - GtkMenu: Use gdk_seat_grab() Attachment 317139 [details] pushed as 534b0af - GtkComboBox: Use gdk_seat_grab() Attachment 317140 [details] pushed as fad174b - GtkEntryCompletion: Use gdk_seat_grab() Attachment 317141 [details] pushed as 91b5497 - GtkNotebook: Use gdk_seat_grab() Attachment 317142 [details] pushed as f663d17 - GtkTreeView: Use gdk_seat_grab() Attachment 317143 [details] pushed as 17525ef - wayland: Add GdkSeat implementation Attachment 317144 [details] pushed as 8ec3fb3 - GdkDisplay: Add ::seat-added/removed signals Attachment 317145 [details] pushed as 09947a6 - wayland: Make gdk_wayland_device_get_focus() work on touch Attachment 317146 [details] pushed as fc19a99 - gdk: Manage GDK_TOUCH_CANCEL events on gdk_windowing_got_event() Attachment 317147 [details] pushed as 77cf80f - wayland: Unset "pointer emulating" touch on wl_touch.cancel Attachment 317148 [details] pushed as 3009eac - wayland: Emit cancelled on touchpoint used on window dragging/moving Attachment 317149 [details] pushed as 4065bd1 - gdk: Deprecate GdkDeviceManager and gdk_device_grab/ungrab() Attachment 317150 [details] pushed as c7280e4 - wayland: Replace deprecated functions Attachment 317151 [details] pushed as a33aefc - wayland: Improve creation of windowing surface roles
Created attachment 317655 [details] [review] x11: create GdkSeat also in GdkX11DeviceManagerCore Missing part from d236fd7aab455c7c53c3a88d6a8397e79c02d0e8?
Comment on attachment 317655 [details] [review] x11: create GdkSeat also in GdkX11DeviceManagerCore Hmm, nice catch, I forgot we can still end up creating a core device manager standalone. However, a GdkX11DeviceManagerXI2 is a subclass of GdkX11DeviceManagerCore, so we'd end up with one extra seat on XI2. We should perhaps take seat creation out of GdkX11DeviceManagerXI2, and do it right after device manager creation (looking up the current master devices, and connecting to ::device-added/removed), that will make seat creation work with either device manager.
(In reply to Carlos Garnacho from comment #49) > Comment on attachment 317655 [details] [review] [review] > x11: create GdkSeat also in GdkX11DeviceManagerCore > > Hmm, nice catch, I forgot we can still end up creating a core device manager > standalone. However, a GdkX11DeviceManagerXI2 is a subclass of > GdkX11DeviceManagerCore, so we'd end up with one extra seat on XI2. Is that only problem? > We should perhaps take seat creation out of GdkX11DeviceManagerXI2, and do > it right after device manager creation (looking up the current master > devices, and connecting to ::device-added/removed), that will make seat > creation work with either device manager. How about going easier way? By checking if it is not XI2 device manager? if (GDK_IS_X11_DEVICE_MANAGER_XI2 (device_manager)) return; create seat Or if (!GDK_IS_X11_DEVICE_MANAGER_XI2 (device_manager)) { create seat }
Created attachment 317673 [details] [review] x11: create GdkSeat also in GdkX11DeviceManagerCore
Ended up pushing with just a minor modification, changed the device manager type check to: if (G_OBJECT_TYPE (object) == GDK_TYPE_X11_DEVICE_MANAGER_CORE) So we don't check for an specific subclass, which I wan't very thrilled about. Attachment 317673 [details] pushed as 54c32fa - x11: create GdkSeat also in GdkX11DeviceManagerCore