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

input-method-v1 #2172

Merged
merged 13 commits into from
Mar 9, 2024
Merged

input-method-v1 #2172

merged 13 commits into from
Mar 9, 2024

Conversation

ammen99
Copy link
Member

@ammen99 ammen99 commented Mar 2, 2024

The implementation is very incomplete atm, hence the draft status.

I still need to figure out how to combine the data from input-method-v1 events and text-input-v3, because the latter uses slightly different format for some requests.

@ammen99 ammen99 mentioned this pull request Mar 5, 2024
@ammen99 ammen99 marked this pull request as ready for review March 5, 2024 22:52
@ammen99
Copy link
Member Author

ammen99 commented Mar 5, 2024

@lilydjwg I have finished a first version of the im-v1 protocol. From my limited testing, it seems to work fine, including when changing between layouts, switchers, etc. But I am usually not using fcitx5 so I am sure I have missed something, please test when you have time and let me know what still needs to be fixed.

@lilydjwg
Copy link
Contributor

lilydjwg commented Mar 6, 2024

That's really quick!

I've done a quick test. Basic functionality works, but it has several issues, like restart fcitx5 crashes wayfire, and interaction with switcher & vswitcher plugins. I'll see if I can fix it.

@ammen99
Copy link
Member Author

ammen99 commented Mar 6, 2024

. Basic functionality works, but it has several issues, like restart fcitx5 crashes wayfire, and interaction with switcher & vswitcher plugins. I'll see if I can fix it.

Restarting fcitx5 was easy enough to fix, I just pushed a commit. For switcher & vswitch, you're right. I tested it and it seemed to work but somehow fcitx5 doesn't work properly with chinese output after the switch, I have to toggle it to english and then back to pinyin and only then it works.

@lilydjwg
Copy link
Contributor

lilydjwg commented Mar 6, 2024

I've resolved most of found issues (https://github.com/lilydjwg/wayfire/commits/im-v1/), except one: moving windows using vswitch. There is no focus change so key events are sent to the window despite vswitch handled it.

@ammen99
Copy link
Member Author

ammen99 commented Mar 6, 2024

@lilydjwg I am looking at your commits but IM keys should never go to plugins so I probably misunderstand a few things you changed .. Also force-release keys should not be done. When keyboard leaves a surface, the application is responsible for releasing all of its keys. This is also how it is done for normal keyboard switches (i.e if you have KEY_A pressed in app X, then switch to Y, Wayfire shouldn't send any release events)

@lilydjwg
Copy link
Contributor

lilydjwg commented Mar 6, 2024

but IM keys should never go to plugins

but then plugins won't receive keys as it's swallowed by this im-v1 plugin.

Also force-release keys should not be done. When keyboard leaves a surface, the application is responsible for releasing all of its keys.

I'm trying to fix unreleased keys issue when im connects / disconnects (keyboard doesn't leave). It doesn't work anyway. Run fcitx5 -r in alacritty and Ctrl-C, you'll get repeating c.

@ammen99
Copy link
Member Author

ammen99 commented Mar 6, 2024

I'm trying to fix unreleased keys issue when im connects / disconnects (keyboard doesn't leave). It doesn't work anyway. Run fcitx5 -r in alacritty and Ctrl-C, you'll get repeating c.

Oh for this we'll have to track pressed keys by the IM, yes. We need a set of pressed keys in the im-v1 plugin, just like in core.

@ammen99
Copy link
Member Author

ammen99 commented Mar 6, 2024

but then plugins won't receive keys as it's swallowed by this im-v1 plugin.

Yeees but also no. The way it works is that the IM sets the processing mode to NO_CLIENT (for the physical keyboard event): which means that grabs and regular clients will not receive the key. But the event is not set to IGNORE, so core still processes it for keybindings.

Also with the NO_CLIENT mode the plugin(s) will not receive the key press on its grab, but I changed switcher/fast-switcher to use the RAW_INPUT flag, which means they are also receiving release events even if they didn't get a press event (like is the case with fcitx5). Note that as soon as we grab the input, the input focus changes so the release event is not grabbed by fcitx5 anymore so those plugins can receive the release event properly.

@lilydjwg
Copy link
Contributor

lilydjwg commented Mar 6, 2024

I see and have updated the commits.

I don't know why but several issues have disappeared for me. Current issues:

  1. Ctrl-C fcitx5 repeats c
  2. Super-Enter to spawn a terminal via the command plugin causes an Enter key to previous terminal window
  3. Using vswitch to move window to another workspace will leave a key to the client, e.g. Super-Shift-J to move down, and then the moved terminal receives a J.

@ammen99
Copy link
Member Author

ammen99 commented Mar 6, 2024

Ok, thanks for testing. I can see what causees these issues and I'll try to figure out how to fix it in the best way. Maybe we'll need a new signal for IMs ...

@ammen99
Copy link
Member Author

ammen99 commented Mar 6, 2024

@lilydjwg I believe that I fixed the issues you mentioned. There are also automatic tests for all of them, the old -v2 implementation is not passing them, the new one does ;)

https://github.com/ammen99/wayfire-tests/tree/master/tests/im

Let me know whether you can find any other bugs. If not, we can merge this PR.

@ssfdust
Copy link
Contributor

ssfdust commented Mar 7, 2024

@ammen99 You are sooooo diligent!

@lilydjwg
Copy link
Contributor

lilydjwg commented Mar 7, 2024

All previous issues are gone, but I find a new one:

  • with fcitx5 activated (e.g. inputting Chinese), pres the , or . key. A fullwidth character should be committed but not until next preedit update.

@ammen99
Copy link
Member Author

ammen99 commented Mar 7, 2024

@lilydjwg That sounds like an fcitx5 bug to me, I don't see what Wayfire can be doing wrong to cause this. Have you checked the messages in the wayland protocol between wayfire and fcitx5 or between wayfire and whatever terminal/editor you're using?

@lilydjwg
Copy link
Contributor

lilydjwg commented Mar 7, 2024

There is a missing done event in that case: lilydjwg@bfd5014

@ammen99
Copy link
Member Author

ammen99 commented Mar 7, 2024

There is a missing done event in that case: lilydjwg@bfd5014

That's a good point thanks :)

Feel free to send a PR to this branch.

@ammen99
Copy link
Member Author

ammen99 commented Mar 9, 2024

I'll merge this PR when CI finishes, if you find any more bugs, feel free to send PRs of course ;)

This prints in detail how we process keyboard key events.
Only xdg-shell popups need the window geometry of their parent added to
their position, im-popups are defined as surface-local coords.
It is still available if someone needs it but it is a gross hack
currently, prefer the upcoming input-method-v1 instead.
This is needed for keyboard events, where the wlr event does not contain
the information required.
@ammen99 ammen99 merged commit efce365 into master Mar 9, 2024
8 checks passed
@ammen99 ammen99 deleted the im-v1 branch March 9, 2024 13:59
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.

3 participants