on_focus_change erroneously invoked on every loop with sway+fcitx5

[mochaaP]

Describe the bug
When Fcitx 5 is enabled on Sway, on_focus_change is repeatedly called due to multiple wl_keyboard's existence. (e.g. on Sway, virtual keyboard and physical keyboard are two different objects.)

To Reproduce
Steps to reproduce the behavior:

1. Install fcitx 5.1.9, Sway 1.9, kitty
2. Run kitty --config NONE --debug-keyboard
3. Activate input method and try focusing different windows
4. See on_focus_change spam in logs, CPU time spike on calls to libxkbcommon

Environment details

kitty 0.34.1 created by Kovid Goyal
Linux thonkbook 6.9.0-0.rc5.20240424git9d1ddab261f3.46.fc41.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Apr 24 15:05:48 UTC 2024 x86_64
S
Kernel 6.9.0-0.rc5.20240424git9d1ddab261f3.46.fc41.x86_64 on an x86_64 (/dev/tty)

Running under: Wayland (swayfx version 0.3.3 (based on sway 1.9.0)) missing: blur
OpenGL: '4.6 (Core Profile) Mesa 24.0.5' Detected version: 4.6
Frozen: False
Paths:
  kitty: /usr/bin/kitty
  base dir: /usr/lib64/kitty
  extensions dir: /usr/lib64/kitty/kitty
  system shell: /usr/bin/fish

Config options different from defaults:

Important environment variables seen by the kitty process:
	PATH                                /opt/cosmo/bin:/home/mochaa/ghq/github.com/google/purr/scripts:/home/mochaa/.local/bin:/home/mochaa/.local/share/solana/install/active_release/bin:/home/mochaa/.local/share/luarocks/bin:/home/mochaa/.local/share/cargo/bin:/home/mochaa/.local/share/go/bin:/home/mochaa/.local/share/cabal/bin:/home/mochaa/.local/share/pnpm/bin:/home/mochaa/.local/share/deno/bin:/home/mochaa/.local/share/bun/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin
	LANG                                en_US.utf8
	EDITOR                              nvim
	SHELL                               /usr/bin/fish
	GLFW_IM_MODULE                      ibus
	DISPLAY                             :0
	WAYLAND_DISPLAY                     wayland-1
	USER                                mochaa
	XCURSOR_SIZE                        24
	XDG_DATA_HOME                       /home/mochaa/.local/share
	XDG_RUNTIME_DIR                     /run/user/1000
	XDG_SESSION_DESKTOP                 sway
	XDG_VTNR                            4
	XDG_STATE_HOME                      /home/mochaa/.local/state
	XDG_SESSION_CLASS                   user
	XDG_CONFIG_HOME                     /home/mochaa/.config
	XDG_SESSION_ID                      5
	XDG_CACHE_HOME                      /home/mochaa/.cache
	XDG_DATA_DIRS                       /home/mochaa/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share/:/usr/share/
	XDG_CURRENT_DESKTOP                 sway
	XDG_SEAT                            seat0
	XDG_SESSION_TYPE                    wayland

Additional context
Notes from fcitx5 developer @/wengxt:

There was a behavior change in fcitx, where it only creates virtual keyboard alongside creating the input method. I don't think this is wrong and this won't be reverted.

Maybe the initial trigger was the second wl_keyboard.enter, because I don't see any other text-input v3 clients doing wl_keyboard.leave on start
So it's almost certainly an issue on kitty's side
Probably the second wl_keyboard.enter triggered kitty's on_focus_change

I think the core issue is text_input's enable/disable should be triggered by text_input_enter / text_input_leave instead of wl_keyboard's enter/leave.

Also it's worth mentioning that multiple wl_keyboard objects could co-exisist.

wl-debug.log

[kovidgoyal]

This is a change in fcitx that breaks existing programs. They need to fix it there. At the very least they should not be making breaking changes in minor updates.

As for kitty, you can simply add

wayland_enable_ime no

to workaround it. IME is a god awful hack, that causes endless bugs, increased latency and even application crashes. That's why it was disabled by default under kitty in X11, maybe I should be doing that in Wayland too. Unless you actually need to type East Asian languages using it, I strongly recommend turning it off, or downgrade fcitx5 to whatever version before they introduced this bug.

As for the actual bug, I have no idea why @wengxt thinks it's a kitty issue. wl_keyboard.leave is an event sent by the compositor not the application. So kitty isnt "doing" it. And text input is not triggered by keyboard enter/leave, see the keyboardHandleEnter and keyboardHandleLeave functions. Text input is enabled/disabled by the tex tinput enter and leave functions, see text_input_enter and text_input_leave in wl_text_input.c. And regardless of the number of keyboards in a system, in Wayland I know of no way other than keyboard enter and leave events to track keyboard focus. If you do, please enlighten me.

