search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

fix(core): reset on frame keys πŸ™€ #11172

Closed srl295 closed 3 weeks ago

srl295 commented 1 month ago

Fixes #10955 Fixes #10227

User Testing

Using Windows 10 or Windows 11. Install the keyman build from this PR.

Use the Text Editor linked in this PR under test artifacts. or if on Windows 10 you can use Notepad.

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

For these tests make sure the application used on the operating system is a non-compliant. A app that is not able to give the context to Keyman.

GROUP_WIN_10: TextEditor

GROUP_WIN_11: TextEditor

GROUP_MAC: Chrome Browser

GROUP_LINUX: Chrome Brower (https://www.editpad.org/)

Install SIL Euro Latin

TEST_CONTROL_1

  1. Press `
  2. press e Expected result: Γ¨

TEST_FRAME_KEY_RESET_MARKERS

This tests the markers are lost

  1. Press `
  2. Press -> right arrow
  3. Press e Expected result: `e

TEST_FRAME_KEY_RESET_NOMARKERS

  1. Press 1 / /
  2. Press -> right arrow
  3. Press 2 Expected result: 1//2

TEST_SCROLLLOCK_KEY_NO_RESET

Do not reset for scroll lock

  1. Press `
  2. Press scroll lock for Windows and Linux. For MacOS Press fn
  3. Press e Expected Result: Γ¨

SUITE_KMX_PROCESSOR_COMPLIANT:

For these tests make sure the application used on the operating system is compliant. That is Keyman can request the context from the application.

GROUP_WIN_10: WordPad

GROUP_WIN_11: Wordpad

GROUP_MAC: TextEdit

GROUP_LINUX: gedit

Install SIL Euro Latin

TEST_CONTROL_1

  1. Press `
  2. press e Expected result: Γ¨

TEST_FRAME_KEY_RESET_MARKERS

This tests the markers are lost

  1. Press `
  2. Press -> right arrow
  3. Press e Expected result: `e

TEST_FRAME_KEY_RESET_NOMARKERS

  1. Press 1 / /
  2. Press -> right arrow
  3. Press 2 Expected result: Β½

TEST_SCROLLLOCK_KEY_NO_RESET

Do not reset for scroll lock

  1. Press `
  2. Press scroll lock for Windows and Linux. For MacOS Press fn
  3. Press e Expected Result: Γ¨

SUITE_LDML_PROCESSOR_NON_COMPLIANT:

LDML keyboard

GROUP_WIN_10: TextEditor

GROUP_WIN_11: TextEditor

GROUP_MAC: Chrome Browser

GROUP_LINUX: Chrome Brower

Install contextreset.zip ldml keyboard

TEST_CONTROL_1

  1. Press ^
  2. Press e Expected result: Γͺ

This was previously failing

TEST_FRAME_KEY_RESET_MARKERS

  1. Press ^
  2. Press -> right arrow
  3. Press e Expected result: e

TEST_FRAME_KEY_RESET_NO_MARKERS

  1. Press a
  2. Press -> right arrow
  3. Press Shift + 8 Expected result: a*

TEST_FRAME_SCROLL_LOCK_NO_RESET

  1. Press ^
  2. Press scroll lock for Windows and Linux. For MacOS Press fn
  3. Press e Expected result: Γͺ

TEST_MODIFIER_TAP_NO_RESET

Do not reset for modifier tap

  1. Press ^
  2. Press and Release ctrl
  3. Press e Expected result: Γͺ

SUITE_LDML_PROCESSOR_COMPLIANT:

LDML keyboard

GROUP_WIN_10: WordPad

GROUP_WIN_11: WordPad

GROUP_MAC: TextEdit

GROUP_LINUX: gedit

Install contextreset.zip ldml keyboard

TEST_CONTROL_1

  1. Press ^
  2. Press e Expected result: Γͺ

This was previously failing

TEST_FRAME_KEY_RESET_MARKERS

  1. Press ^
  2. Press -> right arrow
  3. Press e Expected result: e

TEST_FRAME_KEY_RESET_NO_MARKERS

  1. Press a
  2. Press -> right arrow
  3. Press Shift + 8 Expected result: Star

TEST_FRAME_SCROLL_LOCK_NO_RESET

  1. Press ^
  2. Press scroll lock for Windows and Linux. For MacOS Press fn
  3. Press e Expected result: Γͺ

TEST_MODIFIER_TAP_NO_RESET

Do not reset for modifier tap

  1. Press ^
  2. Press and Release ctrl
  3. Press e Expected result: Γͺ
keymanapp-test-bot[bot] commented 1 month ago

User Test Results

Test specification and instructions

🟩 SUITE_KMX_PROCESSOR_NON_COMPLIANT:

🟩 SUITE_KMX_PROCESSOR_COMPLIANT:

🟩 SUITE_LDML_PROCESSOR_NON_COMPLIANT:

🟩 SUITE_LDML_PROCESSOR_COMPLIANT:

Test Artifacts

mcdurdin commented 1 month ago

For #10955

This should be fixing that issue, right? in which case "fixes ..." rather than "for ..." (and don't forget to link in the Development right hand panel in GH)

keymanapp-test-bot skip

I think we can usefully have user tests for this to verify that outcome is as we expect on each platform. Need to spec with non-compliant apps, a test for pressing specific frame key and another test for non-frame keys, with a keyboard rule that depends on preceding context. With sil_euro_latin, type ' ... frame key | non frame key ... e, and for non-frame key it should combine, with frame key it shouldn't. We could use e.g. Insert as a non-frame key, and e.g. Escape as a frame key, if using an app such as Notepad where those keys won't have any other effect.

In fact may need to have tests with compliant apps as well, to verify markers are dropped when frame keys are pressed. Needs thinking through a bit but I think this is critical given this is fundamental to how Keyman works.

srl295 commented 1 month ago

For #10955

This should be fixing that issue, right? in which case "fixes ..." rather than "for ..." (and don't forget to link in the Development right hand panel in GH)

I usually update that once it actually fixes.

keymanapp-test-bot skip

I think we can usefully have user tests for this to verify that outcome is as we expect on each platform. Need to spec with non-compliant apps, a test for pressing specific frame key and another test for non-frame keys, with a keyboard rule that depends on preceding context. With sil_euro_latin, type ' ... frame key | non frame key ... e, and for non-frame key it should combine, with frame key it shouldn't. We could use e.g. Insert as a non-frame key, and e.g. Escape as a frame key, if using an app such as Notepad where those keys won't have any other effect.

Good point. I have never done a user test so far. So I'm just used to skip.

In fact may need to have tests with compliant apps as well, to verify markers are dropped when frame keys are pressed. Needs thinking through a bit but I think this is critical given this is fundamental to how Keyman works.

Makes sense

srl295 commented 1 month ago

getting closer.. i had to:

In the k_000___null_keyboard.kmn test (I'm getting familiar with kmx and its test code!) there's this case:

store(&NAME) '000 - null keyboard'
c Description: Tests null keyboard
c keys: [K_A][RALT K_B][SHIFT K_C]
c expected: aC
c expected context: C
c context:

store(&version) '6.0'

begin Unicode > use(Main)

group(Main) using keys

kmx does a reset here after the RAlt-b. But vkey 66 (K_B) in the reset table is false.

Should it reset because of the modifiers?

srl295 commented 1 month ago

reset on modifiers doesn't work well either per other tests.

srl295 commented 1 month ago

aha.. it needs to be a key UP event also

srl295 commented 1 month ago

OK, only failing kmx test is the k_020 deadkeys one..

action: emit keystroke

expected        : U+0077 U+0078 U+0061 U+0061 U+0020 U+006F U+006B  [wxaa ok]

text store      : U+0077 U+0078 U+0079 U+0061 U+0061 U+0063 U+0064  [wxyaacd]

expected context: U+0077 U+0078 U+0061 U+0061 U+0020 U+006F U+006B  [wxaa ok]
srl295 commented 1 month ago

should pass - the check for chars_to_delete seems to be counterproductive

srl295 commented 1 month ago

@rc-swag @mcdurdin any comments on the user test required? Assuming the code passes that is going to be the last blocking issue.

The k_102 keytest tests exactly that markers are dropped:

<key id="q" output="q\m{q}"/>
<transform from="q\m{q}a" to="A" />

then typing qEntera yields qa not A because the enter drops markers.

What's the best way to specify that in user tests?

rc-swag commented 1 month ago

What's the best way to specify that in user tests?

I will write some user tests for this today.

rc-swag commented 1 month ago

(moved up into description)

rc-swag commented 1 month ago

Hmm, I think I need to add a test case for the numlock keys *; / ;- . To test it manually I would need to update the test keyboard to have a transform based on these.

srl295 commented 1 month ago

Why does the check status show 'user tests are not required' still?

srl295 commented 1 month ago

@keymanapp-test-bot retest

mcdurdin commented 1 month ago

kmx does a reset here after the RAlt-b. But vkey 66 (K_B) in the reset table is false.

Should it reset because of the modifiers?

Yes, Ctrl or Alt + another (unmatched) key should reset. Ctrl or Alt pressed and released without a chord should not cause a reset.

mcdurdin commented 1 month ago

This is a critical change late in beta to the kmx processor (less worried about ldml processor at this point).

We need to have user tests run on Windows, Linux, and macOS. We need both compliant and non-compliant apps in all cases. For compliant apps, we want to see that markers are reset when frame key is pressed -- and those tests are showing that. For non-compliant apps, we want to verify that all context is reset when frame key is pressed.

rc-swag commented 1 month ago

I started a new ldml keyboad on friday because i want to test some missing scenarios. I can expand the test cases when I do that.

On Sat, 13 Apr 2024, 9:58 am Marc Durdin, @.***> wrote:

This is a critical change late in beta to the kmx processor (less worried about ldml processor at this point).

We need to have user tests run on Windows, Linux, and macOS. We need both compliant and non-compliant apps in all cases. For compliant apps, we want to see that markers are reset when frame key is pressed -- and those tests are showing that. For non-compliant apps, we want to verify that all context is reset when frame key is pressed.

β€” Reply to this email directly, view it on GitHub https://github.com/keymanapp/keyman/pull/11172#issuecomment-2052707316, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN5XSSFO4EWRK4XQINH5IXLY5BYJHAVCNFSM6AAAAABFYCRNWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJSG4YDOMZRGY . You are receiving this because you were mentioned.Message ID: @.***>

srl295 commented 1 month ago

  1. It’s after rule processing.

  2. Yes should check vk <= length of array

rc-swag commented 1 month ago

I don't follow the handling of the numberpad keys. I had previously noted before this change that the numberpad keys with key caps /,*,-,+, Do not get inserted into the the context. I was looking at making a manual keyboard that had rules with these characters to see show the rule could be matched as the pressing these keys no longer results in a context reset. However, I realised that it only works when you press the equivalent of these keys Not using the numberpad. Using the numberpad the characters never make it into the context.

bharanidharanj commented 1 month ago

Test Results

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

GROUP_WIN_11: TextEditor

bharanidharanj commented 1 month ago

SUITE_KMX_PROCESSOR_COMPLIANT:

GROUP_WIN_11: Wordpad

bharanidharanj commented 1 month ago

SUITE_LDML_PROCESSOR_NON_COMPLIANT:

GROUP_WIN_11:

bharanidharanj commented 1 month ago

SUITE_LDML_PROCESSOR_COMPLIANT:

GROUP_WIN_11:

bharanidharanj commented 1 month ago

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

GROUP_WIN_10: TextEditor

bharanidharanj commented 1 month ago

SUITE_KMX_PROCESSOR_COMPLIANT:

GROUP_WIN_10: WordPad

bharanidharanj commented 1 month ago

Test Results

SUITE_LDML_PROCESSOR_NON_COMPLIANT:

GROUP_WIN_10:

SUITE_LDML_PROCESSOR_COMPLIANT:

GROUP_WIN_10:

srl295 commented 1 month ago

@bharanidharanj looking at:

https://github.com/keymanapp/keyman/blob/6a2b9015c53a292ddf4115f8b998aed7303b89fe/mac/Keyman4MacIM/Keyman4MacIM/TextApiCompliance.m#L176

I would use the Mac TextEdit as a compliant app, and it seems there's a list of apps at the link above. do you have any of those installed?

bharanidharanj commented 1 month ago

Test Results

SUITE_KMX_PROCESSOR_COMPLIANT:

GROUP_LINUX:

bharanidharanj commented 1 month ago

Test Results

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

GROUP_LINUX:

bharanidharanj commented 1 month ago

SUITE_LDML_PROCESSOR_COMPLIANT:

GROUP_LINUX:

bharanidharanj commented 1 month ago

SUITE_LDML_PROCESSOR_NON_COMPLIANT:

GROUP_LINUX:

bharanidharanj commented 1 month ago

Test Results

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

GROUP_MAC:

SUITE_KMX_PROCESSOR_COMPLIANT:

GROUP_MAC:

bharanidharanj commented 1 month ago

Test Results

SUITE_LDML_PROCESSOR_COMPLIANT:

GROUP_MAC:

bharanidharanj commented 1 month ago

Test Results

SUITE_LDML_PROCESSOR_NON_COMPLIANT:

GROUP_MAC:

srl295 commented 1 month ago

@bharanidharanj we're going to rework this one a bit. please hold off on testing

srl295 commented 1 month ago

@rc-swag OK now i'm tracking.

 * Refresh app_context to match the cached_context. Does not do normalization,
 * unlike `actions_normalize`. Used in conjunction with keyboard processors that
 * do not support normalization. Resulting app context is same as the cached
 * context, but without markers.
... */
bool km::core::actions_update_app_context_nfu(

but in the kmx case, cached_context is also now empty. however, the above function does NOT set code_points_to_delete. It actually just replaces the app_context with the (now cleared) context.

I guess what I'm wondering is if actions_normalize() should have something such as the following at the top:

diff --git a/core/src/actions_normalize.cpp b/core/src/actions_normalize.cpp
index 3384fda975..a250d6d192 100644
--- a/core/src/actions_normalize.cpp
+++ b/core/src/actions_normalize.cpp
@@ -46,6 +46,11 @@ bool km::core::actions_normalize(
     return false;
   }

+  if (cached_context->empty()) {
+    // cached context was cleared - so don't consider app context.
+    km_core_context_clear(static_cast<km_core_context*>(app_context));
+  }
+
   /*
     The code_points_to_delete value at this point is in NFD. The cached_context
     is in NFD and has already been updated by the keyboard processor to the

that is, if there's nothing in the cached context, don't consider any app_context.

if i'm reading right this would make the ldml behavior match kmx in this situation?

mcdurdin commented 1 month ago

@srl295 That sounds appropriate but I have been out of the loop long enough that I am not 100% confident that I am thinking through all scenarios.

rc-swag commented 1 month ago

that is, if there's nothing in the cached context, don't consider any app_context.

Thank you for the detailed explanation. What you propose would work however, I am thinking that maybe we are handling the result of the invalidated context in two places which may get lost when we look at this code in 1 years time. Do you think we could clear the app_context at the same time we clear the cached context if invalidate_context is true. That way the actions_normalize can stay as it is?

srl295 commented 1 month ago

that is, if there's nothing in the cached context, don't consider any app_context.

Thank you for the detailed explanation. What you propose would work however, I am thinking that maybe we are handling the result of the invalidated context in two places which may get lost when we look at this code in 1 years time. Do you think we could clear the app_context at the same time we clear the cached context if invalidate_context is true. That way the actions_normalize can stay as it is?

that makes even more sense. i will try that, let me see if i can install this locally and test out the cases myself.

srl295 commented 1 month ago

So, I installed the build artifact, and was not able to reproduce the issue so far with the latest commit

Now installing a slightly prior version to make sure i can repro what Ross saw.

srl295 commented 1 month ago

ok SUCCESS i repro'ed the problem in windows downloading an older build of this PR!

srl295 commented 1 month ago
rc-swag commented 1 month ago

@mcdurdin I can investigate but you may know, do the number lock keys not get passed to the keyboard processors?

I was going to write some tests to make sure the number lock keys *;/;+;-; did not reset context however they do. (or they don't get added to the context)

Non-compliant Load euro-latin

  1. Press 1
  2. Using the number pad press / /
  3. Press 2 The result is: 1//2 instead of Β½

Non-compliant context reset

  1. Press a
  2. Using the number pad press * the result is: a* instead of Star
mcdurdin commented 1 month ago

@mcdurdin I can investigate but you may know, do the number lock keys not get passed to the keyboard processors?

I don't have a memory on the answer to this, sorry. I think we supported numlock on + numeric keypad in the past, but it's been a long time since I have played with it.

rc-swag commented 1 month ago

Ok I am creating a separate issue to deal with the numberpad characters. This PR is solving the blocking issue with LDML, it is adding the vkreset array. Anymore is making it too hard to track the changes. https://github.com/keymanapp/keyman/issues/11250

srl295 commented 4 weeks ago

@keymanapp-test-bot retest SUITE_KMX_PROCESSOR_NON_COMPLIANT GROUP_WIN_11 all , SUITE_KMX_PROCESSOR_COMPLIANT GROUP_WIN_11 all, SUITE_LDML_PROCESSOR_NON_COMPLIANT GROUP_WIN_11 all, SUITE_LDML_PROCESSOR_COMPLIANT GROUP_WIN_11 all

rc-swag commented 3 weeks ago

@keymanapp-test-bot retest SUITE_KMX_PROCESSOR_NON_COMPLIANT GROUP_WIN_10 TEST_SCROLLLOCK_KEY_NO_RESET GROUP_MAC TEST_SCROLLLOCK_KEY_NO_RESET GROUP_LINUX TEST_CONTROL_1 TEST_SCROLLLOCK_KEY_NO_RESET SUITE_KMX_PROCESSOR_COMPLIANT GROUP_WIN_10 TEST_SCROLLLOCK_KEY_NO_RESET GROUP_MAC TEST_SCROLLLOCK_KEY_NO_RESET GROUP_LINUX TEST_FRAME_KEY_RESET_MARKERS SUITE_LDML_PROCESSOR_NON_COMPLIANT GROUP_WIN_10 TEST_FRAME_SCROLL_LOCK_NO_RESET GROUP_MAC TEST_FRAME_SCROLL_LOCK_NO_RESET SUITE_LDML_PROCESSOR_COMPLIANT GROUP_WIN_10 TEST_FRAME_KEY_RESET_NO_MARKERS TEST_FRAME_SCROLL_LOCK_NO_RESET GROUP_MAC TEST_FRAME_KEY_RESET_NO_MARKERS TEST_FRAME_SCROLL_LOCK_NO_RESET GROUP_LINUX TEST_FRAME_KEY_RESET_MARKERS

srl295 commented 3 weeks ago

@bharanidharanj the LDML tests now give editors for mac and linux

bharanidharanj commented 3 weeks ago

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

GROUP_WIN_10: TextEditor

bharanidharanj commented 3 weeks ago

Test Results

SUITE_KMX_PROCESSOR_NON_COMPLIANT:

GROUP_WIN_11: TextEditor