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

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Nov 7, 2024

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

also

  • move some test utils to statics
  • add a new test action type and @@key-list keyword
  • print warning on unhanded @-commands

Fixes: #12298

@keymanapp-test-bot skip

- 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
@srl295 srl295 self-assigned this Nov 7, 2024
@github-actions github-actions bot added core/ Keyman Core fix labels Nov 7, 2024
@keymanapp-test-bot 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
@srl295 srl295 marked this pull request as ready for review November 7, 2024 19:26
@@ -4,6 +4,7 @@

@@keys: [SHIFT K_BKQUOTE][K_1][K_BKQUOTE]
@@expected: \u0037\u1790\u17B6\u0127
@@keylist: [SHIFT K_BKQUOTE][SHIFT K_1][K_BKQUOTE][K_1]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just picked this because it's a smaller keyboard.

@srl295
Copy link
Member Author

srl295 commented Nov 7, 2024

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

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

@mcdurdin
Copy link
Member

mcdurdin commented Nov 7, 2024

@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 srl295 marked this pull request as draft November 7, 2024 23:42
@srl295
Copy link
Member Author

srl295 commented Nov 7, 2024

@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
Copy link
Contributor

rc-swag commented Nov 8, 2024

@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
Copy link
Member Author

srl295 commented Nov 8, 2024

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.

@darcywong00 darcywong00 modified the milestones: A18S14, A18S15 Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/ Keyman Core fix
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

bug(windows): LDML keyboards do not appear to work correctly with Alt keys
4 participants