feat(core): support stacked markers, prep for marker segments 🙀 #10326

srl295 · 2024-01-06T17:37:00Z

renamed the single-segment functions to normalize_nfd_markers_segment() since they should only be run on a single segment
commented out NFC for now, dead code
add some additional checks/DebugLog around normalization
add stacked markers tests. Some old tests had to change also.

- first we break #10320

- we can finally support single markers with the new structure

- renamed the single-segment functions to normalize_nfd_markers_segment() since they should only be run on a single segment - commented out NFC for now, dead code - add some additional checks/DebugLog around normalization #10320

keymanapp-test-bot · 2024-01-06T17:37:04Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

srl295 · 2024-01-06T17:38:27Z

core/tests/unit/ldml/test_transforms.cpp

+  {
+    marker_map map;
+    std::cout << __FILE__ << ":" << __LINE__ << "   - dup-char test" << std::endl;
+    const std::u32string src = U"a\uFFFF\u0008\u0001\u0300e\uFFFF\u0008\u0002\u0300";


as it says, normalize_nfd_markers() would get this wrong, because U+0300 appears twice. But at least the segment code handles it properly.

srl295 · 2024-01-06T17:38:59Z

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

-      <transform from="8e\u{0300}\u{0320}\m{stampy}\m{lgtm}" to="FAIL" /> <!-- denormalized, nomatch -->
-      <transform from="8e\u{0320}\u{0300}\m{stampy}\m{lgtm}" to="YES8d" /> <!-- NFD -->
+      <transform from="8e\u{0300}\u{0320}\m{stampy}\m{lgtm}" to="YES8c" /> <!-- denormalized, but matches -->
+      <transform from="8f\u{0320}\u{0300}\m{stampy}\m{lgtm}" to="YES8d" /> <!-- NFD -->


The one with FAIL didn't use to match because of two markers. But now, it can match either way! 💯

- build fix for win #10320

srl295 · 2024-01-08T18:37:33Z

mac failure is just the agreement issue

mcdurdin · 2024-01-09T00:41:14Z

mac failure is just the agreement issue

agreement issue only affects iOS. mac build failures appear to be flaky Apple timestamp service. I thought I'd added a workaround to retry for that though (#10243) which doesn't appear to be working.

mcdurdin · 2024-01-09T00:42:11Z

@jahorton can you figure out why the Test: Language Modeling Layer (Common) build failed?
https://build.palaso.org/viewLog.html?buildId=433516&buildTypeId=Keyman_Common_LMLayer_TestPullRequests

mcdurdin · 2024-01-09T00:45:11Z

agreement issue only affects iOS. mac build failures appear to be flaky Apple timestamp service. I thought I'd added a workaround to retry for that though (#10243) which doesn't appear to be working.

My bad. I conflated two different PR builds. This one was indeed the agreement issue. The timestamp failure was in #10327 (https://build.palaso.org/buildConfiguration/Keyman_KeymanMac_PullRequests/433525?expandBuildDeploymentsSection=false&hideTestsFromDependencies=false&hideProblemsFromDependencies=false&expandBuildProblemsSection=true&expandBuildChangesSection=true). Apologies.

jahorton · 2024-01-09T01:08:20Z

@jahorton can you figure out why the Test: Language Modeling Layer (Common) build failed? https://build.palaso.org/viewLog.html?buildId=433516&buildTypeId=Keyman_Common_LMLayer_TestPullRequests

01:52:46
npm ERR! npm ERR! errno ECONNRESET

01:52:46
npm ERR! npm ERR! network Invalid response body while trying to fetch https://registry.npmjs.org/@parcel%2flogger: aborted

01:52:46
npm ERR! npm ERR! network This is a problem related to network connectivity.

01:52:46
npm ERR! npm ERR! network In most cases you are behind a proxy or have bad network settings.

This has been happening from time to time for the past few months. Not necessarily on that specific package - just the loss of connectivity when trying to retrieve a package.

mcdurdin · 2024-01-10T07:31:25Z

This has been happening from time to time for the past few months. Not necessarily on that specific package - just the loss of connectivity when trying to retrieve a package.

chore(common): add retry checks around npm install (and any other npm network activities) #10350

mcdurdin

LGTM

mcdurdin · 2024-01-10T07:32:42Z

core/src/ldml/ldml_processor.cpp

+  {
+    const auto normalize_ok = ldml::normalize_nfd_markers(ctxtstr);
+    assert(normalize_ok);
+    if(!normalize_ok) {
+      DebugLog("ldml_processor::process_output: failed ldml::normalize_nfd_markers(ctxtstr)");
+    }
+  }


Do we need to abort processing in release builds?

If it fails the string is just unchanged and so the match may not work properly or wrong output. But there's no inherent danger in proceeding.

Is it worth trying to pop up an error code to the engine? I thought the answer before was, no.

No danger is what I was looking for. There's no way we can pop an error, so 'bad output' is the best error we can do. (And no, don't even think about emitting an error as key events "YOUR KEYBOARD HAS GONE WRONG" emitted every time you press a key?!)

