Merge pull request #1193 from vancluever/vancluever/gtk-x11-modifier-state

apprt/gtk: ensure modifier state matches current keypress under X11

src/apprt/gtk/App.zig

@@ -28,6 +28,7 @@ const ClipboardConfirmationWindow = @import("ClipboardConfirmationWindow.zig");
 const c = @import("c.zig");
 const inspector = @import("inspector.zig");
 const key = @import("key.zig");
+const x11 = @import("x11.zig");
 const testing = std.testing;
 
 const log = std.log.scoped(.gtk);
@@ -55,6 +56,9 @@ clipboard_confirmation_window: ?*ClipboardConfirmationWindow = null,
 /// This is set to false when the main loop should exit.
 running: bool = true,
 
+/// Xkb state (X11 only). Will be null on Wayland.
+x11_xkb: ?x11.Xkb = null,
+
 pub fn init(core_app: *CoreApp, opts: Options) !App {
     _ = opts;
 
@@ -169,8 +173,13 @@ pub fn init(core_app: *CoreApp, opts: Options) !App {
         return error.GtkApplicationRegisterFailed;
     }
 
+    // Perform all X11 initialization. This ultimately returns the X11
+    // keyboard state but the block does more than that (i.e. setting up
+    // WM_CLASS).
+    const x11_xkb: ?x11.Xkb = x11_xkb: {
         const display = c.gdk_display_get_default();
-    if (c.g_type_check_instance_is_a(@ptrCast(@alignCast(display)), c.gdk_x11_display_get_type()) != 0) {
+        if (!x11.is_display(display)) break :x11_xkb null;
+
         // Set the X11 window class property (WM_CLASS) if are are on an X11
         // display.
         //
@@ -196,7 +205,10 @@ pub fn init(core_app: *CoreApp, opts: Options) !App {
             "ghostty";
         c.g_set_prgname(prgname);
         c.gdk_x11_display_set_program_class(display, app_id);
-    }
+
+        // Set up Xkb
+        break :x11_xkb try x11.Xkb.init(display);
+    };
 
     // This just calls the "activate" signal but its part of the normal
     // startup routine so we just call it:
@@ -209,6 +221,7 @@ pub fn init(core_app: *CoreApp, opts: Options) !App {
         .config = config,
         .ctx = ctx,
         .cursor_none = cursor_none,
+        .x11_xkb = x11_xkb,
         // If we are NOT the primary instance, then we never want to run.
         // This means that another instance of the GTK app is running and
         // our "activate" call above will open a window.
@@ -573,3 +586,10 @@ test "isValidAppId" {
     try testing.expect(!isValidAppId(""));
     try testing.expect(!isValidAppId("foo" ** 86));
 }
+
+/// Loads keyboard state from Xkb if there is an event pending and Xkb is
+/// loaded (X11 only). Returns null otherwise.
+pub fn modifier_state_from_xkb(self: *App, display_: ?*c.GdkDisplay) ?input.Mods {
+    const x11_xkb = self.x11_xkb orelse return null;
+    return x11_xkb.modifier_state_from_notify(display_);
+}

src/apprt/gtk/Surface.zig

@@ -1222,6 +1222,7 @@ fn keyEvent(
     const self = userdataSelf(ud.?);
     const keyval_unicode = c.gdk_keyval_to_unicode(keyval);
     const event = c.gtk_event_controller_get_current_event(@ptrCast(ec_key));
+    const display = c.gtk_widget_get_display(@ptrCast(self.gl_area));
 
     // Get the unshifted unicode value of the keyval. This is used
     // by the Kitty keyboard protocol.
@@ -1232,7 +1233,6 @@ fn keyEvent(
 
         // Get all the possible keyboard mappings for this keycode. A keycode
         // is the physical key pressed.
-        const display = c.gtk_widget_get_display(@ptrCast(self.gl_area));
         var keys: [*]c.GdkKeymapKey = undefined;
         var keyvals: [*]c.guint = undefined;
         var n_keys: c_int = 0;
@@ -1333,9 +1333,15 @@ fn keyEvent(
     // was presssed (i.e. left control)
     const mods = mods: {
         _ = gtk_mods;
-
         const device = c.gdk_event_get_device(event);
-        var mods = translateMods(c.gdk_device_get_modifier_state(device));
+
+        // Add any modifier state events from Xkb if we have them (X11 only).
+        // Null back from the Xkb call means there was no modifier
+        // event to read. This likely means that the key event did not
+        // result in a modifier change and we can safely rely on the
+        // GDK state.
+        var mods = self.app.modifier_state_from_xkb(display) orelse
+            translateMods(c.gdk_device_get_modifier_state(device));
         mods.num_lock = c.gdk_device_get_num_lock_state(device) == 1;
 
         switch (physical_key) {

src/apprt/gtk/c.zig

@@ -5,6 +5,8 @@ const c = @cImport({
     // Add in X11-specific GDK backend which we use for specific things (e.g.
     // X11 window class).
     @cInclude("gdk/x11/gdkx.h");
+    // Xkb for X11 state handling
+    @cInclude("X11/XKBlib.h");
 });
 
 pub usingnamespace c;

src/apprt/gtk/x11.zig

@@ -0,0 +1,144 @@
+/// Utility functions for X11 handling.
+const std = @import("std");
+const c = @import("c.zig");
+const input = @import("../../input.zig");
+
+const log = std.log.scoped(.gtk_x11);
+
+/// Returns true if the passed in display is an X11 display.
+pub fn is_display(display: ?*c.GdkDisplay) bool {
+    return c.g_type_check_instance_is_a(
+        @ptrCast(@alignCast(display orelse return false)),
+        c.gdk_x11_display_get_type(),
+    ) != 0;
+}
+
+pub const Xkb = struct {
+    base_event_code: c_int,
+    funcs: Funcs,
+
+    /// Initialize an Xkb struct, for the given GDK display. If the display
+    /// isn't backed by X then this will return null.
+    pub fn init(display_: ?*c.GdkDisplay) !?Xkb {
+        // Display should never be null but we just treat that as a non-X11
+        // display so that the caller can just ignore it and not unwrap it.
+        const display = display_ orelse return null;
+
+        // If the display isn't X11, then we don't need to do anything.
+        if (!is_display(display)) return null;
+
+        log.debug("Xkb.init: initializing Xkb", .{});
+        const xdisplay = c.gdk_x11_display_get_xdisplay(display);
+        var result: Xkb = .{
+            .base_event_code = 0,
+            .funcs = try Funcs.init(),
+        };
+
+        log.debug("Xkb.init: running XkbQueryExtension", .{});
+        var opcode: c_int = 0;
+        var base_error_code: c_int = 0;
+        var major = c.XkbMajorVersion;
+        var minor = c.XkbMinorVersion;
+        if (result.funcs.XkbQueryExtension(
+            xdisplay,
+            &opcode,
+            &result.base_event_code,
+            &base_error_code,
+            &major,
+            &minor,
+        ) == 0) {
+            log.err("Fatal: error initializing Xkb extension: error executing XkbQueryExtension", .{});
+            return error.XkbInitializationError;
+        }
+
+        log.debug("Xkb.init: running XkbSelectEventDetails", .{});
+        if (result.funcs.XkbSelectEventDetails(
+            xdisplay,
+            c.XkbUseCoreKbd,
+            c.XkbStateNotify,
+            c.XkbModifierStateMask,
+            c.XkbModifierStateMask,
+        ) == 0) {
+            log.err("Fatal: error initializing Xkb extension: error executing XkbSelectEventDetails", .{});
+            return error.XkbInitializationError;
+        }
+
+        return result;
+    }
+
+    /// Checks for an immediate pending XKB state update event, and returns the
+    /// keyboard state based on if it finds any. This is necessary as the
+    /// standard GTK X11 API (and X11 in general) does not include the current
+    /// key pressed in any modifier state snapshot for that event (e.g. if the
+    /// pressed key is a modifier, that is not necessarily reflected in the
+    /// modifiers).
+    ///
+    /// Returns null if there is no event. In this case, the caller should fall
+    /// back to the standard GDK modifier state (this likely means the key
+    /// event did not result in a modifier change).
+    pub fn modifier_state_from_notify(self: Xkb, display_: ?*c.GdkDisplay) ?input.Mods {
+        const display = display_ orelse return null;
+
+        // Shoutout to Mozilla for figuring out a clean way to do this, this is
+        // paraphrased from Firefox/Gecko in widget/gtk/nsGtkKeyUtils.cpp.
+        const xdisplay = c.gdk_x11_display_get_xdisplay(display);
+        if (self.funcs.XEventsQueued(xdisplay, c.QueuedAfterReading) == 0) return null;
+
+        var nextEvent: c.XEvent = undefined;
+        _ = self.funcs.XPeekEvent(xdisplay, &nextEvent);
+        if (nextEvent.type != self.base_event_code) return null;
+
+        const xkb_event: *c.XkbEvent = @ptrCast(&nextEvent);
+        if (xkb_event.any.xkb_type != c.XkbStateNotify) return null;
+
+        const xkb_state_notify_event: *c.XkbStateNotifyEvent = @ptrCast(xkb_event);
+        // Check the state according to XKB masks.
+        const lookup_mods = xkb_state_notify_event.lookup_mods;
+        var mods: input.Mods = .{};
+
+        log.debug("X11: found extra XkbStateNotify event w/lookup_mods: {b}", .{lookup_mods});
+        if (lookup_mods & c.ShiftMask != 0) mods.shift = true;
+        if (lookup_mods & c.ControlMask != 0) mods.ctrl = true;
+        if (lookup_mods & c.Mod1Mask != 0) mods.alt = true;
+        if (lookup_mods & c.Mod2Mask != 0) mods.super = true;
+        if (lookup_mods & c.LockMask != 0) mods.caps_lock = true;
+
+        return mods;
+    }
+};
+
+/// The functions that we load dynamically from libX11.so.
+const Funcs = struct {
+    XkbQueryExtension: XkbQueryExtensionType,
+    XkbSelectEventDetails: XkbSelectEventDetailsType,
+    XEventsQueued: XEventsQueuedType,
+    XPeekEvent: XPeekEventType,
+
+    const XkbQueryExtensionType = *const fn (?*c.struct__XDisplay, [*c]c_int, [*c]c_int, [*c]c_int, [*c]c_int, [*c]c_int) callconv(.C) c_int;
+    const XkbSelectEventDetailsType = *const fn (?*c.struct__XDisplay, c_uint, c_uint, c_ulong, c_ulong) callconv(.C) c_int;
+    const XEventsQueuedType = *const fn (?*c.struct__XDisplay, c_int) callconv(.C) c_int;
+    const XPeekEventType = *const fn (?*c.struct__XDisplay, [*c]c.union__XEvent) callconv(.C) c_int;
+
+    pub fn init() !Funcs {
+        var libX11 = try std.DynLib.open("libX11.so");
+        defer libX11.close();
+
+        var result: Funcs = undefined;
+        inline for (@typeInfo(Funcs).Struct.fields) |field| {
+            const name = comptime name: {
+                const null_term = field.name ++ .{0};
+                break :name null_term[0..field.name.len :0];
+            };
+
+            @field(result, field.name) = libX11.lookup(
+                field.type,
+                name,
+            ) orelse {
+                log.err(" error dynamic loading libX11: missing symbol {s}", .{field.name});
+                return error.XkbInitializationError;
+            };
+        }
+
+        return result;
+    }
+};