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 688779 - Port mutter to XI2
Port mutter to XI2
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 681082 (view as bug list)
Depends on:
Blocks: 687573
 
 
Reported: 2012-11-21 02:40 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-02-09 20:22 UTC
See Also:
GNOME target: 3.8
GNOME version: ---


Attachments
display: Initialize XInput2 (1.89 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
common: Add the XInput2 headers to common.h, which everything uses (695 bytes, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
display: separate input/non-input events handling in the event callback (73.31 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
core: enable XInput2 by default (3.54 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
window: Move grab op sync handling code out (5.37 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
display: Don't pass an event to a handler that will no-op (1.68 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
Port mutter to use XInput2 events instead of Core Events (85.25 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
display: Only care about input events for the VCP/VCK (1.55 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
compositor: Use XInput2 to grab the pointer/keyboard (4.97 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
compositor: Identify XI2 events as grabbed events (1.67 KB, patch)
2012-11-21 02:40 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
display: Use XInput2 to grab the pointer (2.90 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
display: Grab buttons with XI2 (2.58 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
keybindings: Grab keyboard with XI2 (2.79 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
keybindings: Grab keys with XI2 (2.26 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
display: Add spew for XI2 (2.52 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
ui: Use XI2 to fake GDK events (6.12 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
Remove support for Core Events (4.80 KB, patch)
2012-11-21 02:41 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
Select for XI2 events everywhere else (6.98 KB, patch)
2012-11-26 01:39 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
Require XInput2 (772 bytes, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Initialize XInput2 (1.87 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
common: Add the XInput2 headers to common.h, which everything uses (697 bytes, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: separate input/non-input events handling in the event callback (73.46 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
core: enable XInput2 by default (1.55 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Don't pass an event to a handler that will no-op (1.69 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window: Move grab op sync handling code out (5.40 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Port mutter to use XInput2 events instead of Core Events (79.10 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Only care about input events for the VCP/VCK (1.62 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
compositor: Use XInput2 to grab the pointer/keyboard (5.99 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
compositor: Identify XI2 events as grabbed events (1.67 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Use XInput2 to grab the pointer (6.19 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Grab buttons with XI2 (2.59 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
keybindings: Grab keyboard with XI2 (2.65 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
keybindings: Grab keys with XI2 (2.27 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Select for XI2 events everywhere else (7.12 KB, patch)
2012-12-13 02:14 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
display: Add spew for XI2 (4.09 KB, patch)
2012-12-13 02:15 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
ui: Use XI2 to fake GDK events (6.13 KB, patch)
2012-12-13 02:15 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Remove support for Core Events (4.81 KB, patch)
2012-12-13 02:15 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
keybindings: Fix whitespace and alignment (66.56 KB, patch)
2012-12-13 02:15 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:29 UTC
This is now required for pressure barrier work.

Some of this code is based on Carlos Garnacho's work in wip/xinput2.
My initial work on this was to rebase wip/xinput2, and this can be
found in the wip/xinput2r branch.

After talking with Owen, a different approach seemed necessary:

  * Remove the concept of having multiple pointer/keyboard pairs.
    As far as we're aware, the only use case for this is display
    walls, and GNOME (mutter even more so) is not meant for display
    walls.

  * Remove support for core events after XI2 has landed. Carlos's
    original work built a frakenlayer that supported both XI2 and
    core events. This work is a full port, not a translation layer.
    The only potential issue could be with xvnc -- I have heard
    through a person that xvnc will not send out XI2 events at the
    moment. I do not know if this is still true, nor do I know if
    it will or can be fixed.

Many of these patches have large indentation or other changes. It may
be worthwhile to check out the branch, and use "git show -b" to ignore
whitespace while skipping commits, or use the "&ignorews=1" parameter
on cgit commits.

I have not exhaustively tested these patches; I have ensured that
gnome-shell works, the overview works, dragging windows (from the
panel or not) works, clicking on the buttons in window titlebars
work, and that keybindings (especially things like alt-tab) work.
There may still be some stuck grabs or modals that we should look
out for.

These patches can also be found in the wip/xinput2b branch.

Thanks!
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:33 UTC
Created attachment 229536 [details] [review]
display: Initialize XInput2

Make sure it's the correct version.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:35 UTC
Created attachment 229537 [details] [review]
common: Add the XInput2 headers to common.h, which everything uses
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:38 UTC
Created attachment 229538 [details] [review]
display: separate input/non-input events handling in the event callback

We now use meta_input_event_get_type() to discern input events from the
others. This commit has involved plenty of indenting changes, so it's
better seen with git diff -b.
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:40 UTC
Created attachment 229539 [details] [review]
core: enable XInput2 by default

We don't support more than the core mouse/keyboard pair, really,
but as far as I'm aware, there's no existing use case for multiple
device pairs besides things like display walls, which mutter
doesn't support or really care about supproting.

the MUTTER_USE_CORE_DEVICES envvar has been added to force use
of Xlib core events for devices in order to check for regressions.
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:43 UTC
Created attachment 229540 [details] [review]
window: Move grab op sync handling code out

This removes some duplicate event type checks, and will make
the code cleaner in the future when we want to make the grab_op_event
handler take an XIDeviceEvent directly.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:46 UTC
Created attachment 229541 [details] [review]
display: Don't pass an event to a handler that will no-op

meta_window_handle_mouse_grab_op_event won't do anything on a
EnterNotify/LeaveNotify, so why are we passing something to it?
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:49 UTC
Created attachment 229542 [details] [review]
Port mutter to use XInput2 events instead of Core Events
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:51 UTC
Created attachment 229543 [details] [review]
display: Only care about input events for the VCP/VCK
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:54 UTC
Created attachment 229544 [details] [review]
compositor: Use XInput2 to grab the pointer/keyboard
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:40:57 UTC
Created attachment 229545 [details] [review]
compositor: Identify XI2 events as grabbed events
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:00 UTC
Created attachment 229546 [details] [review]
display: Use XInput2 to grab the pointer
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:02 UTC
Created attachment 229547 [details] [review]
display: Grab buttons with XI2
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:05 UTC
Created attachment 229548 [details] [review]
keybindings: Grab keyboard with XI2
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:08 UTC
Created attachment 229549 [details] [review]
keybindings: Grab keys with XI2
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:10 UTC
Created attachment 229550 [details] [review]
display: Add spew for XI2
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:13 UTC
Created attachment 229551 [details] [review]
ui: Use XI2 to fake GDK events

This removes our final dependency on Core Events, meaning
we can remove support code for them soon.

This commit is a bit ugly as it requires ui having a dependency on
core, but this is already a hack, and two hacks make a right or
however that goes.
Comment 17 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:41:16 UTC
Created attachment 229552 [details] [review]
Remove support for Core Events
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-11-21 02:44:13 UTC
This also goes well with bug #687573.
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-11-26 01:39:34 UTC
Created attachment 229869 [details] [review]
Select for XI2 events everywhere else

In random places that are not grabs, we selected for events on
things like the root window, stage window, COW and more. Switch
these over to using the proper XI2 APIs.



It seems that I accidentally dropped this patch when rebasing the branch.
Attaching here -- supposed to go after the "Grab keys" change, but I don't
think that matters too much.
Comment 20 Owen Taylor 2012-12-12 17:35:45 UTC
One immediate thing looking through this patch set is that the XI2 APIs for handling events are not very convenient and result in ugly code - would it make sense to add:

 meta_display_select_events (display, window,
                             XI_KeyPress, XI_KeyRelease,
                             XI_ButtonPress, XI_ButtonRelease,
                             [...]
                             0);

or maybe:

 meta_display_select_events (display, window,
                             META_EVENTS_KEY | META_EVENTS_BUTTON | [..]);

and similar for grabbing the keyboard and pointer (there it seems we are always using the same event mask, so maybe it doesn't even need to be passed)

There's an argument against this as well - that sticking to the standard XI2 API's means that's immediately understandable and cut-and-pastable by someone coming from some other code base, so I'll just put it out there as a suggestion.
Comment 21 Owen Taylor 2012-12-12 17:36:38 UTC
(In reply to comment #20)
> One immediate thing looking through this patch set is that the XI2 APIs for
> handling events are not very convenient and result in ugly code - would it make
> sense to add:

To follow up here, if you do want to do this, I'm fine if it's a patch at the end - doesn't need to be rebased through the branch.
Comment 22 Owen Taylor 2012-12-12 18:14:21 UTC
You didn't attach the "Require XI2" patch. It looks OK to me. If future changes require libXi newer than 1.6.1 then that will have to be done as conditional compilation.
Comment 23 Owen Taylor 2012-12-12 18:27:44 UTC
Review of attachment 229536 [details] [review]:

I'm OK with the hard requirement on XInput 2.2 - as far as I know it will be available on any Xorg server including things like Xvnc - a few minor issues

::: src/core/display-private.h
@@ +284,3 @@
   int xfixes_event_base;
   int xfixes_error_base;
+  int xinput2_error_base;

it's a little odd that these are xinput2 - it's the XInput extension.

::: src/core/display.c
@@ +782,3 @@
+
+  {
+    int major = 2, minor = 2;

This initialization makes no sense, and does not seem necessary to keep GCC quiet.

@@ +797,3 @@
+
+    if (!has_xi)
+      meta_fatal ("X server does not have an up to date version of XInput2\n");

up-to-date should be hyphenated. I think it's probably better to say 

 X server doesn't have the XInput extension, version 2.2 or newer

to make the requirement clear.
Comment 24 Owen Taylor 2012-12-12 18:28:55 UTC
Review of attachment 229537 [details] [review]:

Looks good
Comment 25 Owen Taylor 2012-12-12 18:39:23 UTC
Review of attachment 229538 [details] [review]:

::: src/core/display.c
@@ +1843,3 @@
   gboolean bypass_compositor;
   gboolean filter_out_event;
+  XEvent *xev;

This patch is a little confusing because it's just a step, but leaving that aside, it's not acceptable to have:

 XEvent *event;
 XIEvent *xev;

'event' 'xevent' and 'xev' are all common names for a XEvent * in X coding with xev probably being the most common. xev variable should be input_event.

@@ +1983,3 @@
     {
+      if (window && !window->override_redirect &&
+          ((evtype == KeyPress) || (evtype == ButtonPress)))

this patch doesn't compile because it introduces evtype which isn't set anywhere - this is reverted out in the "Port mutter to use XInput2 events instead of Core Events" patch
Comment 26 Owen Taylor 2012-12-12 18:47:22 UTC
Review of attachment 229539 [details] [review]:

Maybe the comment needs rewriting and the Author changed from Carlos - I'm thinking there is not much left compared to the original patch and the comment:

 We don't support more than the core mouse/keyboard pair, really,
 but as far as I'm aware, there's no existing use case for multiple
 device pairs besides things like display walls, which mutter
 doesn't support or really care about supporting.

seems to have little to do with this patch. Maybe you could put a better version of that as a comment in the code when you call clutter_x11_enable_xinput (); and try to quantify approximately what would happen if the user has a second master pair with regards to the interaction of the three libraries GTK+/Mutter/Clutter and what the user will see.

::: src/core/main.c
@@ +418,3 @@
   if (g_getenv ("MUTTER_DEBUG"))
     meta_set_debugging (TRUE);
+  if (g_getenv ("MUTTER_USE_CORE_DEVICES"))

I think it's confusing to have a variable that is called MUTTER_ and means "use core X input for GTK+ and Clutter, but keep using XInput2 for Mutter" - that doesn't seem like anything we want long term, and we don't really expect it to fully work - e.g., the GTK+ and Mutter code won't be on the same page. As long as gnome-shell is working reasonably OK on top of this branch and you don't actively need this for debugging, this would be best removed.
Comment 27 Owen Taylor 2012-12-12 18:48:11 UTC
Review of attachment 229539 [details] [review]:

Marking needs-work
Comment 28 Owen Taylor 2012-12-12 18:52:10 UTC
Review of attachment 229540 [details] [review]:

Patch doesn't actually call the function from display.c - that gets introduced in "Port mutter to use XInput2 events instead of Core Events"

::: src/core/window-private.h
@@ +588,1 @@
 void meta_window_handle_mouse_grab_op_event (MetaWindow *window,

This patch should rename this function to 

  meta_window_handle_mouse_grab_op_input_event

for consistency and clarity
Comment 29 Owen Taylor 2012-12-12 19:02:56 UTC
Review of attachment 229540 [details] [review]:

::: src/core/window-private.h
@@ +588,1 @@
 void meta_window_handle_mouse_grab_op_event (MetaWindow *window,

Actually, even though renaming to meta_window_handle_mouse_grab_op_input_event makes sense, it's better not to do it - because in the frame-synchronization branch, what you have as meta_window_handle_mouse_grab_op_sync_event gets replaced with:

void
meta_window_update_sync_request_counter (MetaWindow *window,
                                         gint64      new_counter_value)

and an XEvent isn't passed as all.
Comment 30 Owen Taylor 2012-12-12 19:03:13 UTC
Review of attachment 229541 [details] [review]:

Looks good
Comment 31 Owen Taylor 2012-12-12 19:44:03 UTC
Review of attachment 229542 [details] [review]:

Basics look good, some naming comments

::: src/core/display.c
@@ +1997,3 @@
     {
+      XIDeviceEvent *xev_d = (XIDeviceEvent *) xev;
+      XIEnterEvent *xev_e = (XIEnterEvent *) xev;

With the suggestion of xev => input_event, I'd suggest device_event and enter_event for these variables.

::: src/core/keybindings-private.h
@@ +69,3 @@
+gboolean meta_display_process_key_event     (MetaDisplay   *display,
+                                             MetaWindow    *window,
+                                             XIDeviceEvent *xev);

this would be device_event too, if not left as 'event'

::: src/core/keybindings.c
@@ +110,3 @@
                                       MetaScreen     *screen,
                                       MetaWindow     *window,
+                                      XIDeviceEvent  *xev,

Globally throughout this file the change from 'event' to 'xev' is something I really don't like - 'xev' really strongly binds in my mind to 'XEvent' while 'event' is just generic and can refer to any sort of event. We generally avoid abbreviations like 'ev' as well.

I know it's going to be a rebase pain to change it back, but I'd very much prefer that it not be left this way.

@@ +2621,3 @@
                       MetaScreen     *screen,
                       MetaWindow     *window,
+                      XIDeviceEvent *xev,

Alignment got messed up for all of these

::: src/core/window-private.h
@@ +563,3 @@
                                         XEvent     *event);
 gboolean meta_window_notify_focus      (MetaWindow *window,
+                                        XIEnterEvent *xev);

same comment here

::: src/core/window.c
@@ +6816,3 @@
 
+void
+meta_window_unmap_notify (MetaWindow *window)

This function is not well named since it's much more often called on a focus out than an unmap - 'meta_window_lost_focus' might be a good name.

@@ +9158,3 @@
+   * GDK will handle later these events, and eventually
+   * free the cookie data itself.
+   */

Yikes scary, but probably best.

(This predicate is run only once for each event, so we could XFreeEventData and the effect wouldn't be catastrophic  - still the optimization seems useful.)
Comment 32 Owen Taylor 2012-12-12 19:46:53 UTC
Review of attachment 229543 [details] [review]:

::: src/core/display.c
@@ +1812,3 @@
        */
       xev = (XIEvent *) event->xcookie.data;
+      xev_d = (XIDeviceEvent *) xev;

I don't like having a potentially invalid cast even if we don't dereference it

@@ +1819,3 @@
         case XI_ButtonPress:
         case XI_ButtonRelease:
+          if (xev_d->deviceid == VIRTUAL_CORE_POINTER_ID)

I'd probably just write ((XIDeviceEvent *)xev)->deviceid here - not really worth introducing a new variable for a single dereference.

@@ +1829,3 @@
         case XI_FocusIn:
         case XI_FocusOut:
         case XI_Enter:

You need to check here as well, right?
Comment 33 Owen Taylor 2012-12-12 19:56:21 UTC
Review of attachment 229544 [details] [review]:

::: src/core/core.h
@@ +31,3 @@
 
+#define VIRTUAL_CORE_POINTER_ID 2
+#define VIRTUAL_CORE_KEYBOARD_ID 3

This shouldn't be in core.h - core.h is about ui => core communication.
I'd also like to see the names META_XINPUT_... we almost never (never?) define non-namespaced stuff even in internal header files.

Comment should be moved along with the #defines
Comment 34 Owen Taylor 2012-12-12 19:58:08 UTC
Review of attachment 229545 [details] [review]:

OK
Comment 35 Owen Taylor 2012-12-12 20:08:57 UTC
Review of attachment 229546 [details] [review]:

If there are other master devices other than the virtual core pointer XChangeActivePointerGrab() will not do the right thing - it will pick a *random* master pointer device, check to see if it has a grab, and if not, return as a no-op. You may be able to replace it with an overgrab of the existing grab - you'll have to check semantics and failure conditions carefully. If not, file a bug and link to it.

::: src/core/display.c
@@ -3723,3 @@
                         grab_xwindow,
-                        False,
-                        GRAB_MASK,

If we keep using XChangeActivePointerGrab, the GRAB_MASK constant should still be removed and inlined in the one place it is used - with comments added that things need to be kept in sync.
Comment 36 Owen Taylor 2012-12-12 20:10:31 UTC
Review of attachment 229547 [details] [review]:

OK
Comment 37 Owen Taylor 2012-12-12 20:12:58 UTC
Review of attachment 229548 [details] [review]:

OK
Comment 38 Owen Taylor 2012-12-12 20:14:10 UTC
Review of attachment 229549 [details] [review]:

OK
Comment 39 Owen Taylor 2012-12-12 20:16:27 UTC
Noted: meta_window_ensure_frame() is still using core event selection
Comment 40 Owen Taylor 2012-12-12 20:19:05 UTC
Review of attachment 229550 [details] [review]:

::: src/core/display.c
@@ +3257,3 @@
+      XIEvent *xev = (XIEvent *) event->xcookie.data;
+      meta_spew_xi2event (display, xev, &name, &extra);
+      goto spew;

Spaghetti goto - add meta_get_core_event_spew_details(), meta_get_input_event_spew_details()
Comment 41 Owen Taylor 2012-12-12 20:35:40 UTC
Review of attachment 229551 [details] [review]:

Ugh - but OK - since the ui/core split has no actual meaning for us, it's not worth ugliness like passing the XInput2 event base to the ui/ directory to avoid a hack.

::: src/ui/ui.c
@@ +121,3 @@
+  XIEvent *xev;
+  XIDeviceEvent *xev_d;
+  XIEnterEvent *xev_e;

Surprised that this isn't a compiler warning for uninitialized data, but if it isn't it isn't.
Comment 42 Owen Taylor 2012-12-12 20:36:26 UTC
Review of attachment 229552 [details] [review]:

OK
Comment 43 Owen Taylor 2012-12-12 20:42:42 UTC
Review of attachment 229869 [details] [review]:

::: src/core/screen.c
@@ +736,3 @@
   /* We need to or with the existing event mask since
    * gtk+ may be interested in other events.
    */

the fact that you changed the code to actually match this comment is probably worth a mentin in the commit message

@@ +746,3 @@
+    XISetMask (mask.mask, XI_KeyRelease);
+    XISetMask (mask.mask, XI_ButtonPress);
+    XISetMask (mask.mask, XI_ButtonRelease);

You accidentally added ButtonPress/ButtonRelease - since only one client can select for ButtonPress on the root window, this is a dangerous thing to do.
Comment 44 Jasper St. Pierre (not reading bugmail) 2012-12-12 21:56:22 UTC
(In reply to comment #23)
::: src/core/display.c
> @@ +782,3 @@
> +
> +  {
> +    int major = 2, minor = 2;
> 
> This initialization makes no sense, and does not seem necessary to keep GCC
> quiet.

The version we pass to XIQueryVersion specifies the client version. See http://www.x.org/archive/X11R7.5/doc/man/man3/XIQueryVersion.3.html

... or do you mean that you would prefer it not as C99 init, but in actual code?
Comment 45 Owen Taylor 2012-12-12 22:05:22 UTC
(In reply to comment #44)
> (In reply to comment #23)
> ::: src/core/display.c
> > @@ +782,3 @@
> > +
> > +  {
> > +    int major = 2, minor = 2;
> > 
> > This initialization makes no sense, and does not seem necessary to keep GCC
> > quiet.
> 
> The version we pass to XIQueryVersion specifies the client version. See
> http://www.x.org/archive/X11R7.5/doc/man/man3/XIQueryVersion.3.html
> 
> ... or do you mean that you would prefer it not as C99 init, but in actual
> code?

I had forgotten that XI behaved like this, so it's fine. (Obviously, this breaks basic library design principles, and makes the ordering of initializing GTK+ and calling XIQueryVersion() ourselves a critical one.)
Comment 46 Jasper St. Pierre (not reading bugmail) 2012-12-12 22:07:37 UTC
(In reply to comment #20)
> One immediate thing looking through this patch set is that the XI2 APIs for
> handling events are not very convenient and result in ugly code - would it make
> sense to add:
> 
>  meta_display_select_events (display, window,
>                              XI_KeyPress, XI_KeyRelease,
>                              XI_ButtonPress, XI_ButtonRelease,
>                              [...]
>                              0);
> 
> or maybe:
> 
>  meta_display_select_events (display, window,
>                              META_EVENTS_KEY | META_EVENTS_BUTTON | [..]);
> 
> and similar for grabbing the keyboard and pointer (there it seems we are always
> using the same event mask, so maybe it doesn't even need to be passed)
> 
> There's an argument against this as well - that sticking to the standard XI2
> API's means that's immediately understandable and cut-and-pastable by someone
> coming from some other code base, so I'll just put it out there as a
> suggestion.

I agree that it's complicated and long-winded, but I think the immediacy of the code is important.

This was originally there in garnacho's branch (it was meta_core_select_events, which I think you'd say is wrong), but I removed it to remove the malloc in favor of the stack allocation, and also because it didn't play well with other sorts of masks.

I thought for a while of making a macro or similar, but that seems sort of complicated, and a new person trying to unravel it would be more perplexed by this magic macro that can somehow iterate over the arguments it's given and magically set masks seemed much more wrong to me.

... or maybe the malloc isn't that bad after all.
Comment 47 Jasper St. Pierre (not reading bugmail) 2012-12-13 00:40:03 UTC
(In reply to comment #35)
> Review of attachment 229546 [details] [review]:
> 
> If there are other master devices other than the virtual core pointer
> XChangeActivePointerGrab() will not do the right thing - it will pick a
> *random* master pointer device, check to see if it has a grab, and if not,
> return as a no-op. You may be able to replace it with an overgrab of the
> existing grab - you'll have to check semantics and failure conditions
> carefully. If not, file a bug and link to it.
> 
> ::: src/core/display.c
> @@ -3723,3 @@
>                          grab_xwindow,
> -                        False,
> -                        GRAB_MASK,
> 
> If we keep using XChangeActivePointerGrab, the GRAB_MASK constant should still
> be removed and inlined in the one place it is used - with comments added that
> things need to be kept in sync.

It seems that just re-calling XIGrabDevice should make things work, so I'll drop the branch and re-call XIGrabDevice.
Comment 48 Jasper St. Pierre (not reading bugmail) 2012-12-13 00:41:13 UTC
(In reply to comment #43)
> Review of attachment 229869 [details] [review]:
> 
> ::: src/core/screen.c
> @@ +736,3 @@
>    /* We need to or with the existing event mask since
>     * gtk+ may be interested in other events.
>     */
> 
> the fact that you changed the code to actually match this comment is probably
> worth a mentin in the commit message

How was it not matching before? It was ORing in your_event_mask, just like it is now.
Comment 49 Jasper St. Pierre (not reading bugmail) 2012-12-13 00:51:28 UTC
(In reply to comment #31)
> @@ +2621,3 @@
>                        MetaScreen     *screen,
>                        MetaWindow     *window,
> +                      XIDeviceEvent *xev,
> 
> Alignment got messed up for all of these

Alignment was already messed up for some recent keybindings work, so how do you feel about a post-XI2 "keybindings: Fix alignment" patch?
Comment 50 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:04 UTC
Created attachment 231425 [details] [review]
Require XInput2

This is going to start becoming necessary for mutter to run.
Comment 51 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:09 UTC
Created attachment 231426 [details] [review]
display: Initialize XInput2

Make sure it's the correct version.
Comment 52 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:12 UTC
Created attachment 231427 [details] [review]
common: Add the XInput2 headers to common.h, which everything uses
Comment 53 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:15 UTC
Created attachment 231428 [details] [review]
display: separate input/non-input events handling in the event callback

We now use meta_input_event_get_type() to discern input events from the
others. This commit has involved plenty of indenting changes, so it's
better seen with git diff -b.
Comment 54 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:18 UTC
Created attachment 231429 [details] [review]
core: enable XInput2 by default

Enable XI2 support in both Clutter and GDK.
Comment 55 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:22 UTC
Created attachment 231430 [details] [review]
display: Don't pass an event to a handler that will no-op

meta_window_handle_mouse_grab_op_event won't do anything on a
EnterNotify/LeaveNotify, so why are we passing something to it?
Comment 56 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:26 UTC
Created attachment 231431 [details] [review]
window: Move grab op sync handling code out

This removes some duplicate event type checks, and will make
the code cleaner in the future when we want to make the grab_op_event
handler take an XIDeviceEvent directly.

Based on a patch by Owen Taylor <otaylor@fishsoup.net>
Comment 57 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:29 UTC
Created attachment 231432 [details] [review]
Port mutter to use XInput2 events instead of Core Events

Mechanically transform the event processing of mutter to care
about XI2 events instead of Core Events. Core Events will be left
in the dust soon, and removed entirely.
Comment 58 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:32 UTC
Created attachment 231433 [details] [review]
display: Only care about input events for the VCP/VCK

It's unlikely that we'll ever want to support multiple pointer
devices. Multiple keyboard devices may become useful in the future,
but for now, only care about the core keyboard.
Comment 59 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:35 UTC
Created attachment 231434 [details] [review]
compositor: Use XInput2 to grab the pointer/keyboard
Comment 60 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:39 UTC
Created attachment 231435 [details] [review]
compositor: Identify XI2 events as grabbed events
Comment 61 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:42 UTC
Created attachment 231436 [details] [review]
display: Use XInput2 to grab the pointer

As calling XIGrabDevice multiple times will change it, just
drop the XChangeActivePointerGrab path and just go down the
XIGrabPointer path always.
Comment 62 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:45 UTC
Created attachment 231437 [details] [review]
display: Grab buttons with XI2
Comment 63 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:49 UTC
Created attachment 231438 [details] [review]
keybindings: Grab keyboard with XI2
Comment 64 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:52 UTC
Created attachment 231439 [details] [review]
keybindings: Grab keys with XI2
Comment 65 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:14:58 UTC
Created attachment 231440 [details] [review]
Select for XI2 events everywhere else

In random places that are not grabs, we selected for events on
things like the root window, stage window, COW and more. Switch
these over to using the proper XI2 APIs.
Comment 66 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:15:02 UTC
Created attachment 231441 [details] [review]
display: Add spew for XI2
Comment 67 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:15:05 UTC
Created attachment 231442 [details] [review]
ui: Use XI2 to fake GDK events

This removes our final dependency on Core Events, meaning
we can remove support code for them soon.

This commit is a bit ugly as it requires ui having a dependency on
core, but this is already a hack, and two hacks make a right or
however that goes.
Comment 68 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:15:08 UTC
Created attachment 231443 [details] [review]
Remove support for Core Events
Comment 69 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:15:12 UTC
Created attachment 231444 [details] [review]
keybindings: Fix whitespace and alignment
Comment 70 Jasper St. Pierre (not reading bugmail) 2012-12-13 02:23:42 UTC
This patchset should address most issues.
Comment 71 Owen Taylor 2012-12-13 14:35:13 UTC
Review of attachment 231425 [details] [review]:

OK as before
Comment 72 Owen Taylor 2012-12-13 14:43:26 UTC
(In reply to comment #49)
> (In reply to comment #31)
> > @@ +2621,3 @@
> >                        MetaScreen     *screen,
> >                        MetaWindow     *window,
> > +                      XIDeviceEvent *xev,
> > 
> > Alignment got messed up for all of these
> 
> Alignment was already messed up for some recent keybindings work, so how do you
> feel about a post-XI2 "keybindings: Fix alignment" patch?

Alignment of function parameters is something that I care about only peripherally - it's generally not worth disturbing git blame output for. So, if you are going through these places again and changing XIDeviceEvent *xev to XIDeviceEvent *event, I'd like to see that keep alignment, but a separate patch just about alignment isn't worth it.
Comment 73 Owen Taylor 2012-12-13 14:43:55 UTC
(In reply to comment #48)
> (In reply to comment #43)
> > Review of attachment 229869 [details] [review] [details]:
> > 
> > ::: src/core/screen.c
> > @@ +736,3 @@
> >    /* We need to or with the existing event mask since
> >     * gtk+ may be interested in other events.
> >     */
> > 
> > the fact that you changed the code to actually match this comment is probably
> > worth a mentin in the commit message
> 
> How was it not matching before? It was ORing in your_event_mask, just like it
> is now.

Sorry, just misread the old code, was fine.
Comment 74 Owen Taylor 2012-12-13 14:50:55 UTC
Review of attachment 231426 [details] [review]:

Good
Comment 75 Owen Taylor 2012-12-13 14:51:40 UTC
Review of attachment 231427 [details] [review]:

OK
Comment 76 Owen Taylor 2012-12-13 16:17:12 UTC
Review of attachment 231429 [details] [review]:

OK
Comment 77 Owen Taylor 2012-12-13 16:17:37 UTC
Review of attachment 231430 [details] [review]:

OK
Comment 78 Owen Taylor 2012-12-13 16:30:31 UTC
Review of attachment 231428 [details] [review]:

Comment needs fixing - there is no meta_input_event_get_type() in your patchset. Otherwise good to commit
Comment 79 Owen Taylor 2012-12-13 16:33:27 UTC
Review of attachment 231431 [details] [review]:

Looks good
Comment 80 Owen Taylor 2012-12-13 16:53:36 UTC
Review of attachment 231432 [details] [review]:

Going to assume you did all the mechanical renaming right (thanks!) - not re-reviewing line-by-line
Comment 81 Owen Taylor 2012-12-13 16:56:19 UTC
Review of attachment 231433 [details] [review]:

::: src/core/display.c
@@ +1828,3 @@
         case XI_Leave:
+          if (((XIEnterEvent *) input_event)->deviceid == VIRTUAL_CORE_KEYBOARD_ID)
+            return input_event;

Wouldn't Enter/Leave be for the pointer, not the keyboard?
Comment 82 Owen Taylor 2012-12-13 16:57:30 UTC
Review of attachment 231434 [details] [review]:

Looks good
Comment 83 Owen Taylor 2012-12-13 16:58:22 UTC
Review of attachment 231435 [details] [review]:

Still OK
Comment 84 Owen Taylor 2012-12-13 19:22:49 UTC
Review of attachment 231436 [details] [review]:

I checked this over carefully, and I'm pretty convinced that a) we're guaranteed to succeed with the overgrab if we already have the grab b) the end effect is the same - with one caveat related to the comment in the code in window.c:
 
      /* FIXME: Using CurrentTime is really bad mojo */
      timestamp = CurrentTime;
      meta_display_set_grab_op_cursor (window->display,
                                       NULL,
                                       window->display->grab_op,
                                       TRUE,
                                       window->display->grab_xwindow,
                                       timestamp);

XChangeActivePointerGrab() never adjusts the timestamp of the grab - the timestamp is used only to see if changing the grab is allowed. Since we're supposed to have the grab at this point, using CurrentTime and always changing the grab should be fine. So the bad mojo is limited. However, overgrabbing *does* change the timestamp, and using CurrentTime will update the timestamp with current server time. At this point, if we try to grab again with a timestamp from an event that was generated before we changed the cursor, that will fail.

It's hard to come up with a scenario that matters (maybe if you are in a keyboard resizing, and terminate it again, and then use another Metacity keybinding, all really quickly)- but why don't we make it a non-issue by adding grab_timestamp and removing the comment about bad mojo. This patch definitely needs testing that cursor changing during keyboard resizing is still working as intented.
Comment 85 Owen Taylor 2012-12-13 19:30:17 UTC
Review of attachment 231437 [details] [review]:

Still good
Comment 86 Owen Taylor 2012-12-13 19:30:53 UTC
Review of attachment 231438 [details] [review]:

Still good
Comment 87 Owen Taylor 2012-12-13 19:31:41 UTC
Review of attachment 231439 [details] [review]:

Still good
Comment 88 Owen Taylor 2012-12-13 19:32:37 UTC
Review of attachment 231440 [details] [review]:

Looks good
Comment 89 Owen Taylor 2012-12-13 19:37:54 UTC
Review of attachment 231441 [details] [review]:

::: src/core/display.c
@@ +3221,1 @@
+  /* TODO: extra */

Hmm, the Enter/Leave/FocusIn/FocusOut details are probably a big reason why someone would use the spew functionality at all .. can you just move the strdup_printfs over? I'm 100% fine if you are only spewing the data that was in the X event and not all the extra detail from the XI event.
Comment 90 Owen Taylor 2012-12-13 19:38:24 UTC
Review of attachment 231442 [details] [review]:

Still OK
Comment 91 Owen Taylor 2012-12-13 19:39:09 UTC
Review of attachment 231443 [details] [review]:

Still OK (but see comment on spew patch)
Comment 92 Owen Taylor 2012-12-13 19:42:54 UTC
Review of attachment 231444 [details] [review]:

OK despite affect on 'git blame' - keybindings.c was pretty messy
Comment 93 Jasper St. Pierre (not reading bugmail) 2012-12-13 21:40:26 UTC
Attachment 231425 [details] pushed as dd4e655 - Require XInput2
Attachment 231426 [details] pushed as f0c1e39 - display: Initialize XInput2
Attachment 231427 [details] pushed as 8bf8f3e - common: Add the XInput2 headers to common.h, which everything uses
Attachment 231428 [details] pushed as 129c729 - display: separate input/non-input events handling in the event callback
Attachment 231430 [details] pushed as 6b31bd4 - display: Don't pass an event to a handler that will no-op
Attachment 231431 [details] pushed as 881d256 - window: Move grab op sync handling code out


Moving the "core: enable XInput2 by default" commit further in the stack, and
fixing the commit message for "display: separate input/non-input events"
handling in the event callback, this set of patches should hopefully be
harmless to commit right now.
Comment 94 Jasper St. Pierre (not reading bugmail) 2012-12-13 23:19:46 UTC
Attachment 231429 [details] pushed as 946a42f - core: enable XInput2 by default
Attachment 231432 [details] pushed as 1d82704 - Port mutter to use XInput2 events instead of Core Events
Attachment 231433 [details] pushed as 0fd4059 - display: Only care about input events for the VCP/VCK
Attachment 231434 [details] pushed as 55251aa - compositor: Use XInput2 to grab the pointer/keyboard
Attachment 231435 [details] pushed as 8931b80 - compositor: Identify XI2 events as grabbed events
Attachment 231436 [details] pushed as c1b8e0a - display: Use XInput2 to grab the pointer
Attachment 231437 [details] pushed as 8fb9e00 - display: Grab buttons with XI2
Attachment 231438 [details] pushed as afcdfd1 - keybindings: Grab keyboard with XI2
Attachment 231439 [details] pushed as 7c20621 - keybindings: Grab keys with XI2
Attachment 231440 [details] pushed as 774ceec - Select for XI2 events everywhere else
Attachment 231441 [details] pushed as 945c530 - display: Add spew for XI2
Attachment 231442 [details] pushed as c1ac9d1 - ui: Use XI2 to fake GDK events
Attachment 231443 [details] pushed as 6139bc7 - Remove support for Core Events
Attachment 231444 [details] pushed as 2fcbc46 - keybindings: Fix whitespace and alignment




After talking with Owen, I fixed up the code to meet the review comments, and pushed.

Please file regressions in new bugs.
Comment 95 Jasper St. Pierre (not reading bugmail) 2013-02-09 20:22:03 UTC
*** Bug 681082 has been marked as a duplicate of this bug. ***