When normalizing, we need to stop processing on an NFC boundary, not an
NFD boundary, to support normalizations such as in Bengali, where
appending `U+09D7` to a context of `U+0995 U+09C7` should result in
`U+0995 U+09CC`.
The specification is unclear on this; see https://unicode-org.atlassian.net/browse/CLDR-19218
This also updates the ldml keyboard unit test suite to support running
in full NFC mode (used in all Engine implementations) as well retaining
the NFD mode (now only used by the debugger).
Side note: the Bengali normalization failure case was picked up by the
improvements to the unit test suite, proving once again that good tests
are so valuable.
Fixes: #15491Fixes: #15505
Follows: #15488
Relates-to: CLDR-19218
Ensure that when a single backspace decomposes the last NFC character in
the app context, the remainder of the 'cluster' is preserved, matching
the implication of the CLDR keyboard specification.
This addresses the behavior in #15487 where the cached context became
out of sync with the app context after deleting an entire NFC cluster
such as ê, which caused a loop ending up with the entire context being
deleted, at which point the loop exited with a fail-safe.
Note that the LDML keyboard tests (ldml.cpp) do not currently exercise
the normalization code; this is a gap that should be addressed to ensure
that we are testing final application behavior.
Fixes: #15487
Had a real yak shave this morning with disabling assertions in release
builds in our C/C++ code. It turns out that our unit tests use
`assert()` which we intended to use from `test_assert.h`, but in some
cases `cassert` or `assert.h` had been #included after `test_assert.h`,
overriding our special `assert()` macro. The chain of includes is
somewhat hard to puzzle out -- it's often buried several levels deep.
This meant that a release build would drop all test assertions, meaning
most tests passed, unsurprisingly, as there were no assertions left to
fail ... but some tests failed with crashes because we optimized out
important lines such as `assert(some_important_function())`.
I was quite unhappy with this fragility, so I have opted to rename
`assert()` to `test_assert()` in all of our home-grown C/C++ unit tests,
which further highlighted unit tests which were only using the C/C++
`assert()` and not ours, so then had to figure out which unit test
executables needed to have `test_assert` added, and then ... then ...
discovered a bug in `test_color.h`, where we were #including
`io.h`/`unistd.h` inside a `namespace console_color {}` block, which
just happened to be the first ref to those beautiful headers, and thus
(because `#pragma once`) meant that useful little functions like
`access()` were no longer accessible to us in the global namespace.
I have also audited Every Single Call to `assert()` to verify that we do
not do Important Work inside the parentheses, and, apart from those
offending unit tests, now resolved with `test_assert()`, it looks like
all is good.
I would like to present one very well-shaved yak in this commit.
Fixes: #12619
- split keyboard loading into loading KMX file into blob and then
loading the keyboard processor from the blob.
- deprecate `km_core_keyboard_load`
- move file access next to deprecated method. This is now the only place
that loads a file in Core; unit tests have some more places that
load files.
- introduce GTest and add unit tests for loading from blob
Cherry-picked from `epic/web-core` branch.
Cherry-Pick-Commit: 1deaa323ad
Cherry-Pick-Commit: 59019cc8b7
Cherry-Pick-Commit: bc46458368
Cherry-Pick-Commit: d06aa29956
Cherry-Pick-Commit: 1c88166f6e
Cherry-Pick-Commit: 069cd21ecd
Cherry-Pick-Commit: 052ae2ec35
Cherry-Pick-Commit: 11a2a3ba3a
Part-of: #11293
Part-of: #8093
- add a normalize_nfd() which takes a single codepoint
- temporarily keep ICU in actions_normalize.cpp and ldml_transforms.cpp
- expand wasm opts in unit tests
Moves variable declaration for `bool enabled` from .h to .cpp and fixes
up builds. Fixes `#include` in ldml_test_source.cpp which was inside a
namespace.
Fixes: #10426
Fixes#10615 (surrogate handling, km_core_state_context_set_if_needed).
Fixes#10616 (markers in app context, in unit tests)
Fixes#10617 (memory leak in replace_context).
The surrogate handling and markers in app context bugs were quite
tangled, so opted to fix those both together in one commit. The memory
leak was spotted while verifying the changes for surrogate handling.
Surrogates were not handled in is_context_unchanged, and opted to
refactor to work directly with km_core_context_item arrays rather than
continue with the string-based comparison, for simplicity.
While writing the corresponding unit tests, realised that the
app_context checks were incorrect in a number of existing tests, and so
fixed up the existing tests at the same time as writing up the new
tests. The only changes to core for this were to add assertions for the
app_context to ensure that markers are not inadvertently added.
Fixes#10582.
Returned struct from the `km_core_state_get_actions()` API is now owned
by the state object. This is a change in API contract. The corresponding
`km_core_actions_dispose()` API has been made private, because there is
never any need for API consumers to call it. As this change is happening
in alpha, we won't bump the ABI version.
`state->app_context` is now guaranteed to be in sync with
`state->context` after `km_core_process_event()`, with the actions
struct populated at that point.
The next and final step in this refactor is to remove the actions queue
altogether from the Core and make that a kmn-only concept, but that will
wait until 18.0. The only consumers of the actions queue are the
kmn-specific interactive debugger in Keyman Developer, and kmn-specific
Input Method eXtensions in Keyman Engine for Windows.
Fixes#10530.
Adds the deleted_context member to the km_core_actions struct, and
associated unit tests. Simplifies integration by providing the consumer
with all the data they need in order to execute the transform,
specifically around number of delete operations required, without
needing to query the target application context again. The number of
delete operations will vary according to application compliance and
selected encoding; for example a UTF-16 string may require 2
delete-backs for surrogate pairs in the text buffer for a compliant app,
whereas there will be a single delete-back key event for a non-compliant
app.
The deleted_context member should also be used for debug assertions.
Relates to #10416.
Moves from using action queue to action struct for the LDML keyboard
debugger. Does not do the same for the KMX keyboard debugger, because
that relies on action items to do single-step debugging.
- fix backspace processing
- fix k_102 - we can no longer test for context invalidation here
- fix a test that assumed non-const km_core_actions.output
For: #10410
Fixes#10431.
The `km_core_` prefix has been removed from internal-only functions, and
these function declarations moved to context.hpp.
The functions have not been moved from km_core_context_api.cpp at this
stage.
Rewrote the function documentation in Javadoc style comments for the
internal use functions.
Per discussion in #10422, we can assume that input cached_context is
always NFD. However input actions->output may not start at a
normalization boundary, so we still need to backtrack to a normalization
boundary in order to get our NFC output. But cached_context never need
change.
This makes no change to the algorithm, but tweaks some of the unit tests
to adhere to this input assumption.
Note: we could consider adding a debug assertion that cached_context is
NFD.
Fixes#9999.
Note TODO items:
- [ ] Renormalize cached_context across action boundary. Blocked by #10369.
- [ ] Add extra tests for surrogate pairs
- [ ] Move set_context_from_string into helper module
- [ ] if we don't apply normalization, we still need to fixup the
app_context, to keep it coherent with cached_context (or at least
we need to verify that app_context is never used in this
situation)
Relates to #9999.
Fixes#10384.
The context API endpoints should no longer be considered as part of the
standard Core API. The only consumers that have a need to access these
APIs are the IMX integration in Engine for Windows, and the Keyman
Developer Debugger.
These symbols are currently used by Developer:
* `km_core_context` struct
* `km_core_context_type` enum
* `km_core_context_item` struct
* `KM_CORE_CONTEXT_ITEM_END` macro
* `km_core_state_context()`
* `km_core_context_set()`
* `km_core_context_clear()`
These symbols are currently used by Windows IMX:
* `km_core_context` struct
* `km_core_context_type` enum
* `km_core_context_item` struct
* `KM_CORE_CONTEXT_ITEM_END` macro
* `km_core_context_items_dispose()`
* `km_core_context_item_list_size()`
* `km_core_state_get_intermediate_context()`
The following functions and symbols are moving to
keyman_core_api_context.h:
* `km_core_context` struct
* `km_core_context_type` enum
* `km_core_context_item` struct
* `KM_CORE_CONTEXT_ITEM_END` macro
* `km_core_state_context()` function
* `km_core_state_get_intermediate_context()` function
* `km_core_context_set()` function
* `km_core_context_clear()` function
* `km_core_context_get()` function
* `km_core_context_items_from_utf16()` function
* `km_core_context_items_from_utf8()` function
* `km_core_context_items_to_utf8()` function
* `km_core_context_items_to_utf16()` function
* `km_core_context_items_to_utf32()` function
* `km_core_context_items_dispose()` function
* `km_core_context_length()` function
* `km_core_context_append()` function
* `km_core_context_shrink()` function
* `km_core_context_item_list_size()` function
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.
It was fairly surprising to have the unit tests for
`km_core_state_context_*` methods in `action_api.cpp`, so this change
moves them to a separate file.
Adds state->set_actions(). This sets the Core's action list to match the
contents of the action struct. Note that markers are not supported and
backspace expected_values will be empty, as this information is not
available. As the intended consumer of the action struct does not need
to know this information, this should be adequate.
In the case when the cached context had been cleared the
km_core_state_context_set_if_needed call would just compare
the null terminations of both strings and not set the cached
context to the application context.
