Merge pull request #5489 from keymanapp/chore/common/core/5488-delete-markers-reliably

chore(common/core): handle deletion of markers in actions 🐞
This commit is contained in:
Marc Durdin 2021-07-28 11:09:10 +10:00 committed by GitHub
commit d2d0b4e53c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 212 additions and 60 deletions

View file

@ -471,14 +471,28 @@ services framework to transform the text store in the Client Application, among
other actions.
```c
*/
typedef struct {
uint8_t expected_type; // km_kbp_backspace_type
uintptr_t expected_value; // used mainly in unit tests
} km_kbp_backspace_item;
enum km_kbp_backspace_type {
KM_KBP_BT_UNKNOWN = 0, // Used at beginning of context; user-initiated backspace
KM_KBP_BT_CHAR = 1, // Deleting a character prior to insertion point
KM_KBP_BT_MARKER = 2, // Deleting a marker prior to insertion point
KM_KBP_BT_MAX_TYPE_ID
};
typedef struct {
uint8_t type;
uint8_t _reserved[sizeof(void*)-sizeof(uint8_t)];
union {
uintptr_t marker; // MARKER type
km_kbp_option_item const * option; // OPT types
km_kbp_usv character; // CHAR type
uint8_t capsLock; // CAPSLOCK type, 1 to turn on, 0 to turn off
uintptr_t marker; // MARKER type
km_kbp_option_item const * option; // OPT types
km_kbp_usv character; // CHAR type
uint8_t capsLock; // CAPSLOCK type, 1 to turn on, 0 to turn off
km_kbp_backspace_item backspace; // BACKSPACE type
};
} km_kbp_action_item;

View file

@ -76,6 +76,11 @@ namespace {
*sz_ptr = buf_size - (e - i);
// Skip over any final markers - they are execluded from context
while(ci->type == KM_KBP_CT_MARKER) {
ci++;
}
return ci->type == KM_KBP_CT_END
? KM_KBP_STATUS_OK
: KM_KBP_STATUS_INSUFFICENT_BUFFER;

View file

@ -74,6 +74,15 @@ kmx_processor::update_option(
return option(scope, key, value);
}
void pop_context_push_backspace_action(km_kbp_state *state) {
assert(!state->context().empty());
auto item = state->context().back();
state->context().pop_back();
state->actions().push_backspace(
item.type == KM_KBP_CT_MARKER ? KM_KBP_BT_MARKER : KM_KBP_BT_CHAR,
item.type == KM_KBP_CT_MARKER ? item.marker : item.character);
}
km_kbp_status
kmx_processor::process_event(
km_kbp_state *state,
@ -141,28 +150,41 @@ kmx_processor::process_event(
case BK_DEFAULT:
// This only happens if we know we have context to delete. Last item must be a character
assert(!state->context().empty() && state->context().back().type != KM_KBP_IT_MARKER);
if (!state->context().empty())
state->context().pop_back();
state->actions().push_backspace();
if(!state->context().empty()) {
pop_context_push_backspace_action(state);
} else {
// Note: only runs on non-debug build, fail safe
state->actions().push_backspace(KM_KBP_BT_UNKNOWN);
}
break;
case BK_DEADKEY:
// This only happens if we know we have context to delete. Last item must be a deadkey
assert(!state->context().empty() && state->context().back().type == KM_KBP_IT_MARKER);
if (!state->context().empty())
state->context().pop_back();
if(!state->context().empty()) {
pop_context_push_backspace_action(state);
} else {
// Note: only runs on non-debug build, fail safe
state->actions().push_backspace(KM_KBP_BT_UNKNOWN);
}
break;
case BK_BACKSPACE:
// User-initiated backspace. We need to delete deadkeys from context, both sides of the character deleted
while (!state->context().empty() && state->context().back().type == KM_KBP_IT_MARKER)
state->context().pop_back();
if (!state->context().empty()) {
state->context().pop_back();
while (!state->context().empty() && state->context().back().type == KM_KBP_IT_MARKER)
state->context().pop_back();
while (!state->context().empty() && state->context().back().type == KM_KBP_IT_MARKER) {
pop_context_push_backspace_action(state);
}
if (!state->context().empty()) {
state->actions().push_backspace(KM_KBP_BT_CHAR, state->context().back().character);
state->context().pop_back();
} else {
// Even if context is empty, we send the backspace event, because we may not
// know the context.
state->actions().push_backspace(KM_KBP_BT_UNKNOWN);
}
// Delete deadkey markers prior to char
while (!state->context().empty() && state->context().back().type == KM_KBP_IT_MARKER) {
pop_context_push_backspace_action(state);
}
// Even if context is empty, we send the backspace event, because we may not
// know the context.
state->actions().push_backspace();
break;
default:
assert(false);

View file

@ -127,7 +127,7 @@ namespace km {
{
case KM_KBP_VKEY_BKSP:
state->context().pop_back();
state->actions().push_backspace();
state->actions().push_backspace(KM_KBP_BT_UNKNOWN); // Assuming we don't know the character
break;
case KM_KBP_VKEY_F2:

View file

@ -39,7 +39,7 @@ public:
void push_character(km_kbp_usv usv);
void push_marker(uintptr_t marker);
void push_alert();
void push_backspace();
void push_backspace(km_kbp_backspace_type expected_type, uintptr_t expected_value = 0);
void push_persist(option const &);
void push_persist(option const &&);
void push_emit_keystroke(km_kbp_virtual_key vk=0);
@ -82,9 +82,12 @@ void actions::push_alert() {
inline
void actions::push_backspace() {
void actions::push_backspace(km_kbp_backspace_type expected_type, uintptr_t expected_value) {
assert(empty() || (!empty() && back().type != KM_KBP_IT_END));
emplace_back(km_kbp_action_item {KM_KBP_IT_BACK, {0,}, {0}});
km_kbp_action_item item = {KM_KBP_IT_BACK};
item.backspace.expected_type = expected_type;
item.backspace.expected_value = expected_value;
emplace_back(item);
}

View file

@ -37,6 +37,13 @@ void print_action_item(const char *title, km_kbp_action_item const & item) {
case KM_KBP_IT_CHAR:
std::cout << " char: '" << std::u32string(1, item.character) << "' (" << item.character << ")" << std::endl;
break;
case KM_KBP_IT_BACK:
std::cout << " delete: " <<
(item.backspace.expected_type == KM_KBP_BT_CHAR ? "char" :
item.backspace.expected_type == KM_KBP_BT_MARKER ? "marker" :
"unknown") << " (" <<
item.backspace.expected_value << ")" << std::endl;
break;
case KM_KBP_IT_PERSIST_OPT:
std::cout
<< " option: key: " << item.option->key << std::endl
@ -57,7 +64,8 @@ bool operator==(
case KM_KBP_IT_CHAR: result = lhs.character == rhs.character; break;
case KM_KBP_IT_MARKER: result = lhs.marker == rhs.marker; break;
case KM_KBP_IT_ALERT: break;
case KM_KBP_IT_BACK: break;
case KM_KBP_IT_BACK: result = lhs.backspace.expected_type == rhs.backspace.expected_type &&
lhs.backspace.expected_value == rhs.backspace.expected_value; break;
case KM_KBP_IT_PERSIST_OPT: result = *lhs.option == *rhs.option; break;
case KM_KBP_IT_EMIT_KEYSTROKE: break;
case KM_KBP_IT_INVALIDATE_CONTEXT: break;

View file

@ -152,7 +152,6 @@ void test_basic_rule_matches() {
}));
try_status(km_kbp_process_event(test_state, KM_KBP_VKEY_E, KM_KBP_MODIFIER_SHIFT, 1));
assert(debug_items(test_state, {
km_kbp_state_debug_item{KM_KBP_DEBUG_BEGIN, KM_KBP_DEBUG_FLAG_UNICODE, {KM_KBP_VKEY_E, KM_KBP_MODIFIER_SHIFT, 'E'}},
km_kbp_state_debug_item{KM_KBP_DEBUG_GROUP_ENTER, 0, {}, {u"", &gp}},
@ -166,7 +165,6 @@ void test_basic_rule_matches() {
}));
try_status(km_kbp_process_event(test_state, KM_KBP_VKEY_F, KM_KBP_MODIFIER_SHIFT, 1));
assert(debug_items(test_state, {
km_kbp_state_debug_item{KM_KBP_DEBUG_BEGIN, KM_KBP_DEBUG_FLAG_UNICODE, {KM_KBP_VKEY_F, KM_KBP_MODIFIER_SHIFT, 'F'}},
km_kbp_state_debug_item{KM_KBP_DEBUG_GROUP_ENTER, 0, {}, {u"", &gp}},
@ -176,9 +174,17 @@ void test_basic_rule_matches() {
km_kbp_state_debug_item{KM_KBP_DEBUG_END, 0, {}, {u"", nullptr, nullptr, {}, 5 /* from above */ }},
}));
km_kbp_action_item bksp_d = {KM_KBP_IT_BACK};
bksp_d.backspace.expected_type = KM_KBP_BT_CHAR;
bksp_d.backspace.expected_value = 'D';
km_kbp_action_item bksp_e = {KM_KBP_IT_BACK};
bksp_e.backspace.expected_type = KM_KBP_BT_CHAR;
bksp_e.backspace.expected_value = 'E';
assert(action_items(test_state, {
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK},
bksp_e,
bksp_d,
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv(u'\u0E04')}},
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv(u'\u0E05')}},
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv(u'\u0E06')}},
@ -221,15 +227,18 @@ void test_multiple_groups() {
km_kbp_state_debug_item{KM_KBP_DEBUG_END, 0, {}, {u"", nullptr, nullptr, {}, 3}}, // action item will emit a 'b'
}));
km_kbp_action_item bksp_a = {KM_KBP_IT_BACK};
bksp_a.backspace.expected_type = KM_KBP_BT_CHAR;
bksp_a.backspace.expected_value = 'a';
assert(action_items(test_state, {
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('a')}},
{KM_KBP_IT_BACK},
bksp_a,
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('b')}},
{KM_KBP_IT_END}
}));
try_status(km_kbp_process_event(test_state, KM_KBP_VKEY_2, 0, 1));
assert(debug_items(test_state, {
km_kbp_state_debug_item{KM_KBP_DEBUG_BEGIN, KM_KBP_DEBUG_FLAG_UNICODE, {KM_KBP_VKEY_2, 0, '2'}},
km_kbp_state_debug_item{KM_KBP_DEBUG_GROUP_ENTER, 0, {}, {u"", &gp}},
@ -251,8 +260,12 @@ void test_multiple_groups() {
km_kbp_state_debug_item{KM_KBP_DEBUG_END, 0, {}, {u"", nullptr, nullptr, {}, 4}}, // action item will "abc"
}));
km_kbp_action_item bksp_b = {KM_KBP_IT_BACK};
bksp_b.backspace.expected_type = KM_KBP_BT_CHAR;
bksp_b.backspace.expected_value = 'b';
assert(action_items(test_state, {
{KM_KBP_IT_BACK},
bksp_b,
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('a')}},
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('b')}},
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('c')}},
@ -295,7 +308,6 @@ void test_store_offsets() {
}));
try_status(km_kbp_process_event(test_state, KM_KBP_VKEY_B, 0, 1));
assert(debug_items(test_state, {
km_kbp_state_debug_item{KM_KBP_DEBUG_BEGIN, KM_KBP_DEBUG_FLAG_UNICODE, {KM_KBP_VKEY_B, 0, 'b'}},
km_kbp_state_debug_item{KM_KBP_DEBUG_GROUP_ENTER, 0, {}, {u"", &gp}},
@ -310,11 +322,26 @@ void test_store_offsets() {
km_kbp_state_debug_item{KM_KBP_DEBUG_END, 0, {}, {u"", nullptr, nullptr, {}, 6}},
}));
km_kbp_action_item bksp[] = {
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK}
};
bksp[0].backspace.expected_type = KM_KBP_BT_CHAR;
bksp[1].backspace.expected_type = KM_KBP_BT_CHAR;
bksp[2].backspace.expected_type = KM_KBP_BT_CHAR;
bksp[3].backspace.expected_type = KM_KBP_BT_CHAR;
bksp[0].backspace.expected_value = 'y';
bksp[1].backspace.expected_value = 'a';
bksp[2].backspace.expected_value = 'x';
bksp[3].backspace.expected_value = 'e';
assert(action_items(test_state, {
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK},
{KM_KBP_IT_BACK},
bksp[0],
bksp[1],
bksp[2],
bksp[3],
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('e')}},
{KM_KBP_IT_CHAR, {0,}, {km_kbp_usv('x')}},
{KM_KBP_IT_END}

View file

@ -7,15 +7,16 @@ c 5. One char and two deadkeys in context 'a' dk(5a) dk(5b) + '5'
c 6. One char and two deadkeys and one char in context 'a' dk(6a) dk(6b) 'b' + '6'
c 7. Two deadkeys and one char in context dk(7a) dk(7b) 'a' + '7'
c 8. Three deadkeys in context dk(8a) dk(8b) dk(8c) + '8'
c keys: [K_1][K_X][K_2][K_X][K_3][K_X][K_4][K_X][K_5][K_X][K_6][K_X][K_7][K_X][K_8][K_X]
c expected: 1=OK 2=OK 3=OK 4=OK 5=OK 6=OK 7=OK 8=OK
c context:
c 9. Replacing deadkeys in context via rule [9][x][x]
c keys: [K_1][K_X][K_2][K_X][K_3][K_X][K_4][K_X][K_5][K_X][K_6][K_X][K_7][K_X][K_8][K_X][K_9][K_X][K_X]
c expected: 1=OK 2=OK 3=OK 4=OK 5=OK 6=OK 7=OK 8=OK 9=OK
c context:
store(&VERSION) '9.0'
begin unicode > use(main)
group(main) using keys
group(main) using keys
+ '1' > dk(1)
dk(1) + 'x' > '1=OK '
@ -41,10 +42,17 @@ dk(7a) 'a' + 'x' > '7=Fail1 '
dk(7b) 'a' + 'x' > '7=Fail2 '
+ '8' > dk(8a) dk(8b) dk(8c)
dk(8a) dk(8b) dk(8c) + 'x' > '8=OK'
dk(8a) dk(8b) dk(8c) + 'x' > '8=OK '
dk(8a) + 'x' > '8=Fail1 '
dk(8b) + 'x' > '8=Fail2 '
dk(8c) + 'x' > '8=Fail3 '
dk(8a) dk(8b) + 'x' > '8=Fail4 '
dk(8a) dk(8c) + 'x' > '8=Fail5 '
dk(8b) dk(8c) + 'x' > '8=Fail6 '
c [K_9] [K_X] [K_X] -> test that
+ '9' > '{' dk(9a) dk(9b) '}' dk(9c) dk(9d)
dk(9b) '}' dk(9c) dk(9d) + 'x' > dk(9e) '}' dk(9f) dk(9g)
'{' dk(9a) dk(9e) '}' dk(9f) dk(9g) + 'x' > '9=OK'

View file

@ -24,13 +24,8 @@
#include "state.hpp"
#include "utfcodec.hpp"
#define try_status(expr) \
{auto __s = (expr); if (__s != KM_KBP_STATUS_OK) std::exit(100*__LINE__+__s);}
#ifdef assert
#undef assert
#endif
#define assert(expr) {if (!(expr)) std::exit(100*__LINE__); }
#include "../test_assert.h"
#include "../test_color.h"
namespace
{
@ -161,7 +156,7 @@ key_event next_key(std::string &keys) {
}
}
void apply_action(km_kbp_state const *, km_kbp_action_item const & act, std::u16string & text_store, kmx_options &options) {
void apply_action(km_kbp_state const *, km_kbp_action_item const & act, std::u16string & text_store, std::vector<km_kbp_context_item> & context, kmx_options &options) {
switch (act.type)
{
case KM_KBP_IT_END:
@ -172,6 +167,7 @@ void apply_action(km_kbp_state const *, km_kbp_action_item const & act, std::u16
//std::cout << "beep" << std::endl;
break;
case KM_KBP_IT_CHAR:
context.push_back(km_kbp_context_item{KM_KBP_CT_CHAR, {0,}, {act.character}});
if (Uni_IsSMP(act.character)) {
text_store.push_back(Uni_UTF32ToSurrogate1(act.character));
text_store.push_back(Uni_UTF32ToSurrogate2(act.character));
@ -183,6 +179,7 @@ void apply_action(km_kbp_state const *, km_kbp_action_item const & act, std::u16
break;
case KM_KBP_IT_MARKER:
//std::cout << "deadkey(" << act.marker << ")" << std::endl;
context.push_back(km_kbp_context_item{KM_KBP_CT_MARKER, {0,}, {(uint32_t)act.marker}});
break;
case KM_KBP_IT_BACK:
// It is valid for a backspace to be received with an empty text store
@ -191,17 +188,29 @@ void apply_action(km_kbp_state const *, km_kbp_action_item const & act, std::u16
// 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 (text_store.length() > 0) {
auto ch = text_store.back();
if(act.backspace.expected_type == KM_KBP_BT_MARKER) {
assert(!context.empty());
assert(context.back().type == KM_KBP_CT_MARKER);
context.pop_back();
}
else if (text_store.length() > 0) {
assert(!context.empty() && !text_store.empty());
km_kbp_usv ch = text_store.back();
text_store.pop_back();
if(text_store.length() > 0 && Uni_IsSurrogate2(ch)) {
ch = text_store.back();
if(Uni_IsSurrogate1(ch)) {
auto ch1 = text_store.back();
if(Uni_IsSurrogate1(ch1)) {
// We'll only pop the next character off it is actually a
// surrogate pair
ch = Uni_SurrogateToUTF32(ch1, ch);
text_store.pop_back();
}
}
assert(ch == act.backspace.expected_value);
assert(context.back().type == KM_KBP_CT_CHAR);
assert(context.back().character == ch);
context.pop_back();
}
break;
case KM_KBP_IT_PERSIST_OPT:
@ -323,6 +332,13 @@ int run_test(const km::kbp::path &source, const km::kbp::path &compiled) {
km_kbp_context_item *citems = nullptr;
try_status(km_kbp_context_items_from_utf16(context.c_str(), &citems));
try_status(km_kbp_context_set(km_kbp_state_context(test_state), citems));
// Make a copy of the setup context for the test
std::vector<km_kbp_context_item> test_context;
for(km_kbp_context_item *ci = citems; ci->type != KM_KBP_CT_END; ci++) {
test_context.emplace_back(*ci);
}
km_kbp_context_items_dispose(citems);
// Setup baseline text store
@ -337,12 +353,37 @@ int run_test(const km::kbp::path &source, const km::kbp::path &compiled) {
if (p.vk == KM_KBP_VKEY_CAPS) {
toggle_caps_lock_state();
}
for (auto key_down = 1; key_down >= 0; key_down--) {
try_status(km_kbp_process_event(test_state, p.vk, p.modifier_state | caps_lock_state(), key_down));
for (auto act = km_kbp_state_action_items(test_state, nullptr); act->type != KM_KBP_IT_END; act++) {
apply_action(test_state, *act, text_store, options);
apply_action(test_state, *act, text_store, test_context, options);
}
}
// Compare context and text store at each step - should be identical
size_t n = 0;
try_status(km_kbp_context_get(km_kbp_state_context(test_state), &citems));
try_status(km_kbp_context_items_to_utf16(citems, nullptr, &n));
km_kbp_cp *buf = new km_kbp_cp[n];
try_status(km_kbp_context_items_to_utf16(citems, buf, &n));
// Verify that both our local test_context and the core's test_state.context have
// not diverged
auto ci = citems;
for(auto test_ci = test_context.begin(); ci->type != KM_KBP_CT_END || test_ci != test_context.end(); ci++, test_ci++) {
assert(ci->type != KM_KBP_CT_END && test_ci != test_context.end()); // Verify that both lists are same length
assert(test_ci->type == ci->type && test_ci->marker == ci->marker);
}
km_kbp_context_items_dispose(citems);
if (text_store != buf) {
std::cerr << "text store has diverged from buf" << std::endl;
std::cerr << "text store: " << string_to_hex(text_store) << " [" << text_store << "]" << std::endl;
std::cerr << "context : " << string_to_hex(buf) << " [" << buf << "]" << std::endl;
assert(false);
}
}
// Test if the beep action was as expected
@ -354,6 +395,15 @@ int run_test(const km::kbp::path &source, const km::kbp::path &compiled) {
try_status(km_kbp_context_items_to_utf16(citems, nullptr, &n));
km_kbp_cp *buf = new km_kbp_cp[n];
try_status(km_kbp_context_items_to_utf16(citems, buf, &n));
// Verify that both our local test_context and the core's test_state.context have
// not diverged
auto ci = citems;
for(auto test_ci = test_context.begin(); ci->type != KM_KBP_CT_END || test_ci != test_context.end(); ci++, test_ci++) {
assert(ci->type != KM_KBP_CT_END && test_ci != test_context.end()); // Verify that both lists are same length
assert(test_ci->type == ci->type && test_ci->marker == ci->marker);
}
km_kbp_context_items_dispose(citems);
std::cout << "expected : " << string_to_hex(expected) << " [" << expected << "]" << std::endl;
@ -522,21 +572,34 @@ int load_source(const km::kbp::path & path, std::string & keys, std::u16string &
}
constexpr const auto help_str = "\
kmx <KMN_FILE> <KMX_FILE>\n\
kmx [--color] <KMN_FILE> <KMX_FILE>\n\
help:\n\
\tKMN_FILE:\tThe source file for the keyboard under test.\n\
\tKMX_FILE:\tThe corresponding compiled kmx file produced from KMN_FILE.\n";
} // namespace
int main(int argc, char *argv[])
{
if (argc < 3)
{
int error_args() {
std::cerr << "kmx: Not enough arguments." << std::endl;
std::cout << help_str;
return 1;
}
int main(int argc, char *argv[]) {
int first_arg = 1;
if (argc < 3) {
return error_args();
}
auto arg_color = std::string(argv[1]) == "--color";
if(arg_color) {
first_arg++;
if(argc < 4) {
return error_args();
}
}
console_color::enabled = console_color::isaterminal() || arg_color;
km::kbp::kmx::g_debug_ToConsole = TRUE;
return run_test(argv[1], argv[2]);
return run_test(argv[first_arg], argv[first_arg+1]);
}

View file

@ -650,7 +650,9 @@ process_backspace_action(
size_t num_action_items
) {
IBusKeymanEngine *keyman = (IBusKeymanEngine *)engine;
if (keyman->char_buffer != NULL) {
if (action_items[i].backspace.expected_type == KM_KBP_IT_MARKER) {
g_message("skipping marker type");
} else if (keyman->char_buffer != NULL) {
// ibus_keyman_engine_commit_string(keyman, keyman->char_buffer);
g_message("removing one utf8 char from CHAR buffer");
glong end_pos = g_utf8_strlen(keyman->char_buffer, -1);