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

wayland v2: don't release pressed keys without user input #954

Closed
lilydjwg opened this issue Jan 23, 2024 · 11 comments
Closed

wayland v2: don't release pressed keys without user input #954

lilydjwg opened this issue Jan 23, 2024 · 11 comments

Comments

@lilydjwg
Copy link
Contributor

Describe the bug
6f92a0c introduced the idea to record and release pressed keys on ungrab. This caused a lot of issues when I was trying to workaround and ignore them (see e.g. WayfireWM/wayfire#2111).

Under sway, if a text input client exit upon a key press and the next client is XWayland, the key release will not be triggered and xwayland will produce key repitition.

This doesn't seem to happen anymore.

To Reproduce

  1. Try to implement a Wayland compositor with global shortcuts that handles this correctly

Expected behavior
No superfluous key release event, so it's simpler to reason about and deal with. E.g. the issues mentioned here lilydjwg/wayfire@f036d01 just disappear by itself.

Desktop (please complete the following information):

  • Desktop: Wayfire
  • Display server type: Wayland

Additional context

@lilydjwg
Copy link
Contributor Author

lilydjwg commented Jan 23, 2024

Reproduced the issue with wine notepad. Will investigate.

Edit: suddenly it no longer reproduces....

@wengxt
Copy link
Member

wengxt commented Jan 23, 2024

Such issue still happens under hyprland. I once tried to revert it but was reverted soon.

@wengxt wengxt closed this as completed Jan 23, 2024
@lilydjwg
Copy link
Contributor Author

Could there be an option to change it?

@ammen99
Copy link

ammen99 commented Jan 24, 2024

@wengxt it would be great to have an option to disable it. Right now fcitx5 has these release events as a workaround for hyprland bugs, but that breaks things on wayfire so now wayfire also has to include a workaround for the workaround that fcitx5 has for hyprland ...

@wengxt
Copy link
Member

wengxt commented Jan 24, 2024

@ammen99 compositor should be working for whatever bad thing that client does. Another thing is all these protocol doesn't really have a clear thinking about the issues.. And also, in order to support key repetition, certain fake key release must be sent.

This is an article that describes such things.
https://www.csslayer.info/wordpress/linux/key-repetition-and-key-event-handling-issue-with-wayland-input-method-protocols/

But, come to think of it, if certain key does not repeat at all (this can be queried), actually I can make fcitx not release to it.

@ammen99
Copy link

ammen99 commented Jan 24, 2024

I quickly went through your blog post and I'm wondering, why do you have to actually forward backspaces to the client (including repeat), can you not delete the surrounding text with the text-input-v3 protocol with requests from fcitx5, without the client ever knowing that there were backspaces pressed?

@wengxt
Copy link
Member

wengxt commented Jan 24, 2024

I quickly went through your blog post and I'm wondering, why do you have to actually forward backspaces to the client (including repeat), can you not delete the surrounding text with the text-input-v3 protocol with requests from fcitx5, without the client ever knowing that there were backspaces pressed?

@ammen99
First of all, surrounding text support is extremely poor and buggy. I would try to avoid use it unless it is a must for certain features.
https://www.csslayer.info/wordpress/fcitx-dev/why-surrounding-text-is-the-worst-feature-in-the-linux-input-method-world/

Even if we don't consider that, certain application, specifically, terminal (alacritty, konsole, etc) can't do surrounding text. The concept of surrounding text just does not exists there. Imagine that you're using vim in a terminal and you can't do a long press backspace to delete text, that's not acceptable.

@wengxt
Copy link
Member

wengxt commented Jan 24, 2024

Also, backspace is just one example, there are many other different keys: Like page down, page up, arrow keys those are not covered by text-input-v3. The keys that produce text can be converted to commit_string, but you know, "commit_string" and key are actually go though different codepath on the application side. Which may cause totally different behavior in application. The application is legit to implement the code as

onKeyPressed() {
   if (key is A) {
       ...
   }
}

onTextCommit(text) {
    // Do they need to check if (text == "A") and use the same logic like onKeyPress?
    // If they do, does that even make sense? For example, the "A" can legally comes from a spell
   // checker candidate from input method, which is not "press A" and user won't 
   //expect it to be same as press "A".
}

@ammen99
Copy link

ammen99 commented Jan 24, 2024

I see, good points, thanks for the explanation.

The actual bug we were observing in Wayfire is actually that fcitx5 would send release keys after it is deactivated (for example user pressed alt-tab and then fcitx5 sends alt release which stops the switcher in wayfire). But I guess this also can't be avoided since deactivation could also happen for some other random reason ... This is indeed a huge mess :( I'll see what's the best we can do on Wayfire's side.

@wengxt
Copy link
Member

wengxt commented Jan 25, 2024

@ammen99 can you also check the discussion here applies to wayfire?

341ae16#commitcomment-125121371

@ammen99
Copy link

ammen99 commented Jan 25, 2024

@ammen99 can you also check the discussion here applies to wayfire?

341ae16#commitcomment-125121371

I went through it but I'm not sure I really understood what the discussion was all about. Anyway with the modifiers, yes, if you press <alt> on the vkbd, it will remain pressed (also when switching focus) to the new app, until the alt key is released. Afaik this is how things are supposed to be.

wengxt added a commit that referenced this issue Jan 26, 2024
This may help resolve some compositor issue when they are not intended
to use vk's key to update compositor internal state, especifically,
modifiers keys.
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

No branches or pull requests

3 participants