Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x11: Use modifiers from X event if none were detected by XKB #4151

Closed
wants to merge 1 commit into from

Conversation

bew
Copy link
Contributor

@bew bew commented Aug 15, 2023

This is a working tentative at fixing text expansion/injection done by Espanso by default, where XKB doesn't seem to detect the pressed modifiers Ctrl+Shift before pressing v.

ref: #3840 (comment) (and following comments)

(( So with this patch, XKB's state is not updated and the key event' modifiers are kind of transient: only existing for this event. ))

Edit: waiting for your input on #3840 (comment)

This is a working tentative at fixing text expansion/injection done by
Espanso by default, where XKB doesn't seem to detect the pressed
modifiers `Ctrl+Shift` before pressing `v`.
@bew
Copy link
Contributor Author

bew commented Dec 17, 2023

Hi @wez I'd be interested to know if you'd accept this change as a workaround for Espanso (and maybe other tools) that send mods in the XInput event?

@wez
Copy link
Owner

wez commented Jan 10, 2024

Sorry it's taken a while to get to this.

Today I was screwed over by btrfs and had to reinstall my workstation. As part of that, I switched from Fedora to Ubuntu. My keyboard has a custom "paste" button that sends "shift-Insert" when pressed, and this key misbehaves in wezterm under ubuntu; the shift modifier goes missing from the actual Insert event. It worked fine in Fedora. I don't know what the difference is; both systems are Gnome XOrg systems. There's probably something weird in the Ubuntu configuration that causes this, but I have no desire to investigate it.

Running with your PR seems to fix that, so yes, I am now suddenly motivated to see this PR get finished up! :)

//
// This can happen (FIXME: WHY) for example when Espanso (a text expander/injector)
// simulate inputs.
if let (true, Some(event_xmods)) = (res.is_empty(), fallback_xmods) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing I'm a little unsure about here is how to distinguish between:

  • xkb state knew the modifier state but legitimately produced an empty set
  • xkb has no idea and produced an empty set of mods

I'm not sure if this is purely a theoretical distinction or whether there is something more tricky we need to do in order to be sure about this.

Is there a return code we can use to influence this choice?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe it doesn't matter and we should unconditionally "or" the event-provided modifiers in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do this distinction, but I'd argue it doesn't matter much IMO.

Currently the PR uses the event mods as a fallback only, however I'm thinking it could be different:

  1. when the X event is synthetic (was crafted and sent by another app), use event mods instead of xkb detected mods
  2. when the X event has mods, use event mods instead of xkb detected mods
  3. merge xkb mods and event mods (as you suggested)

Not sure what's the best direction here..
I think the behavior 1 can be a good idea for synthetic events, because if an app sends an xevent to another app it might want a specific set of mods, but at the same time it could also have sent events to indicate mods are pressed followed by the keypress/release event..

Maybe we could do 1 & 2 or 1 & 3, WDYT ?

It would be interesting to have more data about your issue, can you give the output of xev -event keyboard and pressing your paste key?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropping by to share the logging, haven't had a chance to ponder the questions so far.

KeyPress event, serial 28, synthetic NO, window 0x2e00001,
    root 0x6d1, subw 0x0, time 64899989, (300,145), root:(344,1595),
    state 0x0, keycode 50 (keysym 0xffe1, Shift_L), same_screen YES,
    XLookupString gives 0 bytes:
    XmbLookupString gives 0 bytes:
    XFilterEvent returns: False

KeyPress event, serial 28, synthetic NO, window 0x2e00001,
    root 0x6d1, subw 0x0, time 64899989, (300,145), root:(344,1595),
    state 0x1, keycode 118 (keysym 0xff63, Insert), same_screen YES,
    XLookupString gives 0 bytes:
    XmbLookupString gives 0 bytes:
    XFilterEvent returns: False

