refactor(core): KM_CORE_IT_INVALIDATE_CONTEXT shouldn't be no-op, or there should be other api to handle it

wengxt commented 6 months ago

Describe the bug

I'm trying to implement with new get_actions API, and noticed that there's no equivalent for KM_CORE_IT_INVALIDATE_CONTEXT in the km_core_actions struct.

I think it's still necessary have a flag to indicate whether KM_CORE_IT_INVALIDATE_CONTEXT appear in actions. Especially for application that doesn't support surrounding text on linux.

Reproduce the bug

km_core_actions doesn't have a bool invalidate_context field.

Expected behavior

No response

Related issues

No response

Keyman apps

[ ] Keyman for Android
[ ] Keyman for iPhone and iPad
[X] Keyman for Linux
[ ] Keyman for macOS
[ ] Keyman for Windows
[ ] Keyman Developer
[ ] KeymanWeb
[ ] Other - give details at bottom of form

Keyman version

17.0.225

Operating system

Linux

Device

No response

Target application

No response

Browser

No response

Keyboard name

telex

Keyboard version

No response

Language name

No response

Additional context

No response

mcdurdin commented 6 months ago

Appreciate you jumping on this change so quickly.

Note that the existing action queue will continue to be available in 17.0, and in 18.0 we'll hopefully move to using the km_core_actions struct through all the Engines.

However, context management is changing in 17.0. In earlier versions, ownership of context was ambiguous and so there was a lot of work keeping Engine context in sync with Core context, particularly for Non-Compliant Client Apps.

With the new actions struct model, the Core owns the context, and you should move to the km_core_state_context_set_if_needed API to pass in context from the Client App if it is available. This means that the Engine never needs to invalidate the context, because its responsibility ends at the point of passing the context from the Client App to the Core.

Note also there is still one open issue around the changes:

10100

See #10065 for how the context ownership change was implemented in Keyman Engine for Windows

We have not yet updated the documentation (https://github.com/keymanapp/help.keyman.com/issues/909) but hopefully will be able to do that very soon.

At this point, none of the other engines have been fully updated to use the new km_core_actions struct, so there may be other issues that we discover as we implement the change. The macOS Engine is closest:

10066

Notes

Glossary of terms: https://github.com/keymanapp/keyman/wiki/Keyman-Glossary

wengxt commented 6 months ago

@mcdurdin So for example under following scenario: viet_velex type "thuang" would produce thuang in context, and the enter will generate "invalidate context", "emit key stroke".

In an application (including any xim based app) that does not support context at all, what should we do here?

If we don't clear context, the next "s" will wrongly use the "thuang" in the cache.

mcdurdin commented 6 months ago

The Enter key triggers a context reset internally in Core, because it is a non-character key (even if in some contexts it emits a \n).

mcdurdin commented 6 months ago

(But, this would be a good example of a unit test to verify that things are working as we intend!)

wengxt commented 6 months ago

at least I didn't see that from happening in the log

D2023-12-07 18:02:02.526529 instance.cpp:905] KeyEvent: Key(Return states=0) rawKey: Key(Return states=0) origKey: Key(Return states=0) Release:0 keycode: 36
D2023-12-07 18:02:02.526722 engine.cpp:182] Set context from application: thuang
D2023-12-07 18:02:02.526756 engine.cpp:446] before process key event context: thuang
D2023-12-07 18:02:02.526790 engine.cpp:448] km_mod_state=0
364359405        ../../../src/kmx/kmx_context.cpp:108        Set        KMX_Context::Set(): ENTER [6]: (6) U+0074 U+0068 U+0075 U+0061 U+006E U+0067
364359405        ../../../src/kmx/kmx_context.cpp:109        Set                                        (6) U+0074 U+0068 U+0075 U+0061 U+006E U+0067
364359405        ../../../src/kmx/kmx_processevent.cpp:220        ProcessGroup        m_state.vkey: ['K_ENTER' 0xd] shiftFlags: 0; charCode: 0
364359405        ../../../src/kmx/kmx_processevent.cpp:221        ProcessGroup        m_context: U+0074 U+0068 U+0075 U+0061 U+006E U+0067
364359413        ../../../src/kmx/kmx_processevent.cpp:260        ProcessGroup        No match was found in group 0 of 1
364359413        ../../../src/kmx/kmx_processevent.cpp:303        ProcessGroup         ... IsLegacy = FALSE; IsTIP = TRUE
D2023-12-07 18:02:02.534080 engine.cpp:451] after process key event context: thuang
<- This log tries to read context from state and nothing is changed.
D2023-12-07 18:02:02.534133 engine.cpp:458] BACK action 0
D2023-12-07 18:02:02.534166 engine.cpp:505] EMIT_KEYSTROKE action
D2023-12-07 18:02:02.534194 engine.cpp:510] PERSIST_OPT action
D2023-12-07 18:02:02.534234 engine.cpp:536] after processing all actions
D2023-12-07 18:02:02.534331 inputcontext.cpp:323] KeyEvent handling time: 7ms result:0

rc-swag commented 6 months ago

This is a good example. The point when you read the core context using the state should have had the cache cleared. I will have a closer look. As a quick check I just tried this example with an app that is not context-aware with the Keyman for Windows alpha build that uses the new km_core_state_context_set_if_needed (it does not use the action struct yet though) and it correctly handled the thuang Enter s case. However, as @mcdurdin we will check whether we have a test like this in unit tests for core and if not add it.

Edit: Just going to add the line where the core context will be cleared when processing the actions https://github.com/keymanapp/keyman/blob/21c012208bc453e4bebcdcfeaf039f758b1f8cf9/core/src/kmx/kmx_processor.cpp#L250C37-L250C37

ermshiperete commented 6 months ago

keymanapp / keyman