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.

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): reset on frame keys 🙀 #11172

Merged
merged 27 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c86ab58
chore(core): reinstate context reset test in ldml
srl295 Apr 4, 2024
c91095f
fix(core): core to automatically reset context if a frame key pressed
srl295 Apr 4, 2024
2298f11
Merge branch 'chore/core/11112-lone-surrogate' into fix/core/10955-re…
srl295 Apr 5, 2024
f1b0d8c
fix(core): core to automatically reset context if a frame key pressed
srl295 Apr 5, 2024
628d13d
fix(core): core to automatically reset context if a frame key pressed
srl295 Apr 5, 2024
784a994
fix(core): update ldml test source to handle reset
srl295 Apr 5, 2024
55483e4
fix(core): update ldml test source to handle reset
srl295 Apr 5, 2024
a455c5b
fix(core): update core to handle reset context
srl295 Apr 5, 2024
11b0712
fix(core): update core to handle reset context
srl295 Apr 5, 2024
111a50c
fix(core): dx: denoise test
srl295 Apr 5, 2024
51c93ac
fix(core): update core to handle reset context
srl295 Apr 5, 2024
3da900d
Merge branch 'chore/core/11112-lone-surrogate' into fix/core/10955-re…
srl295 Apr 8, 2024
9ca4e99
fix(core): update core to handle reset context
srl295 Apr 8, 2024
d0ca92a
Merge branch 'beta' into fix/core/10955-reset-on-frame
srl295 Apr 11, 2024
46239c6
Update core/src/state.hpp
srl295 Apr 12, 2024
bd10290
fix(core): update core to handle reset context
srl295 Apr 12, 2024
699d576
Merge branch 'beta' into fix/core/10955-reset-on-frame
srl295 Apr 12, 2024
4f8c9eb
fix(core): update core to handle reset context
srl295 Apr 15, 2024
37530ac
fix(core): update core to handle reset context
srl295 Apr 16, 2024
5c04599
Merge branch 'beta' into fix/core/10955-reset-on-frame
srl295 Apr 16, 2024
4f2ef49
fix(core): update core to handle reset context
srl295 Apr 17, 2024
8d35422
Merge branch 'beta' into fix/core/10955-reset-on-frame
srl295 Apr 17, 2024
f77bbb2
fix(core): update state_should_invalidate_context
srl295 Apr 18, 2024
331ff33
Merge branch 'beta' into fix/core/10955-reset-on-frame
srl295 Apr 18, 2024
ba8dcad
Apply suggestions from code review
srl295 Apr 24, 2024
f516538
chore(core): outdent test file per review comment
srl295 Apr 24, 2024
cd867ba
fix(core): update vkey_should_invalidate_context per comment
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
21 changes: 21 additions & 0 deletions core/src/km_core_processevent_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,27 @@ km_core_process_event(km_core_state *state,
}
km_core_status status = state->processor().process_event(state, vk, modifier_state, is_key_down, event_flags);

if (state_should_invalidate_context(state, vk, modifier_state, is_key_down, event_flags)) {
state->context().clear();
// we are already committed. So we need to un-commit (remove the end of the vector)
if (state->actions().back().type == KM_CORE_IT_END) {
state->actions().pop_back();
}
bool foundEmit = false;
km::core::action oldAction;
// try to put the invalidate item before the emit keystroke
Comment on lines +60 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to add some explanation of why we are doing this -- why do we need to add the invalidate context before emit keystroke, etc.

if (state->actions().back().type == KM_CORE_IT_EMIT_KEYSTROKE) {
oldAction = state->actions().back();
state->actions().pop_back();
foundEmit = true;
}
state->actions().push_invalidate_context();
if (foundEmit) {
state->actions().push_back(oldAction);
}
state->actions().commit();
}

state->apply_actions_and_merge_app_context();
srl295 marked this conversation as resolved.
Show resolved Hide resolved

return status;
Expand Down
33 changes: 31 additions & 2 deletions core/src/km_core_state_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@

#include "processor.hpp"
#include "state.hpp"

