keyboard: do not send any press events which were not sent to the client

[ammen99]

This was erroneously removed in #2113.

@lilydjwg Do you remember why you removed this check? It should definitely be there, because otherwise we send release events to clients who have not received enter events, this is a quick recipe for messing up the client state. I have verified that without this check Wayfire sends extra release events so the check is needed for sure. If this breaks something for IMs, we'll have to figure out a better way to solve the particular problem.

Fixes #2166

[lilydjwg]

This is removed in 82275d4. I've just retested, and find the following issues:

• in any window with text focus, press Atl-Tab to activate the switcher plugin and release. The switcher plugin remains active.

This is caused by the following sequence of events:

• user presses down alt
• input method receives alt down, and resends it to pass through
• user presses down tab
• switcher activates
• losing focus, input method releases the pressed alt key; this clears it from pressed_keys
• user releases tab and alt
• the alt release by user is ignored

One reason I removed it is the following sequence of events (this describes what happened before I removed the code, not current behavior; it seems to have changed):

• user presses down some key without text focus
• application receives the key and a text focus is now active so the input method comes in
• user releases the key
• the input method receives the release, and realizes that the application should receive it. so it sends the release via virtual keyboard

With the return, the key release sent by input method will be ignored, and thus not received by application. The application will consider the key pressed and trigger key repeat.

[ammen99]

• losing focus, input method releases the pressed alt key; this clears it from pressed_keys

I thought fcitx5-git was supposed to not do this anymore?? Also if this were to happen, wouldn't switcher exit immediately and not remain active?

[ammen99]

I thought fcitx5-git was supposed to not do this anymore?? Also if this were to happen, wouldn't switcher exit immediately and not remain active?

Ok nvm I figured out how to reproduce, now let's figure out what causes it ...

[lilydjwg]

I thought fcitx5-git was supposed to not do this anymore?

It should be a bug in fcitx5. After fixing it, this issue is gone. I'll try some days to see if there are other issues.

[ammen99]

It should be a bug in fcitx5. After fixing it, this issue is gone. I'll try some days to see if there are other issues.

Nice catch, thanks for figuring it out ;)

[ammen99]

@lilydjwg Now the switcher test seems to be working even with the whole grab thing reverted. I'll push it to this branch, let me know if it breaks something. If it does, I'll write more tests so that we can catch such bugs automatically in the future :)

[lilydjwg]

OK, I'll test it soon.

[ammen99]

I accidentally broke IM popup positioning in master but I pushed a fix to this branch so it should work fine I hope.

[ammen99]

Actually, I am confused. Now it seems like the last commit makes things worse :(

[lilydjwg]

I haven't tested the last commit yet I think we need to go back if we want to work with upstream fcitx5, which has a commit that recreates the virtual keyboard every time it has text focus. It brings back those key release events.

In the long run to avoid these hacks, we may have to give up input method v2 and use input wayland v1 instead. Kwin uses input method v1 and it doesn't have any of these issues.

[ammen99]

I haven't tested the last commit yet I think we need to go back if we want to work with upstream fcitx5, which has a commit that recreates the virtual keyboard every time it has text focus. It brings back those key release events.

:(((

I feel like we're chasing a moving target and I'm not really a big fan of this.

In the long run to avoid these hacks, we may have to give up input method v2 and use input wayland v1 instead. Kwin uses input method v1 and it doesn't have any of these issues.

+100000000 if it somehow manages to avoid these race conditions.

[ammen99]

I looked briefly at input-method-v1, might be worth supporting that instead of -v2. After all, -v2 is not even officially in wayland-protocols by the looks of it.

[ammen99]

I have some time this evening so I'll try to write a quick prototype for the -v1 protocol and see how it goes.

[ammen99]

Actually, trying to see how input-method-v1 works made me realize how we might fix input-method-v2. The serials that we have in the protocol basically allow us to almost emulate the same behavior as activation contexts from v1. I tried to implement my idea in the last commit. Hopefully nothing is broken xD

[ammen99]

Also, an explanation of how things 'should' work afaiu:

• For bindings, we should never need to use the keys coming from the virtual keyboard of the IM. That is what was causing us a ton of headaches before.
• Even if a key press is swallowed by the IM, we should nonetheless keep track of pressed/released keys. Rationale: if we were to switch the focus, the new app should get keyboard-enter with the actual physical keys pressed, no matter that before that the keys were swallowed by the IM.
• Therefore, we basically have to send key presses from the IM directly to the client, if the serials are matching (i.e serial is >= than the last focus switch serial), otherwise, we know that the key press belongs to 'an older context' and we can ignore it.

[ammen99]

Actually, it still doesn't work for some cases :((( I guess I should just get -v1 to work.

[lilydjwg]

I haven't got time to test, but there is another issue from fcitx5's change: every time it creates a virtual keyboard, it sends a keymap & repeat_info. This causes wlroots to broadcast them to all clients, and xwayland will spam the log with xkb warnings. I don't want them: I may be able to discard xwayland's stderr, but those repeated broadcast events will cause a lot of noise when debugging.

[lilydjwg]

With v2 there are two keyboards fighting each other. It's tricky to get things right. In v1 there is only one keyboard so it should be easier to handle.

[ammen99]

Im-v1 is implemented in #2172

[ammen99]

Superseded by #2172