Maybe @wengxt can explain some more.

I am closing since I dont see how this is a bug in kitty, but feel free to continue posting, I am open to being convinced.

[wengxt]

On compositor using zwp input method v2, there may be multiple wl_keyboard objects, one represent the real phyiscal keyboard, one represent the “virtual keyboard object” created by input method.

Input method may choose to create, or delete the virtual keyboard object at literally any time. At least it seems on sway, sway will send a wl_keyboard.leave at that time. I think what sway doing makes totally sense.

What kitty doing wrong here, is that kitty assume that wl_keyboard.leave means the window lost focus. and send text-input-v3.enable/disable.

1. wl_keyboard.leave does means window lost focus in some cases, but obviously, not all cases. Especially when there are multiple wl_keyboard objects.
2. there is text-input-v3.enter/leave that kitty should be using to trigger focus change for text-input-v3, not wl_keyboard.leave

[wengxt]

Briefly reading the existing code,

on wayland text-input-v3.enter/leave should be only source of GLFW_IME_UPDATE_FOCUS. You don't really need to match it with keyboard focus, especially it text-input-v3.enter already carries the surface object.

[kovidgoyal]

On Sat, Apr 27, 2024 at 09:46:24PM -0700, Weng Xuetian wrote: On compositor using zwp input method v2, there may be multiple wl_keyboard objects, one represent the real phyiscal keyboard, one represent the “virtual keyboard object” created by input method.

kitty uses text-input-unstable-v3. I assume that's what you mean.

Input method may choose to create, or delete the virtual keyboard object at literally any time. At least it seems on sway, sway will send a wl_keyboard.leave at that time. I think what sway doing makes totally sense.

Then how is any application supposed to track focus? If there are multiple keyboards and they can all focus different things at different times, there is simply no way to know which window actually has keyboard focus.

What kitty doing wrong here, is that kitty assume that wl_keyboard.leave means the window lost focus. and send text-input-v3.enable/disable. 1. wl_keyboard.leave does means window lost focus in some cases, but obviously, not all cases. Especially when there are multiple wl_keyboard objects. 2. there is text-input-v3.enter/leave that kitty should be using to trigger focus change for text-input-v3, not wl_keyboard.leave

I dont see how that is supposed to work. What if the user hasnt enabled IME? kitty cannot rely on IME for focus events. Indeed, where in the Wayland spec does it say focus is meant to be tracked by IME enter/leave events? text input is an optional protocol. Window focus is a core feature, it cannot depend on an optional protocol.

[kovidgoyal]

On Sat, Apr 27, 2024 at 09:54:00PM -0700, Weng Xuetian wrote: Briefly reading the existing code, on wayland text-input-v3.enter/leave should be only source of GLFW_IME_UPDATE_FOCUS. You don't really need to match it with keyboard focus, especially it text-input-v3.enter already carries the surface name.

So you are saying that basically, only in Wayland, we now have two kinds of focus actual keyboard focus and text input method focus? That makes no sense. Window focus *cannot* depend on an unstable, optional text input protocol. And updating IME state on window focus change is a perfectly reasonable thing to do that causes no issues on anything other than Wayland+fcitx5 with your recent commit to fcitx5. I strongly suggest you consider reverting it and find another way to accomplish whatever you were trying to accomplish with it. If you expect wayland applications to rely on text input state to manage focus, then you need to get some core wayland specs updated to say so.

[wengxt]

Sorry I was confused by some app that bind two seat.

Yeah, there's only one wl_keyboard, but maybe then it's sway to be blamed, when there's no focus change but just virtual keyboard got destroyed and it send a wl_keyboard.enter/leave.

Though I can see why sway's doing this, because it want to clear the key pressed state that was once associated with previous keyboard.

But still, the my statement about text-input-v3.enter still holds. IME focus, and keyboard focus are two separate things. If the event you are calling is IME_FOCUS change, then you should stick to ime focus. Or, if you want to assume the keyboard focus is a prerequisite of ime focus, actually that's also fine and is what compositor do. The easiest fix for the issue would be

_glfwPlatformUpdateIMEState, remove the enable/disable in case GLFW_IME_UPDATE_FOCUS, only keep them as a reply to text-input-v3.enter/leave.

hande_text_input_enter() {
     text_input_focus = true
     // keep existing code here
     // send_enter & commit
}

hande_text_input_leave() {
     text_input_focus = false
     // keep existing code here
}

_glfwPlatformUpdateIMEState
case GLFW_IME_UPDATE_FOCUS:
    if text_input_focus: 
        // Remove the enable/disable() here
        set_content_type .... etc

[kovidgoyal]

I am OK with changing GLFW_IME_UPDATE_FOCUS to rely on the text
input focus rather than the window focus. That seems harmless, since it
only affects IME handling, and wnt break anything for people not using
the IME.