keymanapp / keyman

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

bug(core): ldml key-not-found not quite right 🙀 #9451

Closed srl295 closed 6 months ago

srl295 commented 10 months ago

From review at https://github.com/keymanapp/keyman/pull/9405/files#r1289675373

Some frame keys cause a context invalidation. Frame keys and modifier keys etc then get kicked back to the OS, but any US English character-generating key gets added as an action

ldml processor doesn't do this yet.

Also see https://unicode-org.atlassian.net/browse/CLDR-17262 we may need some spec improvement here.

mcdurdin commented 8 months ago

but any US English character-generating key gets added as an action

Note that we resolved that unassigned keys do nothing on ldml keyboards.

srl295 commented 7 months ago

but any US English character-generating key gets added as an action

Note that we resolved that unassigned keys do nothing on ldml keyboards.

question for @mcdurdin and @rc-swag …

okay, but in reading the kmx code, i'm not sure how I distinguish between an unassigned key and a "frame key and modifier etc".

Do i need to check for KM_CORE_VKEY_ENTER || KM_CORE_VKEY_SHIFT || … || KM_CORE_VKEY_PRTSCN etc?

I could check for every base vkey that could occur on the scancodes lists… Then I would know which vkeys were expected to be mappable by ldml. So K_A if unmapped would do nothing but K_ENTER would be passed through to the OS?

srl295 commented 7 months ago

Comments here:

srl295 commented 7 months ago
- key action: ['K_BKQUOTE' 0xc0]/modifier Unmodified 0x0
Execute debugger commands using "-exec <command>", for example "-exec info registers" will list registers in use (when GDB is the debugger)
Loaded '/usr/lib/swift/libswiftCore.dylib'. Symbols loaded.
context   : U+0061  [a]
testcontext U+61 
context   : U+0061  [a]
- key action: ['K_1' 0x31]/modifier Unmodified 0x0
context   : U+0061  [a]
testcontext U+61 
context   : U+0061  [a]
- key action: ['K_ENTER' 0xd]/modifier Unmodified 0x0
action: context invalidated (markers cleared)
action: emit keystroke
context   : U+0061  [a]
testcontext U+61 
context   : U+0061  [a]
- check expected

I have this working locally (PR in progress) where it distinguishes between a gap key (K_1 above - no output, no action happens) and an 'unmapped' key (K_ENTER above where it shows context invalidated and emit keystroke). i'm not sure how to test for this though. Maybe something with showing markers were cleared, that seems indirect.

srl295 commented 7 months ago

^ PTAL https://github.com/keymanapp/keyman/pull/10090

mcdurdin commented 7 months ago

I have this working locally (PR in progress) where it distinguishes between a gap key (K_1 above - no output, no action happens) and an 'unmapped' key (K_ENTER above where it shows context invalidated and emit keystroke). i'm not sure how to test for this though. Maybe something with showing markers were cleared, that seems indirect.

Also, modifier keys (incl. Caps) should not invalidate context.

srl295 commented 7 months ago

I have this working locally (PR in progress) where it distinguishes between a gap key (K_1 above - no output, no action happens) and an 'unmapped' key (K_ENTER above where it shows context invalidated and emit keystroke). i'm not sure how to test for this though. Maybe something with showing markers were cleared, that seems indirect.

Also, modifier keys (incl. Caps) should not invalidate context.

okay… how do i know if the vkey is a modifier key? do i need a list K_SHIFT, K_CAPS, etc?

mcdurdin commented 6 months ago

do i need a list K_SHIFT, K_CAPS, etc?

One sample from kmn:

https://github.com/keymanapp/keyman/blob/6669d232a606420715f3c6f6f7c7baa0669f9d3d/windows/src/engine/keyman32/k32_lowlevelkeyboardhook.cpp#L62-L76

We have a truth table for context reset:

windows/src/engine/keyman32/VKScanCodes.cpp

This comment in Keyman Engine for Windows may be interesting background:

https://github.com/keymanapp/keyman/blob/0406cb1381c6c95d436aeeb33b115cf7f09c68dd/windows/src/engine/keyman32/keybd_shift.cpp#L25-L62

srl295 commented 6 months ago

@mcdurdin thanks, sounds like i have the data i need!

mcdurdin commented 6 months ago

@mcdurdin thanks, sounds like i have the data i need!

Happy for second-guessing on some of the decisions on the vkcontextreset array -- it was done a long time ago and reasons are lost in the mists of time

srl295 commented 6 months ago

Still TODO:

srl295 commented 6 months ago

Fixed by #10236