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.

Sign up for GitHub

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

Jump to bottom

fix(core): strip markers in actions_update_app_context_nfu() #10607

Merged
merged 6 commits into from
Feb 7, 2024
Merged

fix(core): strip markers in actions_update_app_context_nfu() #10607

merged 6 commits into from
Feb 7, 2024

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Feb 2, 2024 •
edited
Loading

Fixes #10605.

Includes unit test for marker stripping; unit tests for the module extensively refactored to ensure that we are testing the full context and not inadvertently missing markers.

User Testing

  • TEST_HIEROGLYPHIC_WINDOWS
    1. Install Hieroglyphic Keyboard from keyman.com into Keyman for Windows.
    2. Open Wordpad.
    3. Select the Hieroglyphic keyboard
    4. In wordpad, type: dispacebarspacebar
    5. Expected Output : 𓂞
    6. Type:backspace
    7. Expected Output: Empty

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Feb 2, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 2, 2024 •
edited
Loading

User Test Results

Test specification and instructions

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S31 milestone Feb 2, 2024
@github-actions github-actions bot added core/ Keyman Core fix labels Feb 2, 2024
@mcdurdin mcdurdin linked an issue Feb 2, 2024 that may be closed by this pull request
bug(core): actions_update_app_context_nfu needs to strip markers when updating app context #10605
Closed
@bharanidharanj
Copy link

bharanidharanj commented Feb 2, 2024 •
edited by mcdurdin
Loading

Test Results

  • TEST_HIEROGLYPHIC_WINDOWS (FAILED): Tested with the attached PR build (17.0.257-alpha-test-10607) on Windows 10 OS and here is my observation: 1. Installed Hieroglyphic keyboard from Keyman.com. 2. Opened WordPad. 3. Switched to Hieroglyphic keyboard. 4. In wordpad, typed "di". 5. Noticed that it is showing the wrong output. 6. Retested in Windows 11 OS (VM) and I am seeing the same behavior.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed user-test-required User tests have not been completed and removed user-test-required User tests have not been completed user-test-failed labels Feb 2, 2024
@mcdurdin
Merge branch 'fix/core/10582-km_core_state_get_actions-idempotency' i…
dbb24bc 
…nto fix/core/10605-app-context-copy-strip-markers
@mcdurdin
Copy link
Member Author

mcdurdin commented Feb 3, 2024

TEST_HIEROGLYPHIC_WINDOWS SKIP - now running this test in the follow-up PR #10618 as this PR still had other bugs impacting this test.

@mcdurdin mcdurdin modified the milestones: A17S31, B17S1 Feb 3, 2024
rc-swag
rc-swag approved these changes Feb 4, 2024
View reviewed changes
Copy link
Contributor

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

I was little confused with the test cases but the code change LGTM

core/tests/unit/kmnkbd/test_actions_normalize.cpp Show resolved Hide resolved
core/tests/unit/kmnkbd/test_actions_normalize.cpp Show resolved Hide resolved
@mcdurdin
Merge branch 'fix/core/10582-km_core_state_get_actions-idempotency' i…
49eb6f4 
…nto fix/core/10605-app-context-copy-strip-markers
@mcdurdin
Merge branch 'fix/core/10582-km_core_state_get_actions-idempotency' i…
5f4aeb6 
…nto fix/core/10605-app-context-copy-strip-markers
@srl295 srl295 self-requested a review February 6, 2024 17:42
srl295
srl295 approved these changes Feb 6, 2024
View reviewed changes
core/src/actions_normalize.cpp Outdated Show resolved Hide resolved
mcdurdin and others added 2 commits February 7, 2024 12:20
@mcdurdin @srl295
chore: Apply suggestions from code review
c807415 
Co-authored-by: Steven R. Loomis <srl295@gmail.com>
@mcdurdin
Merge branch 'fix/core/10582-km_core_state_get_actions-idempotency' i…
b512852 
…nto fix/core/10605-app-context-copy-strip-markers
Base automatically changed from fix/core/10582-km_core_state_get_actions-idempotency to master February 7, 2024 03:42
@mcdurdin mcdurdin merged commit bcc18bf into master Feb 7, 2024
17 checks passed
@mcdurdin mcdurdin deleted the fix/core/10605-app-context-copy-strip-markers branch February 7, 2024 03:43
@keyman-server
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rc-swag rc-swag rc-swag approved these changes

@srl295 srl295 srl295 approved these changes

@ermshiperete ermshiperete ermshiperete approved these changes

Assignees
No one assigned
Labels
core/ Keyman Core fix has-user-test
Projects
None yet
Milestone
B17S1
Development

Successfully merging this pull request may close these issues.

bug(core): actions_update_app_context_nfu needs to strip markers when updating app context
6 participants
@mcdurdin @bharanidharanj @keyman-server @ermshiperete @srl295 @rc-swag