Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): ldml backspace processing should delete all markers 🙀 #11254

Merged
merged 3 commits into from
Apr 26, 2024
Merged

fix(core): ldml backspace processing should delete all markers 🙀 #11254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc965f4
fix(core): ldml backspace processing should delete all markers
srl295 Apr 18, 2024
fa1fd75
fix(core): ldml fix for multiple marker deletion
srl295 Apr 25, 2024
44f0dea
Merge branch 'fix/core/10955-reset-on-frame' into fix/core/10955-doub…
srl295 Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions core/src/ldml/ldml_processor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -245,17 +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()) {
if ((*end).type == KM_CORE_CT_CHAR) {
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
state->context().pop_back();
// falls through - end wasn't a character
}
}
// 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
Expand Down
61 changes: 61 additions & 0 deletions core/tests/unit/ldml/keyboards/k_210_marker-test.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,65 @@
<check result="´"/>
</test>
</tests>
<tests name="double-marker-10955">
<!-- unmatched delete should delete ALL markers, not just one -->
<test name="double-marker-10955-null">
<keystroke key="v" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<keystroke key="b" />
<check result="v2-squiggles" />
</test>
<test name="double-marker-10955-del-1">
<keystroke key="v" />
<keystroke key="squiggle" />
<backspace />
<keystroke key="b" />
<check result="no-squiggles" />
</test>
<test name="double-marker-10955-del-2">
<keystroke key="v" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<backspace />
<keystroke key="b" />
<check result="no-squiggles" />
</test>
<test name="double-marker-10955-del-3">
<keystroke key="v" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<backspace />
<keystroke key="b" />
<check result="no-squiggles" />
</test>
<test name="double-marker-10955-del-2-1">
<keystroke key="v" />
<keystroke key="v" />
<keystroke key="squiggle" />
<backspace />
<keystroke key="b" />
<check result="vno-squiggles" />
</test>
<test name="double-marker-10955-del-2-2">
<keystroke key="v" />
<keystroke key="v" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<backspace />
<keystroke key="b" />
<check result="vno-squiggles" />
</test>
<test name="double-marker-10955-del-2-3">
<keystroke key="v" />
<keystroke key="v" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<keystroke key="squiggle" />
<backspace />
<keystroke key="b" />
<check result="vno-squiggles" />
</test>
</tests>
</keyboardTest3>
12 changes: 10 additions & 2 deletions core/tests/unit/ldml/keyboards/k_210_marker.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@
<key id="acute" output="\m{acute}" />
<key id="caret" output="C" /> <!-- see transform -->
<key id="hacek" output="H" /> <!-- see transform -->
<key id="squiggle" output="\m{squiggle}" />
</keys>

<layers formId="us">
<layer modifiers="none" id="base">
<row keys="grave acute caret hacek" />
<row keys="grave acute caret hacek squiggle" />
<row keys="q w e" /> <!-- etc -->
<row keys="a s d" /> <!-- etc -->
<row keys="z x c v" /> <!-- etc -->
<row keys="z x c v b" /> <!-- etc -->
</layer>
</layers>

Expand Down Expand Up @@ -56,5 +57,12 @@
<transform from="\m{grave}" to="_" /> <!-- trailing grave becomes _ -->
<!-- no cleanup for trailing acute -->
</transformGroup>
<!-- for the do uble-marker-10955delete tests -->
<transformGroup>
<transform from="\m{squiggle}\m{squiggle}\m{squiggle}b" to="3-squiggles"/>
<transform from="\m{squiggle}\m{squiggle}b" to="2-squiggles"/>
<transform from="\m{squiggle}b" to="1-squiggles"/>
<transform from="b" to="no-squiggles"/>
</transformGroup>
</transforms>
</keyboard3>
61 changes: 32 additions & 29 deletions core/tests/unit/ldml/ldml.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
Loading