input: Handling multiple surfaces for the text-input-v1 protocol implementation and imporve InputMethodRelay logic #5147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vaxerski
reviewed
Mar 17, 2024
sujoshua
force-pushed
the
fix_text_input
branch
from
8c194a3
to
207abc0
Compare
vaxerski
reviewed
Mar 18, 2024
sujoshua
force-pushed
the
fix_text_input
branch
from
207abc0
to
516061f
Compare
|
Sorry, It seems that I've picked up some bad habits from writing too much Go. |
|
generally lgtm, but I'll clang-format it for you cuz it still isn't, lol |
…ementation and imporve InputMethodRelay logic fixes hyprwm#2708
vaxerski
force-pushed
the
fix_text_input
branch
from
01f26f6
to
0772eb0
Compare
vaxerski
approved these changes
Mar 19, 2024
|
good job👍🏻 |
lisuke
pushed a commit
to lisuke/Hyprland
that referenced
this pull request
Apr 15, 2024
* input: Handling multiple surfaces for the text-input-v1 protocol implementation and imporve InputMethodRelay logic fixes hyprwm#2708 * clang-format * minor style nits --------- Co-authored-by: Vaxry <vaxry@vaxry.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your PR, what does it fix/add?
This PR addresses the issue where the text-input-v1 protocol could not handle cases where a client creates multiple
TextInput.The case one client creates multiple
TextInputis common. e.g. chromium-based client applications create aTextInputfor each separate surface. They then sendactivaterequest withsurfaceparam which indirectly specifies theTextInput. Because everyTextInputobject is bind to specificsurface.Previously, the code iterated through a linked list and returned upon finding a match by comparing the current focus surface's client with
TextInput's client. This approach always returned the first matchedTextInput, whose internal surface pointed to a different surface rather than the current focus surface. This was the root cause of issue #2708.Moreover, the original code heavily relied on iterating through linked list is not so efficient comparing to a hash lookup way. Therefore, in the
InputMethodRelay, a map fromsurface -> TextInputhas been established, centered around thesurface.TheInputMethodRelaynow records the current focussurface, enabling the retrieval ofTextInputbased on the surface. Consequently, the logic forpendingSurfacein the original code has been removed, as theTextInputcan now be obtained at any time through the focused surface.About issue #2708 mentioned
yeah it's the same problem. It seems that electron below electron 21 will create a transparent surface. So in former implementation, we always caught that transparent surface's
TextInput.Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)
Firstly, I rarely write C++, so there might be some naive mistakes. Please forgive me ~. Though this patch has been running on my main computer for two days without any issues.
Secondly, there's a comment in the
InputMethodRelay.cppcode marked "BUG POSSIBLE". This is because the logic for handling surfaces intext-input-v3is quite different fromv1. It requires the server to proactively inform the client. Apart from using the client for matching, I can't think of a better method. However, using the client for matching is the original code's method, so the issue mentioned for text-input-v3 still persists. But as far as the user experience with Kitty is concerned, there doesn't seem to be any major problem.Is it ready for merging, or does it need work?
yeah
test_text-input-v1.mp4