Skip to content

feat(core): add km_core_state_context_debug API 🌱#10377

Merged
mcdurdin merged 8 commits intoepic/core/9999-normalizationfrom
feat/core/10365-context-debug
Jan 16, 2024
Merged

feat(core): add km_core_state_context_debug API 🌱#10377
mcdurdin merged 8 commits intoepic/core/9999-normalizationfrom
feat/core/10365-context-debug

Conversation

@mcdurdin
Copy link
Copy Markdown
Member

Fixes #10365.

Adds a km_core_state_context_debug function that returns a km_core_cp string containing the current cached or intermediate context. The function must be used only for debug logging purposes -- the string is not intended to parsed for other purposes and the format may change in the future.

This commit adds the function and corresponding unit tests. Subsequent commits will update existing context debug functions in various engines to use this API endpoint instead.

@keymanapp-test-bot skip

Fixes #10365.

Adds a `km_core_state_context_debug` function that returns a km_core_cp
string containing the current cached or intermediate context. The
function must be used only for debug logging purposes -- the string is
not intended to parsed for other purposes and the format may change in
the future.

This commit adds the function and corresponding unit tests. Subsequent
commits will update existing context debug functions in various engines
to use this API endpoint instead.
@mcdurdin mcdurdin requested a review from rc-swag as a code owner January 15, 2024 00:24
@github-actions github-actions bot added core/ Keyman Core feat labels Jan 15, 2024
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Jan 15, 2024

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S30 milestone Jan 15, 2024
@mcdurdin
Copy link
Copy Markdown
Member Author

mcdurdin commented Jan 15, 2024

@ermshiperete I see an error with the libkeymancore.symbols file in the Debian Packaging which I don't really understand:

[linux/scripts] build.sh parameters: <--gha --bin-pkg /home/runner/work/keyman/keyman/artifacts/keyman-binarypkgs/libkeymancore_17.0.244-1~PR-10377-2003.1+jammy1_amd64.deb --git-sha verify --git-base c42c28796ccfca48d9027cdab850f0d413eea9b5 verify>
[linux/scripts] ## verify starting...
--- libkeymancore.symbols (libkeymancore_17.0.244_amd64)
+++ dpkg-gensymbolsjpPYYb	2024-01-15 01:38:22.123316666 +0000
@@ -1,5 +1,6 @@
 libkeymancore.so.1 libkeymancore #MINVER#
 * Build-Depends-Package: libkeymancore-dev
+ _ZTSSt18codecvt_utf8_utf16IDsLm1114111ELSt12codecvt_mode0EE@Base 17.0.244
  km_core_actions_dispose@Base 17.0.197
  km_core_context_append@Base 17.0.195
  km_core_context_clear@Base 17.0.195
[linux/scripts] ## verify failed
Error: Process completed with exit code 2.

https://github.com/keymanapp/keyman/actions/runs/7523138690/job/20476241685

Can you explain? Also, should I be adding the new API symbols to that file?

@ermshiperete
Copy link
Copy Markdown
Contributor

#10386 should fix the packaging failure.

km_core_context_length@Base 17.0.195
km_core_context_set@Base 17.0.195
km_core_context_shrink@Base 17.0.195
km_core_cp_dispose@Base 17.0.241
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should use the version number that is current when it gets merged otherwise the API check will flag it.

As of version 17, the cached context is an internal property of the
state, not exposed to the consumer of the API -- apart from the
Keyman Developer Keyboard Debugger. However, for other debug
purposes, it is helpful to be able to examine the cached context, so
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
purposes, it is helpful to be able to examine the cached context, so
purposes, it is helpful for keyboard authors to be able to examine the cached context, so

or maybe "users of the debugger"? trying to capture the nuance here, that this is exposed to third party users?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is for use by consumers of the API -- i.e. Keyman Engine. So not really useful to keyboard authors or users of the Keyman Developer debugger. It's useful for us.

buffer << " U+" << std::hex << std::setfill('0') << std::setw(4) << std::hex << cp->character;
} else {
// A marker
buffer << " M(" << cp->marker << ")";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this format used anywhere else? I have used \m(1) but only because it matches LDML. Are there prior debugging strings which look similar to these, out of curiosity.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, because we used to call markers 'deadkeys', so most other log statements have something like dk(1).

mcdurdin and others added 3 commits January 16, 2024 05:46
Note: resolved merge conflict with action_api.cpp by shifting new code
into state_context_api.cpp.
Co-authored-by: Steven R. Loomis <srl295@gmail.com>
@mcdurdin mcdurdin changed the title feat(core): adds km_core_state_context_debug feat(core): add km_core_state_context_debug API Jan 15, 2024
@mcdurdin mcdurdin changed the base branch from master to epic/core/9999-normalization January 15, 2024 23:23
@mcdurdin mcdurdin changed the title feat(core): add km_core_state_context_debug API feat(core): add km_core_state_context_debug API 🌱 Jan 15, 2024
@mcdurdin mcdurdin merged commit 707f711 into epic/core/9999-normalization Jan 16, 2024
@mcdurdin mcdurdin deleted the feat/core/10365-context-debug branch January 16, 2024 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(core): Add method to get context for debug purposes 🌱

3 participants