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): clear core context on invalidate cache #10230

Merged
merged 10 commits into from
Dec 19, 2023

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Dec 12, 2023

Fixes: #10182
The core context cache belonging to the state object is now cleared when the keyboard processor determines the context needs to be invalidated.

Test k_000___null_keyboard has been updated as the [RALT K_B] should
cause the context to be reset.

Test k_008___vkey_input__ctrl_alt_2_ has been updated as the [LCTRL LALT
K_B] does not match a keyboard rule the context is deemed invalid and should be
reset.

It could be a consideration to modify the text_store object to better
reflect the state of an application context however, this would mean
we would need two expected results one for the application and one
for the context.

@keymanapp-test-bot skip
Only skipping because the Windows and Linux builds can't be used to test as they currently still use the actions list and clear the core context cache. MacOS is probably is probably there?

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Dec 12, 2023
@rc-swag rc-swag self-assigned this Dec 12, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Dec 12, 2023

Test k_000___null_keyboard has been updated as the [RALT K_B] should
cause the context to be reset.

Test k_008___vkey_input__ctrl_alt_2_ has been updated as the [LCTRL LALT
K_B] does not match a keyboard rule the context is deemed invalid and should be
reset.

It could be a consideration to modify the text_store object to better
reflect the state of a applications context however this would mean
we would need two expected results one for the application and one
for the context
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Dec 14, 2023
@rc-swag rc-swag marked this pull request as ready for review December 14, 2023 02:44
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

I think the unit tests may need extra work for this to get to a resolution -- because they should be comparing the received output and not the cached context.

common/test/keyboards/baseline/k_000___null_keyboard.kmn Outdated Show resolved Hide resolved
core/src/kmx/kmx_processor.cpp Show resolved Hide resolved
@rc-swag rc-swag marked this pull request as draft December 14, 2023 06:54
The unit tests needed to be updated to handle the cases
where the context is invalidated by a keystroke. However, they
still need to be able to verify the final string the application
should recieve. To do this a expected context token has been added
to the unit test format.
@rc-swag
Copy link
Contributor Author

rc-swag commented Dec 15, 2023

The unit tests needed to be updated to handle the cases
where the context is invalidated by a keystroke. However, they
still need to be able to verify the final string the application
should receive. To do this an expected context token has been added
to the unit test format.

Question for @ermshiperete and @sgschantz I updated this so that the existing unit tests did not need the expect context field added. The majority of tests are going to see expected and expected_context be the same. If expected context is not supplied it defaults to be the same as expected.
The alternative is that every test will need to set expected and expected context. Thoughts?

@rc-swag rc-swag marked this pull request as ready for review December 15, 2023 01:27
@ermshiperete
Copy link
Contributor

I think making expected the default if expected context is not set makes sense.

core/tests/unit/kmx/kmx.cpp Outdated Show resolved Hide resolved
core/tests/unit/kmx/kmx.cpp Outdated Show resolved Hide resolved
core/tests/unit/kmx/kmx.cpp Outdated Show resolved Hide resolved
core/tests/unit/kmx/kmx.cpp Outdated Show resolved Hide resolved
sgschantz and others added 2 commits December 18, 2023 09:09
fix(mac): unit tests should expect core context clear after invalidate
@rc-swag rc-swag dismissed mcdurdin’s stale review December 19, 2023 05:24

I have addressed the review comments - reviewer is on leave and was happy for someone else to review in his absence

@rc-swag rc-swag merged commit 4246c7a into master Dec 19, 2023
17 checks passed
@rc-swag rc-swag deleted the fix/core/clear-core-context-invalidate branch December 19, 2023 05:25
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.233-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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