fix(core): implement ldml_processor::get_key_list() 🙀 #12644

srl295 · 2024-11-07T15:36:13Z

ldml::vkeys class updated to keep a set<> of keys for the get_key_list() implementation
note that ALL keys are included, not just assigned keys.
all EmbeddedLdmLTestSource keyboards get the get_key_list() compared to the key2.kmap table.
fix a test case to not leak!

also

move some test utils to statics
print warning on unhanded @-commands
move kmxplus loading up into the LdmlTestSource parent class, so that all ldml test sources can access the KMX+ binary format for inspection and verification.
update some test utility functions - dump out a key_event as a string, etc.

(tests from #12281 )

User Testing

Please paste in the following LDML keyboard source for testing:

<?xml version="1.0"?>
<keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="arc" conformsTo="45">
  <info author="Marc Durdin" name="Test 'other' modifier"/>
  <version number="1.0.0"/>
  <keys>
    <import base="cldr" path="45/keys-Zyyy-punctuation.xml"/>
    <import base="cldr" path="45/keys-Zyyy-currency.xml"/>
  	<!-- switch keys -->

    <key id="a" output="🅐" />
    <key id="b" output="🅑" />
    <key id="c" output="🅒" />
    <key id="d" output="🅓" />
    <key id="e" output="🅔" />
  </keys>
  <layers formId="us">
    <layer modifiers="none">
      <row keys="grave 1 2 3 4 5 6 7 8 9 0 hyphen equal"/>
      <row keys="a"/>
      <row keys="gap"/>
      <row keys="gap"/>
      <row keys="space"/>
    </layer>
    <layer modifiers="shift">
      <row keys="tilde bang at hash dollar percent caret amp asterisk open-paren close-paren underscore plus"/>
      <row keys="b"/>
      <row keys="gap"/>
      <row keys="gap"/>
      <row keys="space"/>
    </layer>
    <layer modifiers="ctrl">
      <row keys="grave 1 2 3 4 5 6 7 8 9 0 hyphen equal"/>
      <row keys="c"/>
      <row keys="gap"/>
      <row keys="gap"/>
      <row keys="space"/>
    </layer>
    <layer modifiers="other">
      <row keys="grave 1 2 3 4 5 6 7 8 9 0 hyphen equal"/>
      <row keys="d e"/>
      <row keys="gap"/>
      <row keys="gap"/>
      <row keys="space"/>
    </layer>
  </layers>
</keyboard3>

Create a basic LDML keyboard, and paste in the source above. Compile the keyboard and its package. Use this keyboard in all the subsequent tests.

For all tests, pass the test if:

q emits 🅐
shift+q emits 🅑
ctrl+q emits 🅒
alt+q emits 🅓
alt+w emits 🅔
the following key combinations have no output:
- alt+e
- ctrl+e
- shift+e
- ctrl+shift+e
- ctrl+alt+e
- e
TEST_DEBUGGER: Test the keyboard in the Keyman Developer Debugger
TEST_WINDOWS: Install the keyboard in Keyman for Windows, and test the keyboard in Notepad
TEST_MAC: Install the keyboard in Keyman for macOS, and test the keyboard in TextEdit
TEST_LINUX: Install the keyboard in Keyman for Linux, and test the keyboard in a text editor

- move some test utils to statics - add a new test action type and `@@key-list` keyword - print warning on unhanded @-commands Fixes: #12298

- ldml::vkeys class updated to keep a set<> of keys - fix the test case to not leak! Fixes: #12298

keymanapp-test-bot · 2024-11-07T15:36:44Z

User Test Results

Test specification and instructions

✅ TEST_DEBUGGER (PASSED) (notes)
✅ TEST_WINDOWS (PASSED) (notes)
✅ TEST_MAC (PASSED) (notes)
✅ TEST_LINUX (PASSED) (notes)

Test Artifacts

core/tests/unit/ldml/keyboards/k_004_tinyshift.xml

srl295 · 2024-11-07T19:28:06Z

@rc-swag @mcdurdin would this need user testing?

also note that gap keys / unassigned keys are NOT included in the list.

mcdurdin · 2024-11-07T23:08:51Z

@rc-swag @mcdurdin would this need user testing?

Yes, I think it should have testing (all desktop platforms)

also note that gap keys / unassigned keys are NOT included in the list.

They probably need to be, otherwise they will fall back to default output (think Shift modifier for example). The rule we decided on for LDML keyboards was that if a layer is defined at all for a given modifier set, it is defined for all keys on that modifier set (even if it is no-op).

srl295 · 2024-11-07T23:44:10Z

@rc-swag @mcdurdin would this need user testing?

Yes, I think it should have testing (all desktop platforms)

OK. Will ponder the plan.

also note that gap keys / unassigned keys are NOT included in the list.

They probably need to be, otherwise they will fall back to default output (think Shift modifier for example). The rule we decided on for LDML keyboards was that if a layer is defined at all for a given modifier set, it is defined for all keys on that modifier set (even if it is no-op).

OK. So the set will be more fixed, more based on the modifier set than the exact key layout. I can probably just generate the list then.

rc-swag · 2024-11-08T00:32:11Z

@rc-swag @mcdurdin would this need user testing?

Yes, I think it should have testing (all desktop platforms)

OK. Will ponder the plan.

Following the test the found the issue in the first place will exercise the code. Not sure for the other platforms if there is a ready made user test.

srl295 · 2024-11-08T04:59:14Z

great, yes bringing in the test case from #12298 (comment) makes sense.

i think i could do this with a set of the modifier combinations, and then just expand on the 'vkey map' X modifier combinations.

Fixes: #12298

@keyList

- move kmxplus processing into the base LdmlTestSource class - add a function to traverse the layer list looking for keys to add - The @@keyList keyword only has one example from each modifier set Fixes: #12298

- just compare the key list to the key2.kmap table - check the keylist for all LdmlTestSource instances - no syntax needed Fixes: #12298

srl295 · 2024-11-26T16:14:59Z

@mcdurdin @rc-swag I think this is ready for some code review to start. I'm working on the user testing story now.

rc-swag · 2024-11-29T03:12:25Z

But as I noted it uses the modifier OTHER and does not expand it to the actual modifiers.

I thought you may have already done the expansion before the add keys part. In the issue for this PR I commented on this I stated that. #12298 (comment)
Implementation detail:
Of interest with this example also is that the ALT key is in a keyboard rule via the the LDML rule other for modifiers. The get_key_list() will obviously have to do the logical expansion of the rule.

The windows engine to my knowledge has no concept of OTHER.

mcdurdin · 2024-11-29T03:26:23Z

But as I noted it uses the modifier OTHER and does not expand it to the actual modifiers.

The OTHER modifier is an internal detail of the LDML implementation and must not leak out into Core.

…key-list

srl295 · 2024-11-29T21:54:52Z

@rc-swag you did state it in the issue. Will fix it.

Of interest with this example also is that the ALT key is in a keyboard rule via the the LDML rule other for modifiers. The get_key_list() will obviously have to do the logical expansion of the rule.

I will note though, that even if I explicitly set the layer to be 'alt' modifier (and not other) I don't think I'm seeing the correct behavior (i.e. alt-q, alt-w) locally.

I will work on the expansion, but as noted let's try to debug next wk

- expand OTHER and ALT / CTRL appropriately - add KM_CORE_MODIFIER_NONE=0 - disable test of get_key_list() for now Fixes: #12298

- reintroduce example keycaps Fixes: #12298

…key-list

rc-swag · 2024-12-05T05:57:09Z

@srl295 Ok I built the latest commit of this and tested a build of the keyboard in the user testing section. The alt (including RALT) key behaves correctly and ouputs a 🅓. As seperate keyboard test I also replaced other with alt and that also worked. To see have preserved keys printed into the log I commented #define _DEBUG_LOGKEY in ln.38 of keys.cpp. That allowed me to see that it appeared to be working as expected.

CKMTipTextService::_PreserveAltKeys: Preserved key 162: K_Q 0, 0
CKMTipTextService::_PreserveAltKeys: Preserved key 163: K_Q 4, 0
CKMTipTextService::_PreserveAltKeys: Preserved key 164: K_Q 2, 0
CKMTipTextService::_PreserveAltKeys: Preserved key 165: K_Q 10, 
CKMTipTextService::_PreserveAltKeys: Preserved key 166: K_Q 40, 
CKMTipTextService::_PreserveAltKeys: Preserved key 167: K_Q 8, 0

- yes, expand 'other' to all possible combinations - use ALT and CTRL instead of RALT,LALT and RCTRL,LCTRL in the key list (reduce expansions up to 4x) Fixes: #12298

srl295 · 2024-12-06T18:26:38Z

@srl295 Ok I built the latest commit of this and tested a build of the keyboard in the user testing section. The alt (including RALT) key behaves correctly and ouputs a 🅓. As seperate keyboard test I also replaced other with alt and that also worked. To see have preserved keys printed into the log I commented #define _DEBUG_LOGKEY in ln.38 of keys.cpp. That allowed me to see that it appeared to be working as expected.
CKMTipTextService::_PreserveAltKeys: Preserved key 162: K_Q 0, 0
CKMTipTextService::_PreserveAltKeys: Preserved key 163: K_Q 4, 0
CKMTipTextService::_PreserveAltKeys: Preserved key 164: K_Q 2, 0
CKMTipTextService::_PreserveAltKeys: Preserved key 165: K_Q 10, 
CKMTipTextService::_PreserveAltKeys: Preserved key 166: K_Q 40, 
CKMTipTextService::_PreserveAltKeys: Preserved key 167: K_Q 8, 0

I missed this but this is great news.

I've updated the code to do the combinatoric explosion as discussed. Passes tests (which do a spot check of the expansions.)

Think this may be ready to go.

srl295 · 2024-12-06T18:28:08Z

Thanks @dinakaranr - actually

(Windows 10)
alt+q emits 🅓 no results
alt+w emits 🅔 no results
The below key combination does not give any outputs: It correct
alt+e, ctrl+e, shift+e, ctrl+shift+e, ctrl+alt+e, e
This behavior already has an issue in Github and the PR number is #12298 hence, I passed this test case.

Actually, you are right that it is already an issue, however, this PR is intended to fix the issue - therefore, please fail the test case.

@rc-swag should this test case now pass? ☝️

Lorcon1 · 2024-12-07T00:00:34Z

rc-swag

lgtm

rc-swag · 2024-12-09T06:04:44Z

@rc-swag should this test case now pass? ☝️

It should.
@keymanapp-test-bot retest TEST_WINDOWS

dinakaranr · 2024-12-09T08:55:43Z

Test Results

TEST_WINDOWS (Passed):
Window 10 OS:

I installed the Keyman 18.0.156-alpha-test-12644 version and then started the configuration.
On Keyman developer: Navigate to the "Packaging" tab and then click the *.kmx link button to install the keyboard in Windows 10.
Select the LDML keyboard that was installed through "Keyman Developer"
Open the notepad. Press the keys below and observe the options below.
q emits "A" and appears. It's correct.
shift+q emits "B" appeared. It's correct
ctrl+q emits "C" appeared. It's correct
alt+q emits "D"appeared. It's correct.
alt+w emits "E" appeared. It's correct.
The below key combination does not give any outputs: It's correct

alt+e, ctrl+e, shift+e, ctrl+shift+e, ctrl+alt+e, e
The D and E keys appeared on the notepad. It works great for me. Thank you.
Please take a look at the screenshot

keyman-server · 2024-12-09T18:01:13Z

Changes in this pull request will be available for download in Keyman version 18.0.156-alpha

mcdurdin · 2024-12-10T00:48:39Z

Can we get a cherry-pick of this to 17.0 please? (A squashed pick might be good here, given 18 commits in this PR...)

- Cherry pick of #12644 - Cherry pick of de3397b Fixes: #12298

srl295 added 2 commits November 6, 2024 09:30

feat(core): ldml: scaffolding for testing get_key_list()

dd11a25

- move some test utils to statics - add a new test action type and `@@key-list` keyword - print warning on unhanded @-commands Fixes: #12298

feat(core): ldml: implementation for testing get_key_list()

382b511

- ldml::vkeys class updated to keep a set<> of keys - fix the test case to not leak! Fixes: #12298

srl295 self-assigned this Nov 7, 2024

github-actions bot added core/ Keyman Core fix labels Nov 7, 2024

keymanapp-test-bot bot added this to the A18S14 milestone Nov 7, 2024

Base automatically changed from fix/developer/12467-invalid-marker-names to master November 7, 2024 19:26

Merge branch 'master' into fix/core/12298-get-key-list

cfda52b

srl295 marked this pull request as ready for review November 7, 2024 19:26

srl295 requested review from mcdurdin and rc-swag as code owners November 7, 2024 19:26

srl295 commented Nov 7, 2024

View reviewed changes

core/tests/unit/ldml/keyboards/k_004_tinyshift.xml Outdated Show resolved Hide resolved

srl295 marked this pull request as draft November 7, 2024 23:42

Merge branch 'master' into fix/core/12298-get-key-list

741d305

darcywong00 modified the milestones: A18S14, A18S15 Nov 9, 2024

feat(core): get_key_list needs to include all keys

2e5b601

Fixes: #12298

darcywong00 modified the milestones: A18S15, A18S16 Nov 24, 2024

srl295 added 3 commits November 25, 2024 12:54

feat(core): test improvements for get_key_list()

6d1035a

- move kmxplus processing into the base LdmlTestSource class - add a function to traverse the layer list looking for keys to add - The @@keyList keyword only has one example from each modifier set Fixes: #12298

feat(core): test improvements for get_key_list()

f9862c3

- just compare the key list to the key2.kmap table - check the keylist for all LdmlTestSource instances - no syntax needed Fixes: #12298

Merge branch 'master' into fix/core/12298-get-key-list

3904d78

keymanapp-test-bot bot added the has-user-test label Nov 26, 2024

srl295 changed the title ~~fix(core): implement ldml_processor::get_key_list()~~ fix(core): implement ldml_processor::get_key_list() 🙀 Nov 29, 2024

srl295 added 3 commits November 29, 2024 15:33

Merge remote-tracking branch 'origin/master' into fix/core/12298-get-…

c964364

…key-list

chore(core): update to ldmL_test_source.cpp

29db34f

chore(core): update to ldmL_test_source.cpp

d92dc17

srl295 mentioned this pull request Nov 29, 2024

feat(core,linux,developer,windows): implement loading KMX from blob 🍒 #12721

Merged

srl295 and others added 4 commits November 29, 2024 17:25

feat(core): improvements for get_key_list()

7f3bd30

- expand OTHER and ALT / CTRL appropriately - add KM_CORE_MODIFIER_NONE=0 - disable test of get_key_list() for now Fixes: #12298

feat(core): improvements for get_key_list()

9e2b8d0

- reintroduce example keycaps Fixes: #12298

Merge remote-tracking branch 'origin/master' into fix/core/12298-get-…

da7c864

…key-list

Merge branch 'master' into fix/core/12298-get-key-list

6afa88b

fix(core): update get_key_list to account for other combinatorics

feba979

- yes, expand 'other' to all possible combinations - use ALT and CTRL instead of RALT,LALT and RCTRL,LCTRL in the key list (reduce expansions up to 4x) Fixes: #12298

darcywong00 modified the milestones: A18S16, A18S17 Dec 7, 2024

rc-swag approved these changes Dec 9, 2024

View reviewed changes

keymanapp-test-bot bot added the user-test-required User tests have not been completed label Dec 9, 2024

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Dec 9, 2024

srl295 merged commit de3397b into master Dec 9, 2024
20 checks passed

srl295 deleted the fix/core/12298-get-key-list branch December 9, 2024 14:14

srl295 added a commit that referenced this pull request Dec 10, 2024

fix(core): implement ldml_processor::get_key_list() 🍒 🙀

a3986e7

- Cherry pick of #12644 - Cherry pick of de3397b Fixes: #12298

srl295 mentioned this pull request Dec 10, 2024

fix(core): implement ldml_processor::get_key_list() 🍒 🙀 🏠 #12816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): implement ldml_processor::get_key_list() 🙀 #12644

fix(core): implement ldml_processor::get_key_list() 🙀 #12644

srl295 commented Nov 7, 2024 •

edited

Loading

keymanapp-test-bot bot commented Nov 7, 2024 •

edited

Loading

srl295 commented Nov 7, 2024

mcdurdin commented Nov 7, 2024

srl295 commented Nov 7, 2024 •

edited

Loading

rc-swag commented Nov 8, 2024 •

edited by srl295

Loading

srl295 commented Nov 8, 2024

srl295 commented Nov 26, 2024

rc-swag commented Nov 29, 2024

mcdurdin commented Nov 29, 2024

srl295 commented Nov 29, 2024

rc-swag commented Dec 5, 2024 •

edited

Loading

srl295 commented Dec 6, 2024

srl295 commented Dec 6, 2024

Lorcon1 commented Dec 7, 2024

rc-swag left a comment

rc-swag commented Dec 9, 2024 •

edited

Loading

dinakaranr commented Dec 9, 2024

keyman-server commented Dec 9, 2024

mcdurdin commented Dec 10, 2024 •

edited

Loading

fix(core): implement ldml_processor::get_key_list() 🙀 #12644

fix(core): implement ldml_processor::get_key_list() 🙀 #12644

Conversation

srl295 commented Nov 7, 2024 • edited Loading

User Testing

keymanapp-test-bot bot commented Nov 7, 2024 • edited Loading

User Test Results

Test Artifacts

srl295 commented Nov 7, 2024

mcdurdin commented Nov 7, 2024

srl295 commented Nov 7, 2024 • edited Loading

rc-swag commented Nov 8, 2024 • edited by srl295 Loading

srl295 commented Nov 8, 2024

srl295 commented Nov 26, 2024

rc-swag commented Nov 29, 2024

mcdurdin commented Nov 29, 2024

srl295 commented Nov 29, 2024

rc-swag commented Dec 5, 2024 • edited Loading

srl295 commented Dec 6, 2024

srl295 commented Dec 6, 2024

Lorcon1 commented Dec 7, 2024

rc-swag left a comment

Choose a reason for hiding this comment

rc-swag commented Dec 9, 2024 • edited Loading

dinakaranr commented Dec 9, 2024

Test Results

keyman-server commented Dec 9, 2024

mcdurdin commented Dec 10, 2024 • edited Loading

srl295 commented Nov 7, 2024 •

edited

Loading

keymanapp-test-bot bot commented Nov 7, 2024 •

edited

Loading

srl295 commented Nov 7, 2024 •

edited

Loading

rc-swag commented Nov 8, 2024 •

edited by srl295

Loading

rc-swag commented Dec 5, 2024 •

edited

Loading

rc-swag commented Dec 9, 2024 •

edited

Loading

mcdurdin commented Dec 10, 2024 •

edited

Loading