KeyRelease event, serial 28, synthetic NO, window 0x2e00001,
    root 0x6d1, subw 0x0, time 64899990, (300,145), root:(344,1595),
    state 0x1, keycode 118 (keysym 0xff63, Insert), same_screen YES,
    XLookupString gives 0 bytes:
    XFilterEvent returns: False

KeyRelease event, serial 28, synthetic NO, window 0x2e00001,
    root 0x6d1, subw 0x0, time 64899991, (300,145), root:(344,1595),
    state 0x1, keycode 50 (keysym 0xffe1, Shift_L), same_screen YES,
    XLookupString gives 0 bytes:
    XFilterEvent returns: False

wez added a commit that referenced this pull request Jan 11, 2024
@wez
Copy link
Owner

wez commented Jan 11, 2024

I took a peek at this this morning, and the situation I'm seeing frustrated me: in my case, what is happening is that there is a keyboard state notification from the X server that sometimes clears all the modifier masks prior to delivering the Insert key press. I don't have a good explanation for what is happening or why.

I refactored your PR slightly to make the flow a bit easier to see; now instead of passing around an Option<BtnMask>, Modifiers is computed from the mask in the xcb event handlers and passed in and ORed in with the other modifier bits.

I've pushed this to main; it seems to resolve the issue that I'm facing and the I think the risk for misbehavior is relatively low.
We'll see if anyone surfaces with a complaint!

Thanks!

@wez wez closed this Jan 11, 2024
@bew bew deleted the try-fix-espanso-key-injection branch January 12, 2024 08:52
@bew
Copy link
Contributor Author

bew commented Jan 12, 2024

Much simpler indeed 👍 0466898
So I see you went with merging mods, we'll see if it's right :)

It's weird that this change makes your key work though, I don't see how it can be related ¯\_(ツ)_/¯

bew added a commit to bew/wezterm that referenced this pull request Jan 12, 2024
wez added a commit that referenced this pull request Jan 23, 2024
In discussion about the ongoing weirdness with modifier processing,
it sounds like we could/should be using `update_key` rather than
`update_mask` under X11.  (Wayland: has to use `update_mask`,
per the docs:
https://docs.rs/xkbcommon/latest/xkbcommon/xkb/struct.State.html#method.update_key)

I quickly tried out a proof of concept with this and it
doesn't seem to hurt, but since I'm unsure if there are other
nuances to consider, I've put this behind a config option.

`x11_use_passive_key_updates` defaults to true which results
in the use of `update_mask` on X11.

Setting it to false will switch to using `update_key` instead,
and may improve handling of xkb states.

refs: #4841
refs: ibus/ibus#2600
refs: #4151
wez added a commit that referenced this pull request Jan 23, 2024
in discussion: ibus/ibus#2600 (comment)
the summary is that the StateNotify event which is used to update
the xkd modifier state is sourced directly from the X server and
doesn't account for any state maintained by the IME.

The key press and release events do include the correct state,
so we should always prefer to use the modifiers from those events
on X11.

Under wayland, we continue to use the kbd state.

refs: ibus/ibus#2600
refs: #4151
wez added a commit that referenced this pull request Jan 24, 2024
test scenario is:

```
bash -c "sleep 5; for((i=0;i<30;i++)); do xdotool keydown --delay 0 Shift_L keydown --delay 0 9 keyup --delay 0 Shift_L keyup --delay 0 9; done"
```

That should cause a series of `(` characters to be emitted, but prior to
this commit is was usually mostly `9`'s.

What's changing here is:
* We copy the pertinent fields from the last xcb StateNotify event.
  That ostensibly has the current modifier and layout state, but
  because it comes from the X server, it doesn't factor in knowledge
  from the IME.
* When processing an XCB key event, compute the current modifier
  mask and override the XKB state with it.
* Now XKB will produce correct information about the key syms
* Restore the modifier state from the saved StateNotify information.

refs: #4151
refs: #4615
refs: fcitx/fcitx5#893
refs: ibus/ibus#2600
refs: #3840
bew added a commit to bew/dotfiles that referenced this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants