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 759309 - Add GdkSeat
Add GdkSeat
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-12-10 15:51 UTC by Carlos Garnacho
Modified: 2015-12-21 20:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdk: Add GdkSeat (20.94 KB, patch)
2015-12-10 15:55 UTC, Carlos Garnacho
committed Details | Review
gdk: Add GdkSeatDefault (13.93 KB, patch)
2015-12-10 15:55 UTC, Carlos Garnacho
committed Details | Review
GdkDevice: Add GdkSeat property and getter (4.43 KB, patch)
2015-12-10 15:55 UTC, Carlos Garnacho
committed Details | Review
GdkDisplay: Add GdkSeat getters (5.41 KB, patch)
2015-12-10 15:55 UTC, Carlos Garnacho
committed Details | Review
GdkEvent: Add GdkSeat getter and internal setter (2.53 KB, patch)
2015-12-10 15:55 UTC, Carlos Garnacho
committed Details | Review
x11: Use GdkSeatDefault to implement GdkSeat (14.17 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkButton: Use gdk_seat_grab() (1.65 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkCellRendererAccel: Use gdk_seat_grab() (2.82 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkMenu: Use gdk_seat_grab() (6.83 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkComboBox: Use gdk_seat_grab() (4.22 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkEntryCompletion: Use gdk_seat_grab() (3.37 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkNotebook: Use gdk_seat_grab() (1.68 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GtkTreeView: Use gdk_seat_grab() (1.88 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
wayland: Add GdkSeat implementation (28.33 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
GdkDisplay: Add ::seat-added/removed signals (2.35 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
wayland: Make gdk_wayland_device_get_focus() work on touch (1.46 KB, patch)
2015-12-10 15:56 UTC, Carlos Garnacho
committed Details | Review
gdk: Manage GDK_TOUCH_CANCEL events on gdk_windowing_got_event() (1.46 KB, patch)
2015-12-10 15:57 UTC, Carlos Garnacho
committed Details | Review
wayland: Unset "pointer emulating" touch on wl_touch.cancel (1.98 KB, patch)
2015-12-10 15:57 UTC, Carlos Garnacho
committed Details | Review
wayland: Emit cancelled on touchpoint used on window dragging/moving (2.02 KB, patch)
2015-12-10 15:57 UTC, Carlos Garnacho
committed Details | Review
gdk: Deprecate GdkDeviceManager and gdk_device_grab/ungrab() (4.54 KB, patch)
2015-12-10 15:57 UTC, Carlos Garnacho
committed Details | Review
wayland: Replace deprecated functions (1.22 KB, patch)
2015-12-10 15:57 UTC, Carlos Garnacho
committed Details | Review
wayland: Improve creation of windowing surface roles (8.72 KB, patch)
2015-12-10 15:57 UTC, Carlos Garnacho
committed Details | Review
x11: create GdkSeat also in GdkX11DeviceManagerCore (1.67 KB, patch)
2015-12-19 06:17 UTC, Alberts Muktupāvels
none Details | Review
x11: create GdkSeat also in GdkX11DeviceManagerCore (1.55 KB, patch)
2015-12-19 17:28 UTC, Alberts Muktupāvels
committed Details | Review

Description Carlos Garnacho 2015-12-10 15:51:52 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.
Comment 1 Carlos Garnacho 2015-12-10 15:55:34 UTC
Created attachment 317130 [details] [review]
gdk: Add GdkSeat
Comment 2 Carlos Garnacho 2015-12-10 15:55:40 UTC
Created attachment 317131 [details] [review]
gdk: Add GdkSeatDefault
Comment 3 Carlos Garnacho 2015-12-10 15:55:45 UTC
Created attachment 317132 [details] [review]
GdkDevice: Add GdkSeat property and getter
Comment 4 Carlos Garnacho 2015-12-10 15:55:50 UTC
Created attachment 317133 [details] [review]
GdkDisplay: Add GdkSeat getters
Comment 5 Carlos Garnacho 2015-12-10 15:55:55 UTC
Created attachment 317134 [details] [review]
GdkEvent: Add GdkSeat getter and internal setter
Comment 6 Carlos Garnacho 2015-12-10 15:56:01 UTC
Created attachment 317135 [details] [review]
x11: Use GdkSeatDefault to implement GdkSeat
Comment 7 Carlos Garnacho 2015-12-10 15:56:06 UTC
Created attachment 317136 [details] [review]
GtkButton: Use gdk_seat_grab()
Comment 8 Carlos Garnacho 2015-12-10 15:56:12 UTC
Created attachment 317137 [details] [review]
GtkCellRendererAccel: Use gdk_seat_grab()
Comment 9 Carlos Garnacho 2015-12-10 15:56:17 UTC
Created attachment 317138 [details] [review]
GtkMenu: Use gdk_seat_grab()
Comment 10 Carlos Garnacho 2015-12-10 15:56:23 UTC
Created attachment 317139 [details] [review]
GtkComboBox: Use gdk_seat_grab()
Comment 11 Carlos Garnacho 2015-12-10 15:56:29 UTC
Created attachment 317140 [details] [review]
GtkEntryCompletion: Use gdk_seat_grab()
Comment 12 Carlos Garnacho 2015-12-10 15:56:34 UTC
Created attachment 317141 [details] [review]
GtkNotebook: Use gdk_seat_grab()
Comment 13 Carlos Garnacho 2015-12-10 15:56:40 UTC
Created attachment 317142 [details] [review]
GtkTreeView: Use gdk_seat_grab()
Comment 14 Carlos Garnacho 2015-12-10 15:56:46 UTC
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.
Comment 15 Carlos Garnacho 2015-12-10 15:56:52 UTC
Created attachment 317144 [details] [review]
GdkDisplay: Add ::seat-added/removed signals

These will be emitted as seats come and go.
Comment 16 Carlos Garnacho 2015-12-10 15:56:58 UTC
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.
Comment 17 Carlos Garnacho 2015-12-10 15:57:04 UTC
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.
Comment 18 Carlos Garnacho 2015-12-10 15:57:10 UTC
Created attachment 317147 [details] [review]
wayland: Unset "pointer emulating" touch on wl_touch.cancel

And emit the corresnponding leave event on its master pointer.
Comment 19 Carlos Garnacho 2015-12-10 15:57:15 UTC
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.
Comment 20 Carlos Garnacho 2015-12-10 15:57:21 UTC
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.
Comment 21 Carlos Garnacho 2015-12-10 15:57:26 UTC
Created attachment 317150 [details] [review]
wayland: Replace deprecated functions

We can just gdk_seat_ungrab() here.
Comment 22 Carlos Garnacho 2015-12-10 15:57:33 UTC
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.
Comment 23 Matthias Clasen 2015-12-10 18:28:37 UTC
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
Comment 24 Matthias Clasen 2015-12-10 18:43:37 UTC
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
Comment 25 Matthias Clasen 2015-12-10 18:46:46 UTC
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

?
Comment 26 Matthias Clasen 2015-12-10 18:53:09 UTC
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 ?
Comment 27 Matthias Clasen 2015-12-10 18:55:09 UTC
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
Comment 28 Matthias Clasen 2015-12-10 18:59:04 UTC
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.
Comment 29 Matthias Clasen 2015-12-10 19:00:24 UTC
Review of attachment 317136 [details] [review]:

ok
Comment 30 Matthias Clasen 2015-12-10 19:01:19 UTC
Review of attachment 317137 [details] [review]:

ok
Comment 31 Matthias Clasen 2015-12-10 19:03:18 UTC
Review of attachment 317138 [details] [review]:

ok. Isn't this where we would want to use the prepare func to show the window ?
Comment 32 Matthias Clasen 2015-12-10 19:03:48 UTC
Review of attachment 317139 [details] [review]:

ok
Comment 33 Matthias Clasen 2015-12-10 19:05:13 UTC
Review of attachment 317140 [details] [review]:

ok
Comment 34 Matthias Clasen 2015-12-10 19:07:10 UTC
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 ?
Comment 35 Matthias Clasen 2015-12-10 19:07:48 UTC
Review of attachment 317142 [details] [review]:

ok
Comment 36 Matthias Clasen 2015-12-10 19:15:13 UTC
Review of attachment 317143 [details] [review]:

ok
Comment 37 Matthias Clasen 2015-12-10 19:17:23 UTC
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
Comment 38 Matthias Clasen 2015-12-10 19:29:37 UTC
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?
Comment 39 Matthias Clasen 2015-12-10 19:50:07 UTC
Review of attachment 317146 [details] [review]:

ok
Comment 40 Matthias Clasen 2015-12-10 19:54:08 UTC
Review of attachment 317146 [details] [review]:

ok
Comment 41 Matthias Clasen 2015-12-10 19:56:44 UTC
Review of attachment 317147 [details] [review]:

ok
Comment 42 Matthias Clasen 2015-12-10 20:00:13 UTC
Review of attachment 317148 [details] [review]:

ok
Comment 43 Matthias Clasen 2015-12-10 20:02:25 UTC
Review of attachment 317149 [details] [review]:

ok
Comment 44 Matthias Clasen 2015-12-10 20:04:16 UTC
Review of attachment 317150 [details] [review]:

ok
Comment 45 Matthias Clasen 2015-12-10 20:06:13 UTC
Review of attachment 317151 [details] [review]:

ok
Comment 46 Carlos Garnacho 2015-12-14 18:21:21 UTC
(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.
Comment 47 Carlos Garnacho 2015-12-14 23:49:36 UTC
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
Comment 48 Alberts Muktupāvels 2015-12-19 06:17:19 UTC
Created attachment 317655 [details] [review]
x11: create GdkSeat also in GdkX11DeviceManagerCore

Missing part from d236fd7aab455c7c53c3a88d6a8397e79c02d0e8?
Comment 49 Carlos Garnacho 2015-12-19 12:09:36 UTC
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.
Comment 50 Alberts Muktupāvels 2015-12-19 17:17:08 UTC
(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
  }
Comment 51 Alberts Muktupāvels 2015-12-19 17:28:39 UTC
Created attachment 317673 [details] [review]
x11: create GdkSeat also in GdkX11DeviceManagerCore
Comment 52 Carlos Garnacho 2015-12-21 20:01:59 UTC
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