From dc965f4e1ad56b66475932339d7d639aa8220382 Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Thu, 18 Apr 2024 08:57:19 -0500 Subject: [PATCH 1/2] fix(core): ldml backspace processing should delete all markers - current code would only delete a single marker and fall through - should loop consuming all markers until a non-marker char is hit --- core/src/ldml/ldml_processor.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/ldml/ldml_processor.cpp b/core/src/ldml/ldml_processor.cpp index 9667a2275af..b39d80d56d0 100644 --- a/core/src/ldml/ldml_processor.cpp +++ b/core/src/ldml/ldml_processor.cpp @@ -227,7 +227,7 @@ ldml_processor::process_key_up(ldml_event_state &ldml_state) void ldml_processor::process_backspace(ldml_event_state &ldml_state) const { if (!!bksp_transforms) { - // process with an empty string voa the bksp transforms + // process with an empty string via the bksp transforms auto matchedContext = process_output(ldml_state, std::u32string(), bksp_transforms.get()); if (matchedContext > 0) { @@ -245,15 +245,16 @@ void ldml_event_state::emit_backspace() { // attempt to get the last char // TODO-LDML: emoji backspace auto end = state->context().rbegin(); - if (end != state->context().rend()) { + while (end != state->context().rend()) { if ((*end).type == KM_CORE_CT_CHAR) { actions.code_points_to_delete++; state->context().pop_back(); return; } else if ((*end).type == KM_CORE_BT_MARKER) { - // nothing in actions + // remove any markers before char state->context().pop_back(); - // falls through - end wasn't a character + end++; + // loop again } } /* From fa1fd7515772ec3872f3dbc6bfd770864eb737aa Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Thu, 25 Apr 2024 16:59:20 -0500 Subject: [PATCH 2/2] fix(core): ldml fix for multiple marker deletion - current code only deletes a single marker and falls through - update the ldml test code, get rid of 'expected character' backspace logic (now that we have context object) - update test cases --- core/src/ldml/ldml_processor.cpp | 12 ++-- .../unit/ldml/keyboards/k_210_marker-test.xml | 61 +++++++++++++++++++ .../unit/ldml/keyboards/k_210_marker.xml | 12 +++- core/tests/unit/ldml/ldml.cpp | 61 ++++++++++--------- 4 files changed, 108 insertions(+), 38 deletions(-) diff --git a/core/src/ldml/ldml_processor.cpp b/core/src/ldml/ldml_processor.cpp index b39d80d56d0..f90fcfaa946 100644 --- a/core/src/ldml/ldml_processor.cpp +++ b/core/src/ldml/ldml_processor.cpp @@ -246,17 +246,15 @@ void ldml_event_state::emit_backspace() { // TODO-LDML: emoji backspace auto end = state->context().rbegin(); while (end != state->context().rend()) { - if ((*end).type == KM_CORE_CT_CHAR) { + if (end->type == KM_CORE_CT_CHAR) { actions.code_points_to_delete++; state->context().pop_back(); return; - } else if ((*end).type == KM_CORE_BT_MARKER) { - // remove any markers before char - state->context().pop_back(); - end++; - // loop again } - } + // else loop again + assert(end->type != KM_CORE_CT_END); // inappropriate here. + state->context().pop_back(); + } /* We couldn't find a character at end of context (context is empty), so we'll pass the backspace keystroke on to the app to process; the diff --git a/core/tests/unit/ldml/keyboards/k_210_marker-test.xml b/core/tests/unit/ldml/keyboards/k_210_marker-test.xml index 5a36d1941b3..22865b12c5c 100644 --- a/core/tests/unit/ldml/keyboards/k_210_marker-test.xml +++ b/core/tests/unit/ldml/keyboards/k_210_marker-test.xml @@ -62,4 +62,65 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/core/tests/unit/ldml/keyboards/k_210_marker.xml b/core/tests/unit/ldml/keyboards/k_210_marker.xml index 0d25de75ac6..6f2eb9021a9 100644 --- a/core/tests/unit/ldml/keyboards/k_210_marker.xml +++ b/core/tests/unit/ldml/keyboards/k_210_marker.xml @@ -17,14 +17,15 @@ + - + - + @@ -56,5 +57,12 @@ + + + + + + + diff --git a/core/tests/unit/ldml/ldml.cpp b/core/tests/unit/ldml/ldml.cpp index 6b19bb57870..13450c94672 100644 --- a/core/tests/unit/ldml/ldml.cpp +++ b/core/tests/unit/ldml/ldml.cpp @@ -39,8 +39,6 @@ namespace { bool g_beep_found = false; -bool g_already_complained = false; - km_core_option_item test_env_opts[] = { KM_CORE_OPTIONS_END @@ -115,49 +113,54 @@ apply_action( {(uint32_t)act.marker}}); break; case KM_CORE_IT_BACK: + { + // single char removed in context + km_core_usv ch = 0; + bool matched_text = false; + // assume the backspace came from set_action() and there's no further info. + assert(act.backspace.expected_type == KM_CORE_BT_CHAR); + assert(act.backspace.expected_value == 0); // It is valid for a backspace to be received with an empty text store // as the user can press backspace with no text in the store and Keyman // will pass that back to the client, as the client may do additional // processing at start of a text store, e.g. delete from a previous cell // in a table. Or, if Keyman has a cached context, then there may be // additional text in the text store that Keyman can't see. - if (act.backspace.expected_type == KM_CORE_BT_MARKER) { - assert(!context.empty()); - assert(context.back().type == KM_CORE_CT_MARKER); - context.pop_back(); - // no change to text store. - } else if (text_store.length() > 0) { + // If there's anything in the text store, pop it off. Two if a pair. + if (text_store.length() > 0) { assert(!context.empty() && !text_store.empty()); - km_core_usv ch = text_store.back(); + const auto ch1 = text_store.back(); text_store.pop_back(); - if (text_store.length() > 0 && Uni_IsSurrogate2(ch)) { - auto ch1 = text_store.back(); - if (Uni_IsSurrogate1(ch1)) { + if (text_store.length() > 0 && Uni_IsSurrogate2(ch1)) { + const auto ch2 = text_store.back(); + if (Uni_IsSurrogate1(ch2)) { // We'll only pop the next character off it is actually a // surrogate pair - ch = Uni_SurrogateToUTF32(ch1, ch); + ch = Uni_SurrogateToUTF32(ch2, ch1); // reverse order text_store.pop_back(); + } else { + ch = 0xFFFF; // unpaired } + } else { + ch = ch1; // single char } - if (act.backspace.expected_type == KM_CORE_BT_CHAR) { - if (act.backspace.expected_value == 0) { - // using set_action() doesn't provide for expected backspaces, so can't validate here - // only complain once. - if (!g_already_complained) { - std::cerr << "Note: TODO-LDML: not validating backspace.expected_value nor ch - no information available." << std::endl; - g_already_complained = true; - } - } else { - assert(ch == act.backspace.expected_value); - assert(context.back().character == ch); + } else { + matched_text = true; // no text to match as context is empty. + } + // now, we need to simulate what ldml_processor::emit_backspace() is going to do. + auto end = context.rbegin(); + while (end != context.rend()) { + if (end->type == KM_CORE_CT_CHAR) { + assert(!matched_text); + assert_equal(end->character, ch); // expect popped char to be same as what's in context + matched_text = true; + context.pop_back(); + break; // exit on first real char } - assert(context.back().type == KM_CORE_CT_CHAR); + assert(end->type != KM_CORE_CT_END); // inappropriate here. context.pop_back(); - } else { - // assume it's otherwise KM_CORE_BT_UNKNOWN - assert(act.backspace.expected_type == KM_CORE_BT_UNKNOWN); - assert(context.empty()); // if KM_CORE_BT_UNKNOWN, context should be empty. } + assert(matched_text); } break; case KM_CORE_IT_PERSIST_OPT: