feat(core): ldml improve key-not-found 🙀 #10090

srl295 · 2023-11-29T00:11:41Z

distinguish between unmapped and 0-length output strings
don't do any processing for 0-length output strings
no change to developer, that will be next
remove a comment referencing transform=no

- distinguish between unmapped and 0-length output strings - don't do any processing for 0-length output strings - no change to developer, that will be next - remove a comment referencing transform=no For: #9451

keymanapp-test-bot · 2023-11-29T00:11:51Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

srl295 · 2023-11-29T00:12:58Z

PTAL and see if it makes sense. The NEXT step will be to change kmc to actually emit some kind of pseudo-gap ( essentially <key gap="true" /> for all keys in a layout that aren't othewise specified.

mcdurdin

This is looking pretty close. There are some questions around modifier keys and key combinations that I think we need to cover

mcdurdin · 2023-11-29T05:07:04Z

core/src/ldml/ldml_processor.cpp


-  if (key_str.empty()) {
+  if (!found) {


We need to exclude modifiers (incl. Caps) from this test. Press and release Ctrl or Shift should not invalidate context. Press and release Alt on Windows is a special case -- because it opens the menu. But it may not need to invalidate context because other actions will do that anyway.

However, Ctrl+A should invalidate context. So we need to be aware of the modifier state of key combinations and invalidate context for missing key events. And if we define Ctrl+Q in the keyboard, does that mean that all other Ctrl+key combinations will be treated as gap? Philosophy time.

Also, I wonder how many keyboard developers will get annoyed when their spacebar doesn't work until they add a key for it?

we're only on key down here, for one thing. key up events are silently ignored. (line 185) - should that change?

if Ctrl-A was assigned as a key, then it should not invalidate context. So I'm going to need some more details here...

space - we could detect space here and simply send it to the OS, invalidating context. But, since we already have an implied space key in LDML, kmc could simply slip that one in unless it's otherwise defined. (i.e. instead of gap). The net effect would be that if you define no keys you'll get a keyboard with one functional key- the spacebar.

seems like there's enough here that it's time for a CLDR ticket: https://unicode-org.atlassian.net/browse/CLDR-17262

we're only on key down here, for one thing. key up events are silently ignored. (line 185) - should that change?

The key up events don't tend to harm if the key is already up (in my experience). So should be okay to go through.

if Ctrl-A was assigned as a key, then it should not invalidate context. So I'm going to need some more details here...

If Ctrl+A is assigned, then yes, it gets processed as normal. If it isn't assigned, however, and falls back through to the app (as a user would expect), doing a select-all on Windows for example, then we would want a context reset.

key up

I don't think you mean "all keyup events trigger passthrough/context reset" as that would be disruptive to markers. so I'm not sure what the code should do different here.

ctrl=A

OK so far.

I'm still unclear on which vkeys need to be passed through to the OS, causing a context reset. is there a list? should the list be in CLDR?

srl295 · 2023-11-29T20:11:59Z

the other question is how in the world can i best test this besides with a debugger?

- restructure keyup/keydown code some - new utility function emit_invalidate_passthrough_keystroke() For: #9451

srl295 · 2023-11-29T20:32:51Z

@mcdurdin I've restructured a bit to make it easier to pass-through keys as needed pending that discussion.

- unused nit fix For: #9451

mcdurdin · 2023-11-29T22:51:10Z

the other question is how in the world can i best test this besides with a debugger?

the heuristic of losing markers is actually pretty good.

srl295 · 2023-11-29T23:32:14Z

the other question is how in the world can i best test this besides with a debugger?

the heuristic of losing markers is actually pretty good.

Seems a little in direct, but I'll give it a try
I assume the test harness isn't going to try to do any default output, just a reset

- also affects markers, feat(core): normalization per spec for transforms/etc 🙀 #9468 - keep markers in nfd context string - fix ldml test harness to handle context reset - update test case - still issues with overproduction of markers in the context For: #9451

srl295 · 2023-12-07T00:35:25Z

little bit of a side excursion fixing a markers issue. #9468

Commit is now much improved, but overproducing markers in the context:

- key action: ['K_BKSP' 0x8]/modifier Unmodified 0x0
context   : U+0036 U+0065  [6e]
testcontext U+0036 \m{1} \m{1} \m{1} \m{1} U+0065
- check expected
expected  : U+0059 U+0045 U+0053 U+0036  [YES6]
text store: U+0036 U+0065  [6e]

Still to-do is to get back to the key-not-found logic.

- don't skip markers when calling context_to_string()! Oops. - update docs on ldml_processor::remove_text() - update remove_text() to handle markers in the context string. This is really: #9468

srl295 · 2023-12-07T20:02:52Z

core/src/ldml/ldml_processor.cpp

@@ -279,7 +285,7 @@ size_t ldml_processor::process_output(km_core_state *state, const std::u32string
  assert(ldml::normalize_nfd_markers(nfd_str)); // TODO-LDML: else fail?
  // extract context string, in NFD
  std::u32string old_ctxtstr_nfd;
-  (void)context_to_string(state, old_ctxtstr_nfd, false);
+  (void)context_to_string(state, old_ctxtstr_nfd, true);


sure enough, it was NOT INCLUDING MARKERS until this was turned to true. (that's what 'false' means)

srl295 · 2023-12-07T20:03:22Z

core/src/ldml/ldml_processor.cpp

@@ -316,8 +322,8 @@ size_t ldml_processor::process_output(km_core_state *state, const std::u32string

  // Ok. We've done all the happy manipulations.

-  /** NFD and no markers */
-  std::u32string ctxtstr_cleanedup = ldml::remove_markers(ctxtstr);


sure enough it was, surprisingly, REMOVING MARKERS

srl295 · 2023-12-07T20:04:21Z

core/src/ldml/ldml_processor.cpp

-  // TODO-LDML: need to emit marker here - need to emit text w/ markers, and handle appropriately.
-  // // TODO-LDML: 1-marker hack! need to support a string with intermixed markers.
-  if (str.length() == 3 && str[0] == LDML_UC_SENTINEL && str[1] == LDML_MARKER_CODE) {
-    emit_marker(state, str[2]);
-  }


@mcdurdin this is the hack I was mentioning. This PR is thus now a major boost to #9468

srl295 · 2023-12-07T20:05:11Z

core/src/ldml/ldml_processor.cpp

+void ldml_processor::emit_invalidate_passthrough_keystroke(km_core_state *state) {
+  state->actions().push_invalidate_context();
+  state->actions().push_emit_keystroke();
+}
+


encapsulating this in case it needs to change later. i guess I could pass the vkey here?

srl295 · 2023-12-07T20:05:49Z

core/src/ldml/ldml_vkeys.cpp

@@ -27,12 +27,12 @@ static const uint16_t BOTH_ALT  = LALTFLAG  | RALTFLAG;
 static const uint16_t BOTH_CTRL = LCTRLFLAG | RCTRLFLAG;

 std::u16string
-vkeys::lookup(km_core_virtual_key vk, uint16_t modifier_state) const {
+vkeys::lookup(km_core_virtual_key vk, uint16_t modifier_state, bool &found) const {


empty string return now means "no output", distinct from "not found"

srl295 · 2023-12-07T20:07:35Z

at this point I'm not going to try to split this into two PRs, key-not-found vs markers. If I realized the test cases were going to bump up against that so quickly, i might have done so.

srl295 · 2023-12-07T20:09:04Z

the other question is how in the world can i best test this besides with a debugger?

the heuristic of losing markers is actually pretty good.

Test it with markers, they said. What could go wrong, they said.

If test plans can be faithful wounds, this is one of 'em! 😆

- copy exception table for reset from VKScanCodes.cpp - update test For: #9451

srl295 · 2023-12-07T23:17:17Z

I copied the table from VKScanCodes.cpp , it seemed to be what i wanted, ptal…

mcdurdin · 2023-12-07T23:48:48Z

I copied the table from VKScanCodes.cpp , it seemed to be what i wanted, ptal…

I wonder if we should be trying to centralize that as a common shared file under /common/cpp/scancodes/some_file_name_here.cpp?

srl295 · 2023-12-08T04:56:22Z

I copied the table from VKScanCodes.cpp , it seemed to be what i wanted, ptal…

I wonder if we should be trying to centralize that as a common shared file under /common/cpp/scancodes/some_file_name_here.cpp?

maybe a good item for a future PR? ?

mcdurdin · 2023-12-12T02:05:30Z

core/src/ldml/ldml_processor.cpp

+
+
+void ldml_processor::emit_invalidate_passthrough_keystroke(km_core_state *state, km_core_virtual_key vk, uint16_t _kmn_unused(modifier_state)) {
+  if (ldml_VKContextReset[vk]) {


do we need to vk<256? assert() for debug builds?

mcdurdin · 2023-12-12T02:06:51Z

core/src/ldml/ldml_processor.cpp

+// from VKScanCodes.cpp
+static const int ldml_VKContextReset[256] = {


This should probably be in Core as a separate .cpp, rather than embedded in ldml_processor.cpp

mcdurdin

LGTM, pending the assert just noted and if possible moving the vkscancodes stuff into a separate cpp

- move vkey_to_contextreset table into a common spot in core - assert that vk < 0x100 to avoid running off the table For: #9451

keyman-server · 2023-12-13T03:41:46Z

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

feat(core): ldml improve key-not-found 🙀

11334ed

- distinguish between unmapped and 0-length output strings - don't do any processing for 0-length output strings - no change to developer, that will be next - remove a comment referencing transform=no For: #9451

keymanapp-test-bot bot added this to the A17S27 milestone Nov 29, 2023

github-actions bot added core/ Keyman Core feat labels Nov 29, 2023

srl295 self-assigned this Nov 29, 2023

srl295 mentioned this pull request Nov 29, 2023

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

Closed

mcdurdin requested changes Nov 29, 2023

View reviewed changes

srl295 added 2 commits November 29, 2023 14:29

feat(core): ldml improve key-not-found 🙀

84cc59c

- restructure keyup/keydown code some - new utility function emit_invalidate_passthrough_keystroke() For: #9451

Merge branch 'master' into fix/core/9451-key-not-found-epic-ldml

95e4a20

feat(core): ldml improve key-not-found 🙀

4d22b17

- unused nit fix For: #9451

srl295 added 2 commits December 7, 2023 19:54

feat(core): ldml_processor fix for markers 🙀

2071d25

- don't skip markers when calling context_to_string()! Oops. - update docs on ldml_processor::remove_text() - update remove_text() to handle markers in the context string. This is really: #9468

Merge branch 'master' into fix/core/9451-key-not-found-epic-ldml

3c95c85

srl295 commented Dec 7, 2023

View reviewed changes

feat(core): ldml improve key-not-found 🙀

1e32a2a

- copy exception table for reset from VKScanCodes.cpp - update test For: #9451

srl295 linked an issue Dec 7, 2023 that may be closed by this pull request

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

Closed

srl295 requested a review from mcdurdin December 7, 2023 23:16

srl295 marked this pull request as ready for review December 7, 2023 23:16

srl295 requested a review from rc-swag as a code owner December 7, 2023 23:16

mcdurdin modified the milestones: A17S27, A17S28 Dec 8, 2023

srl295 added 2 commits December 8, 2023 18:39

Merge branch 'master' into fix/core/9451-key-not-found-epic-ldml

44afc6c

Merge branch 'master' into fix/core/9451-key-not-found-epic-ldml

d628427

srl295 removed a link to an issue Dec 12, 2023

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

Closed

mcdurdin reviewed Dec 12, 2023

View reviewed changes

mcdurdin approved these changes Dec 12, 2023

View reviewed changes

mcdurdin mentioned this pull request Dec 12, 2023

feat(windows): Investigate if VKContextReset is needed in for core on kmx processor #10227

Closed

2 tasks

feat(core): ldml improve key-not-found 🙀

1cd97fa

- move vkey_to_contextreset table into a common spot in core - assert that vk < 0x100 to avoid running off the table For: #9451

srl295 merged commit 2604870 into master Dec 12, 2023
17 checks passed

srl295 deleted the fix/core/9451-key-not-found-epic-ldml branch December 12, 2023 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): ldml improve key-not-found 🙀 #10090

feat(core): ldml improve key-not-found 🙀 #10090

srl295 commented Nov 29, 2023

keymanapp-test-bot bot commented Nov 29, 2023 •

edited

Loading

srl295 commented Nov 29, 2023

mcdurdin left a comment

mcdurdin Nov 29, 2023

srl295 Nov 29, 2023

mcdurdin Nov 29, 2023

srl295 Nov 30, 2023

srl295 commented Nov 29, 2023 •

edited

Loading

srl295 commented Nov 29, 2023

mcdurdin commented Nov 29, 2023

srl295 commented Nov 29, 2023 •

edited

Loading

srl295 commented Dec 7, 2023

srl295 Dec 7, 2023 •

edited

Loading

srl295 Dec 7, 2023

srl295 Dec 7, 2023

srl295 Dec 7, 2023

srl295 Dec 7, 2023

srl295 commented Dec 7, 2023

srl295 commented Dec 7, 2023 •

edited

Loading

srl295 commented Dec 7, 2023

mcdurdin commented Dec 7, 2023

srl295 commented Dec 8, 2023

mcdurdin Dec 12, 2023

mcdurdin Dec 12, 2023

mcdurdin left a comment

keyman-server commented Dec 13, 2023



		void ldml_processor::emit_invalidate_passthrough_keystroke(km_core_state *state, km_core_virtual_key vk, uint16_t _kmn_unused(modifier_state)) {
		if (ldml_VKContextReset[vk]) {

		// from VKScanCodes.cpp
		static const int ldml_VKContextReset[256] = {

feat(core): ldml improve key-not-found 🙀 #10090

feat(core): ldml improve key-not-found 🙀 #10090

Conversation

srl295 commented Nov 29, 2023

keymanapp-test-bot bot commented Nov 29, 2023 • edited Loading

User Test Results

Test Artifacts

srl295 commented Nov 29, 2023

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Nov 29, 2023 • edited Loading

srl295 commented Nov 29, 2023

mcdurdin commented Nov 29, 2023

srl295 commented Nov 29, 2023 • edited Loading

srl295 commented Dec 7, 2023

srl295 Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Dec 7, 2023

srl295 commented Dec 7, 2023 • edited Loading

srl295 commented Dec 7, 2023

mcdurdin commented Dec 7, 2023

srl295 commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin left a comment

Choose a reason for hiding this comment

keyman-server commented Dec 13, 2023

keymanapp-test-bot bot commented Nov 29, 2023 •

edited

Loading

srl295 commented Nov 29, 2023 •

edited

Loading

srl295 commented Nov 29, 2023 •

edited

Loading

srl295 Dec 7, 2023 •

edited

Loading

srl295 commented Dec 7, 2023 •

edited

Loading