feat(core): support stacked markers, prep for marker segments 🙀#10326
feat(core): support stacked markers, prep for marker segments 🙀#10326
Conversation
- 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
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| { | ||
| 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"; |
There was a problem hiding this comment.
as it says, normalize_nfd_markers() would get this wrong, because U+0300 appears twice. But at least the segment code handles it properly.
| <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 --> |
There was a problem hiding this comment.
The one with FAIL didn't use to match because of two markers. But now, it can match either way! 💯
|
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. |
|
@jahorton can you figure out why the Test: Language Modeling Layer (Common) build failed? |
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. |
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. |
|
| { | ||
| 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)"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need to abort processing in release builds?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?!)
| { | ||
| 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)"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Ditto, do we need to abort processing?
- cleanup NFC code later if it's entirely unneeded. Co-authored-by: Marc Durdin <marc@durdin.net>
|
Changes in this pull request will be available for download in Keyman version 17.0.242-alpha |
#10320
@keymanapp-test-bot skip