Created attachment 212862 [details] [review]
display: Skip XKB keyboard changes events when more are queued

Since the X server sometimes spams us with this kind of events, we
should skip them until the last one we see in the queue before doing
the actual work which is expensive in itself already.

Created attachment 213308 [details] [review]
display: Batch processing of XKB keyboard changes

Since the X server sometimes spams us with this kind of events, we
should skip them and do the actual work, which is expensive in itself,
only once per batch.

--

Owen suggested this as a cleaner/safer way to achieve the same.

Review of attachment 213308 [details] [review]:

Code looks mostly good to me, not so sure about the commit message - "batching XKB keyboard changes" still suggests that each change is processed, maybe something like "Compress consecutive XKB keyboard changes" is a better fit? Also "this kind of events" => "these kinds of events"

::: src/core/display.c
@@ +2685,3 @@
 	      break;
             case XkbNewKeyboardNotify:
             case XkbMapNotify:

A small comment why we are doing this would be helpful. It might also be a good idea to mention that meta_display_process_mapping_event() only uses the event type (not xkb_type, time, ...), which is why it's safe to just throw away excess events.

@@ +2690,3 @@
+                  display->xkb_event = g_memdup (event, sizeof (XkbAnyEvent));
+                  display->xkb_idle_event_handler_id =
+                    g_idle_add (xkb_idle_process_event, display);

I'd consider using
   g_idle_add_full (G_PRIORITY_DEFAULT_IDLE,
                    xkb_idle_process_event,
                    g_memdup (event, sizeof (XkbAnyEvent)),
                    g_free)

You wouldn't need to add xkb_event (MetaDisplay is a singleton, so you can just use meta_get_display() in the handler) and you don't have to remember to free the data when you call g_source_remove() without also calling xkb_idle_process_event() (which is something you should actually do in meta_display_close())

Can I ask the obligatory "why do we get so many events"?

Created attachment 224418 [details] [review]
display: Only process keyboard mapping events for the core X keyboard

The X server sends a XkbNewKeyboardNotify event for each keyboard
device when a new keyboard description is loaded. These days a typical
computer has several keyboard devices, e.g. xinput on this laptop
lists 8. Since the work we do on these events is relatively expensive
and we are only really interested in changes to the virtual core
keyboard we can skip other devices' events to cut on needless work.
--

Thanks for keeping me honest Jasper :-)

It would be better if we could tell the server to send us only events
for the core keyboard device but I didn't find a way to do that so I
think this is the best we can do.

My apologies for wasting your time Florian.

Whoa. I was expecting an answer like "because the X server is dumb, I filed a bug here" or "that's expected behavior"

Review of attachment 224418 [details] [review]:

This is obviously going to have to change for XI2, so I'm curious if we'll reintroduce the lag then there, or if we can make it device-specific then.

This works for me, minus one minor nit (which results in code removal, yay!)

::: src/core/keybindings.c
@@ +4220,3 @@
+#ifdef HAVE_XKB
+static guint
+get_core_keyboard_id (MetaDisplay *display)

The VCK ID is going to always be 3, unless this is doing something I'm missing. It's hardcoded in a bunch of libraries like GDK, so it's never going to change.

(In reply to comment #7)
> This is obviously going to have to change for XI2, so I'm curious if we'll
> reintroduce the lag then there, or if we can make it device-specific then.

I don't think we'll have to change anything for XI2 unless we want to
start supporting different layouts per keyboard device.

> This works for me, minus one minor nit (which results in code removal, yay!)
>
> ::: src/core/keybindings.c
> @@ +4220,3 @@
> +#ifdef HAVE_XKB
> +static guint
> +get_core_keyboard_id (MetaDisplay *display)
>
> The VCK ID is going to always be 3, unless this is doing something I'm missing.
> It's hardcoded in a bunch of libraries like GDK, so it's never going to change.

Ok, thanks.

Attachment 224418 [details] pushed as 4cf461f - display: Only process keyboard mapping events for the core X keyboard