#include "vkey_to_contextreset.hpp"
#include "kmx_file.h"

using namespace km::core;

Expand Down Expand Up @@ -363,4 +364,32 @@ km_core_cp * km_core_state_context_debug(
result[s.size()] = 0;

return result;
}
}

static bool
state_has_action_type(km_core_state *state, uint8_t type) {
return std::any_of(
state->actions().begin(), state->actions().end(),
[type](const km::core::action &a) { return a.type == type; });
}

bool
state_should_invalidate_context(km_core_state *state,
km_core_virtual_key vk,
uint16_t modifier_state,
uint8_t is_key_down,
uint16_t _kmn_unused(event_flags)) {
// if emit_keystroke is present, check if a context reset is needed
if (state_has_action_type(state, KM_CORE_IT_EMIT_KEYSTROKE)) {
if (vk == KM_CORE_VKEY_BKSP && state->context().empty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test going to be before or after the context has been manipulated by the rule processor? That is, if we have a single char in context, then user presses backspace, could this result in a context reset when we don't want it? (Yes it may not cause trouble but I want us to be clear on our boundaries and order of execution)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that suggests that bksp for the first character will delete that character and also pass it back to the app for a second deletion?

Copy link
Contributor

@rc-swag rc-swag Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct, we can't rely on the state->context().empty(). I originally thought we could use code_points_to_delete value from the action struct. However, the kmx processor currently only gives the action list. This information could be found by checking each action item for KM_CORE_IT_BACK then action_items->backspace.expected_type==KM_CORE_BT_CHAR.
I think there a few cleaner choices here.

  1. Move the new code we have in km_core_state_api starting at line 55
 if (state_should_invalidate_context(state, vk, modifier_state, is_key_down, event_flags)) {
    state->context().clear();

To inside state::apply_actions_and_merge_app_context() after line action_item_list_to_actions_object(action_items, &this->_action_struct);. This way we can just look at the action struct for code_points_to_delete. A bonus here is that this will also mean no change is needed when the kmx_processor is updated to use the action_struct instead of action_list. Unfortunately it would mean the helper functions @srl295 has written would have to be updated to use the action struct.

  1. Go with what we discussed as an option in or zoom meeting. Turn km_core_bool emit_keystroke; member of the action struct to a tri-state.
0 = do nothing; 1 = emit_key_stroke; 2 = emit_key_stroke and invalidate context. 

This puts the decision back on the processors though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srl295
Further thoughts looking again at the "kmx_processor" and the "ldml processor". All we have to do is remove the check for state->context().empty().
In the case of a backspace key being pressed the processors only set KM_CORE_IT_EMIT_KEYSTROKE when the context is empty. Therefore that check has already been made. That is if action is KM_CORE_IT_EMIT_KEYSTROKE and vkey== KM_CORE_VKEY_BKSP. We do not need to check if the context is empty here.

I think we can keep the change suggest in option 1 for the future when the action list is deprecated.

return true; // context is empty - so pass back
} else if (is_key_down &&
// either a ctrl or alt modifier
((modifier_state & (K_CTRLFLAG | K_ALTFLAG | LCTRLFLAG | RCTRLFLAG | LALTFLAG | RALTFLAG))
// or a frame key
srl295 marked this conversation as resolved.
Show resolved Hide resolved
|| vkey_to_contextreset[vk])) {
return true;
}
}
return false;
}
272 changes: 0 additions & 272 deletions core/src/kmx/kmx_consts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,278 +104,6 @@ const struct char_to_vkey s_char_to_vkey[] = {
{0, 0, 0}
};

const bool vkey_to_contextreset[256] = {
true, //L"K_?00", // &H0
true, //L"K_LBUTTON", // &H1
true, //L"K_RBUTTON", // &H2
true, //L"K_CANCEL", // &H3
true, //L"K_MBUTTON", // &H4
true, //L"K_?05", // &H5
true, //L"K_?06", // &H6
true, //L"K_?07", // &H7
true, //L"K_BKSP", // &H8
true, //L"K_TAB", // &H9
true, //L"K_?0A", // &HA
true, //L"K_?0B", // &HB
true, //L"K_KP5", // &HC
true, //L"K_ENTER", // &HD
true, //L"K_?0E", // &HE
true, //L"K_?0F", // &HF
false, //L"K_SHIFT", // &H10
false, //L"K_CONTROL", // &H11
false, //L"K_ALT", // &H12
true, //L"K_PAUSE", // &H13
false, //L"K_CAPS", // &H14
true, //L"K_KANJI?15", // &H15
true, //L"K_KANJI?16", // &H16
true, //L"K_KANJI?17", // &H17
true, //L"K_KANJI?18", // &H18
true, //L"K_KANJI?19", // &H19
true, //L"K_?1A", // &H1A
true, //L"K_ESC", // &H1B
true, //L"K_KANJI?1C", // &H1C
true, //L"K_KANJI?1D", // &H1D
true, //L"K_KANJI?1E", // &H1E
true, //L"K_KANJI?1F", // &H1F
false, //L"K_SPACE", // &H20
true, //L"K_PGUP", // &H21
true, //L"K_PGDN", // &H22
true, //L"K_END", // &H23
true, //L"K_HOME", // &H24
true, //L"K_LEFT", // &H25
true, //L"K_UP", // &H26
true, //L"K_RIGHT", // &H27
true, //L"K_DOWN", // &H28
true, //L"K_SEL", // &H29
true, //L"K_PRINT", // &H2A
true, //L"K_EXEC", // &H2B
true, //L"K_PRTSCN", // &H2C
false, //L"K_INS", // &H2D
true, //L"K_DEL", // &H2E
true, //L"K_HELP", // &H2F
false, //L"K_0", // &H30
false, //L"K_1", // &H31
false, //L"K_2", // &H32
false, //L"K_3", // &H33
false, //L"K_4", // &H34
false, //L"K_5", // &H35
false, //L"K_6", // &H36
false, //L"K_7", // &H37
false, //L"K_8", // &H38
false, //L"K_9", // &H39
false, //L"K_?3A", // &H3A
false, //L"K_?3B", // &H3B
false, //L"K_?3C", // &H3C
false, //L"K_?3D", // &H3D
false, //L"K_?3E", // &H3E
false, //L"K_?3F", // &H3F
false, //L"K_?40", // &H40

false, //L"K_A", // &H41
false, //L"K_B", // &H42
false, //L"K_C", // &H43
false, //L"K_D", // &H44
false, //L"K_E", // &H45
false, //L"K_F", // &H46
false, //L"K_G", // &H47
false, //L"K_H", // &H48
false, //L"K_I", // &H49
false, //L"K_J", // &H4A
false, //L"K_K", // &H4B
false, //L"K_L", // &H4C
false, //L"K_M", // &H4D
false, //L"K_N", // &H4E
false, //L"K_O", // &H4F
false, //L"K_P", // &H50
false, //L"K_Q", // &H51
false, //L"K_R", // &H52
false, //L"K_S", // &H53
false, //L"K_T", // &H54
false, //L"K_U", // &H55
false, //L"K_V", // &H56
false, //L"K_W", // &H57
false, //L"K_X", // &H58
false, //L"K_Y", // &H59
false, //L"K_Z", // &H5A
false, //L"K_?5B", // &H5B
false, //L"K_?5C", // &H5C
false, //L"K_?5D", // &H5D
false, //L"K_?5E", // &H5E
false, //L"K_?5F", // &H5F
false, //L"K_NP0", // &H60
false, //L"K_NP1", // &H61
false, //L"K_NP2", // &H62
false, //L"K_NP3", // &H63
false, //L"K_NP4", // &H64
false, //L"K_NP5", // &H65
false, //L"K_NP6", // &H66
false, //L"K_NP7", // &H67
false, //L"K_NP8", // &H68
false, //L"K_NP9", // &H69
false, //L"K_NPSTAR", // &H6A
false, //L"K_NPPLUS", // &H6B
false, //L"K_SEPARATOR", // &H6C
false, //L"K_NPMINUS", // &H6D
false, //L"K_NPDOT", // &H6E
false, //L"K_NPSLASH", // &H6F
true, //L"K_F1", // &H70
true, //L"K_F2", // &H71
true, //L"K_F3", // &H72
true, //L"K_F4", // &H73
true, //L"K_F5", // &H74
true, //L"K_F6", // &H75
true, //L"K_F7", // &H76
true, //L"K_F8", // &H77
true, //L"K_F9", // &H78
true, //L"K_F10", // &H79
true, //L"K_F11", // &H7A
true, //L"K_F12", // &H7B
true, //L"K_F13", // &H7C
true, //L"K_F14", // &H7D
true, //L"K_F15", // &H7E
true, //L"K_F16", // &H7F
true, //L"K_F17", // &H80
true, //L"K_F18", // &H81
true, //L"K_F19", // &H82
true, //L"K_F20", // &H83
true, //L"K_F21", // &H84
true, //L"K_F22", // &H85
true, //L"K_F23", // &H86
true, //L"K_F24", // &H87

false, //L"K_?88", // &H88
false, //L"K_?89", // &H89
false, //L"K_?8A", // &H8A
false, //L"K_?8B", // &H8B
false, //L"K_?8C", // &H8C
false, //L"K_?8D", // &H8D
false, //L"K_?8E", // &H8E
false, //L"K_?8F", // &H8F

false, //L"K_NUMLOCK", // &H90
false, //L"K_SCROLL", // &H91

false, //L"K_?92", // &H92
false, //L"K_?93", // &H93
false, //L"K_?94", // &H94
false, //L"K_?95", // &H95
false, //L"K_?96", // &H96
false, //L"K_?97", // &H97
false, //L"K_?98", // &H98
false, //L"K_?99", // &H99
false, //L"K_?9A", // &H9A
false, //L"K_?9B", // &H9B
false, //L"K_?9C", // &H9C
false, //L"K_?9D", // &H9D
false, //L"K_?9E", // &H9E
false, //L"K_?9F", // &H9F
false, //L"K_?A0", // &HA0
false, //L"K_?A1", // &HA1
false, //L"K_?A2", // &HA2
false, //L"K_?A3", // &HA3
false, //L"K_?A4", // &HA4
false, //L"K_?A5", // &HA5
false, //L"K_?A6", // &HA6
false, //L"K_?A7", // &HA7
false, //L"K_?A8", // &HA8
false, //L"K_?A9", // &HA9
false, //L"K_?AA", // &HAA
false, //L"K_?AB", // &HAB
false, //L"K_?AC", // &HAC
false, //L"K_?AD", // &HAD
false, //L"K_?AE", // &HAE
false, //L"K_?AF", // &HAF
false, //L"K_?B0", // &HB0
false, //L"K_?B1", // &HB1
false, //L"K_?B2", // &HB2
false, //L"K_?B3", // &HB3
false, //L"K_?B4", // &HB4
false, //L"K_?B5", // &HB5
false, //L"K_?B6", // &HB6
false, //L"K_?B7", // &HB7
false, //L"K_?B8", // &HB8
false, //L"K_?B9", // &HB9

false, //L"K_COLON", // &HBA
false, //L"K_EQUAL", // &HBB
false, //L"K_COMMA", // &HBC
false, //L"K_HYPHEN", // &HBD
false, //L"K_PERIOD", // &HBE
false, //L"K_SLASH", // &HBF
false, //L"K_BKQUOTE", // &HC0

false, //L"K_?C1", // &HC1
false, //L"K_?C2", // &HC2
false, //L"K_?C3", // &HC3
false, //L"K_?C4", // &HC4
false, //L"K_?C5", // &HC5
false, //L"K_?C6", // &HC6
false, //L"K_?C7", // &HC7
false, //L"K_?C8", // &HC8
false, //L"K_?C9", // &HC9
false, //L"K_?CA", // &HCA
false, //L"K_?CB", // &HCB
false, //L"K_?CC", // &HCC
false, //L"K_?CD", // &HCD
false, //L"K_?CE", // &HCE
false, //L"K_?CF", // &HCF
false, //L"K_?D0", // &HD0
false, //L"K_?D1", // &HD1
false, //L"K_?D2", // &HD2
false, //L"K_?D3", // &HD3
false, //L"K_?D4", // &HD4
false, //L"K_?D5", // &HD5
false, //L"K_?D6", // &HD6
false, //L"K_?D7", // &HD7
false, //L"K_?D8", // &HD8
false, //L"K_?D9", // &HD9
false, //L"K_?DA", // &HDA

false, //L"K_LBRKT", // &HDB
false, //L"K_BKSLASH", // &HDC
false, //L"K_RBRKT", // &HDD
false, //L"K_QUOTE", // &HDE
false, //L"K_oDF", // &HDF
false, //L"K_oE0", // &HE0
false, //L"K_oE1", // &HE1
false, //L"K_oE2", // &HE2
false, //L"K_oE3", // &HE3
false, //L"K_oE4", // &HE4

false, //L"K_?E5", // &HE5

false, //L"K_oE6", // &HE6

false, //L"K_?E7", // &HE7
false, //L"K_?E8", // &HE8

false, //L"K_oE9", // &HE9
false, //L"K_oEA", // &HEA
false, //L"K_oEB", // &HEB
false, //L"K_oEC", // &HEC
false, //L"K_oED", // &HED
false, //L"K_oEE", // &HEE
false, //L"K_oEF", // &HEF
false, //L"K_oF0", // &HF0
false, //L"K_oF1", // &HF1
false, //L"K_oF2", // &HF2
false, //L"K_oF3", // &HF3
false, //L"K_oF4", // &HF4
false, //L"K_oF5", // &HF5

false, //L"K_?F6", // &HF6
false, //L"K_?F7", // &HF7
false, //L"K_?F8", // &HF8
false, //L"K_?F9", // &HF9
false, //L"K_?FA", // &HFA
false, //L"K_?FB", // &HFB
false, //L"K_?FC", // &HFC
false, //L"K_?FD", // &HFD
false, //L"K_?FE", // &HFE
false, //L"K_?FF" // &HFF
};


} // namespace kmx
} // namespace core
} // namespace km
2 changes: 0 additions & 2 deletions core/src/kmx/kmx_processevent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ KMX_BOOL KMX_ProcessEvent::ProcessGroup(LPGROUP gp, KMX_BOOL *pOutputKeystroke)
// If there is now no character in the context, we want to
// emit the backspace for application to use
if(!pdeletecontext || *pdeletecontext == 0) { // I4933
m_actions.QueueAction(QIT_INVALIDATECONTEXT, 0);
if(m_debug_items) {
m_debug_items->push_group_exit(m_actions.Length(), KM_CORE_DEBUG_FLAG_NOMATCH, gp);
}
Expand All @@ -301,7 +300,6 @@ KMX_BOOL KMX_ProcessEvent::ProcessGroup(LPGROUP gp, KMX_BOOL *pOutputKeystroke)
return FALSE;
} else { // I4024 // I4128 // I4287 // I4290
DebugLog(" ... IsLegacy = FALSE; IsTIP = TRUE"); // I4128
m_actions.QueueAction(QIT_INVALIDATECONTEXT, 0);
if(m_debug_items) {
m_debug_items->push_group_exit(m_actions.Length(), KM_CORE_DEBUG_FLAG_NOMATCH, gp);
}
Expand Down
3 changes: 0 additions & 3 deletions core/src/kmx/kmx_processevent.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,6 @@ struct char_to_vkey {

extern const struct char_to_vkey s_char_to_vkey[];

/** for vkeys 0..FF, 'true' if a context reset should be performed before emit */
extern const bool vkey_to_contextreset[];

} // namespace kmx
} // namespace core
} // namespace km
1 change: 1 addition & 0 deletions core/src/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ kmx_files = files(
'keyboard.cpp',
'state.cpp',
'debuglog.cpp',
'vkey_to_contextreset.cpp',
'km_core_action_api.cpp',
'km_core_context_api.cpp',
'km_core_keyboard_api.cpp',
Expand Down
Loading
Loading