mcdurdin · 2024-01-10T07:33:02Z

core/src/ldml/ldml_processor.cpp

+  {
+    const auto normalize_ok = ldml::normalize_nfd_markers(ctxtstr_cleanedup);
+    assert(normalize_ok);
+    if(!normalize_ok) {
+      DebugLog("ldml_processor::process_output: failed ldml::normalize_nfd_markers(ctxtstr_cleanedup)");
+    }
+  }
+


Ditto, do we need to abort processing?

core/src/ldml/ldml_transforms.cpp

core/src/ldml/ldml_transforms.hpp

- cleanup NFC code later if it's entirely unneeded. Co-authored-by: Marc Durdin <marc@durdin.net>

keyman-server · 2024-01-11T18:18:15Z

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

srl295 added 3 commits January 5, 2024 18:52

chore(core): stacked markers tests 🙀

52fe907

- first we break #10320

feat(core): stacked markers tests 🙀

f4be785

- we can finally support single markers with the new structure

feat(core): stacked markers 🙀

b2b8979

- renamed the single-segment functions to normalize_nfd_markers_segment() since they should only be run on a single segment - commented out NFC for now, dead code - add some additional checks/DebugLog around normalization #10320

srl295 requested review from mcdurdin and rc-swag as code owners January 6, 2024 17:37

srl295 self-assigned this Jan 6, 2024

srl295 added this to the A17S30 milestone Jan 6, 2024

github-actions bot added core/ Keyman Core feat labels Jan 6, 2024

srl295 commented Jan 6, 2024

View reviewed changes

srl295 added 2 commits January 6, 2024 13:56

feat(core): stacked markers 🙀

776a902

- build fix for win #10320

Merge branch 'master' into feat/core/10320-stack-markers-epic-ldml

74b5530

mcdurdin mentioned this pull request Jan 9, 2024

bug(mac): timestamp server retry does not seem to be happening #10334

Open

srl295 requested a review from jahorton January 9, 2024 01:46

mcdurdin approved these changes Jan 10, 2024

View reviewed changes

Apply suggestions from code review

0d9f543

- cleanup NFC code later if it's entirely unneeded. Co-authored-by: Marc Durdin <marc@durdin.net>

srl295 merged commit f68e9f3 into master Jan 11, 2024
17 checks passed

srl295 deleted the feat/core/10320-stack-markers-epic-ldml branch January 11, 2024 15:40

mcdurdin linked an issue Jan 12, 2024 that may be closed by this pull request

feat(core): Normalization needs to support multiple markers 🙀 #10320

Closed

mcdurdin mentioned this pull request Jan 12, 2024

feat(core): Normalization needs to support multiple markers 🙀 #10320

Closed

jahorton mentioned this pull request Jan 15, 2024

chore(common): investigate why HISTORY.md had blank entries #10351

Open

mcdurdin added the epic-core/9999-normalization label Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): support stacked markers, prep for marker segments 🙀 #10326

feat(core): support stacked markers, prep for marker segments 🙀 #10326

srl295 commented Jan 6, 2024

keymanapp-test-bot bot commented Jan 6, 2024 •

edited

Loading

srl295 Jan 6, 2024

srl295 Jan 6, 2024

srl295 commented Jan 8, 2024

mcdurdin commented Jan 9, 2024

mcdurdin commented Jan 9, 2024

mcdurdin commented Jan 9, 2024

jahorton commented Jan 9, 2024 •

edited

Loading

mcdurdin commented Jan 10, 2024 •

edited

Loading

mcdurdin left a comment

mcdurdin Jan 10, 2024

srl295 Jan 10, 2024

mcdurdin Jan 11, 2024

mcdurdin Jan 10, 2024

keyman-server commented Jan 11, 2024

feat(core): support stacked markers, prep for marker segments 🙀 #10326

feat(core): support stacked markers, prep for marker segments 🙀 #10326

Conversation

srl295 commented Jan 6, 2024

keymanapp-test-bot bot commented Jan 6, 2024 • edited Loading

User Test Results

Test Artifacts

srl295 Jan 6, 2024

Choose a reason for hiding this comment

srl295 Jan 6, 2024

Choose a reason for hiding this comment

srl295 commented Jan 8, 2024

mcdurdin commented Jan 9, 2024

mcdurdin commented Jan 9, 2024

mcdurdin commented Jan 9, 2024

jahorton commented Jan 9, 2024 • edited Loading

mcdurdin commented Jan 10, 2024 • edited Loading

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Jan 10, 2024

Choose a reason for hiding this comment

srl295 Jan 10, 2024

Choose a reason for hiding this comment

mcdurdin Jan 11, 2024

Choose a reason for hiding this comment

mcdurdin Jan 10, 2024

Choose a reason for hiding this comment

keyman-server commented Jan 11, 2024

keymanapp-test-bot bot commented Jan 6, 2024 •

edited

Loading

jahorton commented Jan 9, 2024 •

edited

Loading

mcdurdin commented Jan 10, 2024 •

edited

Loading