fix(linux): Fix reordering of output #7079
Merged
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
keymanapp-test-bot
bot
added
the
user-test-missing
User Test ResultsTest specification and instructions
✅ SUITE_WRITER: LibreOffice Writer20 tests in 4 groups PASSED
✅ SUITE_GEDIT: gedit20 tests in 4 groups PASSED
✅ SUITE_FIREFOX: Firefox20 tests in 4 groups PASSED
✅ SUITE_CHROMIUM: Chromium20 tests in 4 groups PASSED
🟩 SUITE_TERMINAL: gnome-terminal
✅ SUITE_ANKI: Anki20 tests in 4 groups PASSED
🟩 SUITE_SEARCHBAR: Searchbar in gnome-shell
Test Artifacts
|
ermshiperete
force-pushed
the
feat/linux/outputOrdering
branch
from
a5e5382 to
70ea643
Compare
|
Rough diagram of IPC sequenceDiagram
Note right of Client: _key_snooper_cb receives new key event
Client-)IBusDaemon: ProcessKeyEvent
IBusDaemon-)EngineDBus: ProcessKeyEvent
EngineDBus-)EngineProcessor: process-key-event
activate EngineProcessor
EngineProcessor-)IBusDaemon: ForwardKeyEvent +PREFILTER
deactivate EngineProcessor
IBusDaemon-)Client: ForwardKeyEvent +PREFILTER
Note right of Client: _key_snooper_cb receives forwarded key event.
Note right of Client: +PREFILTER means we reprocess the key event
Client-)IBusDaemon: ProcessKeyEvent +PREFILTER
IBusDaemon-)EngineDBus: ProcessKeyEvent +PREFILTER
EngineDBus-)EngineProcessor: process-key-event +PREFILTER
activate EngineProcessor
EngineProcessor-)IBusDaemon: bksp + text emitted
deactivate EngineProcessor
IBusDaemon-)Client: bksp + text emitted
|
ermshiperete
force-pushed
the
feat/linux/outputOrdering
branch
from
dedb999 to
95c9c04
Compare
ermshiperete
commented
Aug 18, 2022
linux/ibus-keyman/src/engine.c
Outdated
| // generated will be processed before the character we're adding. We need to send a | ||
| // valid keyval/keycode combination so that it doesn't get swallowed by GTK but which | ||
| // isn't very likely used in real keyboards. F24 seems to work for that. | ||
| ibus_engine_forward_key_event((IBusEngine*)keyman, KEYMAN_NOCHAR_KEYSYM, KEYMAN_F24_KEYCODE_OUTPUT_SENTINEL, IBUS_PREFILTER_MASK); |
There was a problem hiding this comment.
@mcdurdin Do we need to forward the fake key event if the app supports surrounding text?
There was a problem hiding this comment.
No, I don't think so. Order of operations for apps that support surrounding text is:
- Delete and commit text via surrounding-text API.
- If requested by the keyboard, emit original keystroke.
You are right, and this would be a good optimisation because it removes some of the more 'hacky' compat stuff we'd otherwise be doing here.
There was a problem hiding this comment.
With the prefilter version of ibus we get warnings with gedit:
(gedit:2437): Gdk-WARNING **: 17:52:19.388: Event with type 8 not holding a GdkDevice. It is most likely synthesized outside Gdk/GTK+
(gedit:2437): Gdk-WARNING **: 17:52:19.388: Event with type 9 not holding a GdkDevice. It is most likely synthesized outside Gdk/GTK+
Strangely enough we don't get these warnings with Chromium.
There was a problem hiding this comment.
Can we extend the comment above to reference the issue with Chromium doing its 'rate limiting' of hotkey-type keystrokes?
ermshiperete
force-pushed
the
feat/linux/outputOrdering
branch
from
95c9c04 to
134188d
Compare
ermshiperete
commented
Aug 23, 2022
linux/ibus-keyman/src/engine.c
Outdated
|
|
||
| // This keycode is a fake keycode that we send when it's time to commit the text, ensuring the | ||
| // correct output order of backspace and text. | ||
| if ((engine->client_capabilities & IBUS_CAP_PREFILTER) && keycode == KEYMAN_F24_KEYCODE_OUTPUT_SENTINEL && |
There was a problem hiding this comment.
Good thing we check keycode and not keyval since Chromium sends us a different value than what we used in the forward key event (from Chromium: 0x1008ff43, we sent: 0x100fdd0). Other apps send us the same value back.
keymanapp-test-bot
bot
added
has-user-test
user-test-required
ermshiperete
force-pushed
the
feat/linux/outputOrdering
branch
3 times, most recently
from
085be9a to
85e3c10
Compare
mcdurdin
requested changes
Sep 8, 2022
|
@bharanidharanj Please retest TEST_TERMINAL_IPA in Jammy with a slight variation: after you opened gnome-terminal, press the Enter key first, then start with the test (cf #7428). @keymanapp-test-bot retest SUITE_TERMINAL GROUP_JAMMY_X11 TEST_TERMINAL_IPA |
|
The following failing tests in Jammy are caused by gnome-terminal not supporting surrounding text and deleting clusters instead of single code points (see #7130), so we skip these tests. SUITE_TERMINAL
|
SUITE_TERMINAL: gnome-terminalGROUP_JAMMY_X11: Ubuntu 22.04 Jammy with Gnome Shell and X11
|
SUITE_FIREFOX: FirefoxGROUP_JAMMY_X11: Ubuntu 22.04 Jammy with Gnome Shell and X11
|
SUITE_CHROMIUM: ChromiumGROUP_JAMMY_X11: Ubuntu 22.04 Jammy with Gnome Shell and X11
|
SUITE_CHROMIUM: ChromiumGROUP_WASTA: Wasta 20.04 with Cinnamon
|
SUITE_ANKI: AnkiGROUP_WASTA: Wasta 20.04 with Cinnamon
|
SUITE_CHROMIUM: ChromiumGROUP_FOCAL: Ubuntu 20.04 Focal with Gnome Shell and X11
|
keymanapp-test-bot
bot
removed
the
user-test-required
This change implements a commit queue which allows to control the order of the output. This ensures that any backspace we generate will be processed before the character we're adding. Requires changes in ibus (surrounding text fix (see #7072) and prefilter change) Fixes #1489, #4028, #4029, #4030, #4505, #5510, #6639
If we're dealing with an older ibus version that doesn't have the necessary patches we fall back to the old behaviour. Otherwise we use the new output reordering.
We forward a fake event for both keydown and keyup. This change will set the flag on the fake event so that we get keydown and keyup. This is probably not technically necessary, but it helps in debugging because you can then see that both fake events still belong to the same keypress.
Only send fake event if client does not support surrounding text.
Co-authored-by: Marc Durdin <[email protected]>
ermshiperete
force-pushed
the
feat/linux/outputOrdering
branch
from
a0cf9c6 to
bcd2963
Compare
|
Changes in this pull request will be available for download in Keyman version 16.0.78-alpha |
ermshiperete
added a commit
that referenced
this pull request
May 24, 2024
…ervice This change allows to press F24 as ordered output sentinel from keyman-system-service. This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus. The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content. The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079). The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays the same: when we receive the F24 key we commit the characters previously queued. Part of #10799.
ermshiperete
added a commit
that referenced
this pull request
May 24, 2024
…ervice This change allows to press F24 as ordered output sentinel from keyman-system-service. This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus. The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content. The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079). The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays the same: when we receive the F24 key we commit the characters previously queued. Part of #10799.
ermshiperete
added a commit
that referenced
this pull request
May 28, 2024
…vice This change allows to press F24 as ordered output sentinel from keyman-system-service. This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus. The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content. The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079). The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays the same: when we receive the F24 key we commit the characters previously queued. Fixes: #10799
ermshiperete
added a commit
that referenced
this pull request
Jun 13, 2024
…ervice This change allows to press F24 as ordered output sentinel from keyman-system-service. This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus. The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content. The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079). The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays the same: when we receive the F24 key we commit the characters previously queued. Part of #10799.
ermshiperete
added a commit
that referenced
this pull request
Aug 21, 2024
…ervice This change allows to press F24 as ordered output sentinel from keyman-system-service. This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus. The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content. The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079). The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays mostly the same: when we receive the F24 key we commit the characters previously queued. The difference to the previous implementation is that we now also queue the backspace keys that we generate. Part-of: #10799
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.
Quick link to the test results
This change implements a commit queue which allows to control the order of the output. This ensures that any backspace we generate will be processed before the character we're adding.
Requires changes in ibus (surrounding text fix (see #7072) and prefilter change).
User Testing
Preparations
The tests should be run on these Linux platforms:
Add the Keyman Test ppa:
Install all updates
Verify that you have the updated ibus version required to test this PR
there, otherwise you don't have updated ibus version.
On Ubuntu 22.04 Jammy, install the Firefox .deb package (not snap) - this is not needed for older distributions
On Ubuntu 22.04 Jammy, Ubuntu 20.04 Focal and Wasta install the Chromium .deb package (not snap) - this is not needed for Bionic
Install Anki
tar xfx anki-2.*-linux.tar.bz2)Install build artifacts of this PR
Reboot
Install the following keyboards in Keyman:
SUITE_WRITER: LibreOffice Writer
Open LibreOffice Writer.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री". (If the result looks wrong, select all text and change the font to "Siddhanta")xEjmr. Verify that the output is "ខ្មែរ".SUITE_GEDIT: gedit
Open gedit.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री".xEjmr. Verify that the output is "ខ្មែរ".SUITE_FIREFOX: Firefox
Open https://keyman.com/keyboards in Firefox.
NOTE: If the output looks wrong, copy the text from the browser and paste it into gedit and verify that it there.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री".xEjmr. Verify that the output is "ខ្មែរ".SUITE_CHROMIUM: Chromium
Open Chromium browser.
NOTE: If the output looks wrong, copy the text from the browser and paste it into gedit and verify that it there.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री".xEjmr. Verify that the output is "ខ្មែរ".SUITE_TERMINAL: gnome-terminal
Open Terminal.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री".xjmEr(Note the different character order!). Copy the output characters from terminal and paste them in gedit. Verify that the pasted text is "ខ្មែរ". (Terminal has a rendering problem with some Khmer characters, so the output in terminal looks wrong, even though Keyman added the correct characters.)SUITE_ANKI: Anki
Open Anki.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री" (If the result looks wrong, copy/paste it in text editor and verify there).xEjmr. Verify that the output is "ខ្មែរ".SUITE_SEARCHBAR: Searchbar in gnome-shell
Press Windows key to open searchbar.
Tests
n>. Verify that the result is "ŋ".han<space>geul<space>. Verify that the result is "한글".shrI. Verify that the result is "श्री".xEjmr. Verify that the output is "ខ្មែរ".