Skip to content

fix: remove saving and restoring context kbd options#7077

Merged
rc-swag merged 1 commit intomasterfrom
fix/7074/keyboard-options-not-set
Aug 22, 2022
Merged

fix: remove saving and restoring context kbd options#7077
rc-swag merged 1 commit intomasterfrom
fix/7074/keyboard-options-not-set

Conversation

@rc-swag
Copy link
Copy Markdown
Contributor

@rc-swag rc-swag commented Aug 17, 2022

Fixes: #7074

This PR removes the saving of context and keyboard options
on the non-updateable call to process hook when using the core processor.

The windows Text Input Processor calls twice for each keystroke the first call is not updatable. With the core integration we run
the processor only once - the actions and the context are available to be applied on the updatable call.
Saving and restoring the options meant that the stores values were overwritten and seeing as the process event call was not called again the stores changed value was lost.

@rc-swag
fix: remove saving and restoring context kbd options
f2fe3b1 
Remove the saving of context and keyboard options
on the non updateable call to process hook.

The windows Text Input Processor calls twice one call
is not updateable. With the core integeration we run
the processor only once, preserving the actions and
context to be applied on the updatable call.
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Aug 17, 2022
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Aug 17, 2022 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_21_OPTION (PASSED): Tested this as per the instructions and it is working as expected. (notes)
  • ✅ TEST_COMMUNITY_KBD (PASSED): Tested this as per the instructions and it is working as expected. (notes)
  • ✅ TEST_OUTPUT_KEYSTROKE (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.
  • ✅ TEST_IF_AND_CONTEXT (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.
  • ✅ TEST_DEADKEY_AND_CONTEXT (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.
  • ✅ TEST_DEADKEY_AND_CONTEXT_EX (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.

Test Artifacts

@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Aug 17, 2022 •
edited
Loading

User Testing

Install the keyman for windows build associated with this PR
TEST_21_OPTION:

  1. Install 021 - options.zip
  2. Open Notepad
  3. Select 021-options.kmx as the current keyboard.
  4. Type a1a0a

Expected Result: "no foo.foo.no foo"

TEST_COMMUNITY_KBD:

  1. Install test.kmx.zip
  2. Open Notepad
  3. Select test.kmx as the current keyboard.
  4. Type aaa==aaa==aaa==aaa

Expected Result: "aaaAAAaaaAAA"
Keyboards for the following test are found in the zip file 7077_test_keyboards.zip
TEST_OUTPUT_KEYSTROKE

  1. Install 043 - output and keystroke.kmx
  2. Open Notepad
  3. Select 043 - output and keystroke.kmx as the current keyboard.
  4. Type 123
    Expected Result: "abd3"

TEST_IF_AND_CONTEXT

  1. Install 044 - if and context.kmx
  2. Open Notepad
  3. Select 044 - if and context.kmx as the current keyboard.
  4. Type ab
    Expected Result: "ex"

TEST_DEADKEY_AND_CONTEXT

  1. Install 045 - deadkey and context.kmx
  2. Open Notepad
  3. Select 045 - deadkey and context.kmx as the current keyboard.
  4. Type yzShift+/
    Expected Result: "correct"

TEST_DEADKEY_AND_CONTEXT_EX

  1. Install 046 - deadkey and contextex.kmx
  2. Open Notepad
  3. Select 046 - deadkey and contextex.kmx as the current keyboard.
  4. Type yShift+mShift+/
    Expected Result: "correct"

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Aug 17, 2022
@rc-swag rc-swag marked this pull request as ready for review August 17, 2022 03:47
@rc-swag rc-swag requested a review from sgschantz as a code owner August 17, 2022 03:47
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Aug 17, 2022

Looks good, I love the simplification of the code! I would like to see some user tests about context checks given this also changes the way we handle context.

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Aug 17, 2022

Once we approve and merge this, we need a cherry-pick to stable-15.0

@mcdurdin mcdurdin self-requested a review August 17, 2022 04:39
mcdurdin
mcdurdin approved these changes Aug 17, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Approving this, but please do add the extra user test for context changes!

@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Aug 17, 2022

extra user test for context changes!

Absolutely see this - issue #7078. I went back to versions where process_event at the core level was called both times [!Updateable/Updateable] and it still had this issue.
Feel free to comment on some context test cases I may miss.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Aug 17, 2022

  • TEST_21_OPTION (PASSED): Tested this as per the instructions and it is working as expected.

  • TEST_COMMUNITY_KBD (PASSED): Tested this as per the instructions and it is working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 17, 2022
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Aug 17, 2022

Feel free to comment on some context test cases I may miss.

I think we need some more user tests in this PR for context changes, if that's ok.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Aug 18, 2022
@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Aug 18, 2022

  • TEST_OUTPUT_KEYSTROKE (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.
  • TEST_IF_AND_CONTEXT (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.
  • TEST_DEADKEY_AND_CONTEXT (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.
  • TEST_DEADKEY_AND_CONTEXT_EX (PASSED): Downloaded the Corresponding Keyboard and tested as per the instructions. It is working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 18, 2022
@mcdurdin mcdurdin added this to the A16S9 milestone Aug 19, 2022
@rc-swag rc-swag merged commit 73da0d8 into master Aug 22, 2022
@rc-swag rc-swag deleted the fix/7074/keyboard-options-not-set branch August 22, 2022 01:36
@keyman-server
Copy link
Copy Markdown
Collaborator

keyman-server commented Aug 22, 2022

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

@rc-swag rc-swag mentioned this pull request Aug 24, 2022
Merged
@MakaraSok MakaraSok mentioned this pull request Aug 29, 2022
Closed
4 tasks
@rc-swag rc-swag mentioned this pull request Apr 19, 2023
Merged
3 tasks
@rc-swag rc-swag mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@sgschantz sgschantz Awaiting requested review from sgschantz

Assignees

No one assigned

Labels

fix has-user-test windows/engine/ windows/

Projects

None yet

Milestone

A16S9

Development

Successfully merging this pull request may close these issues.

bug(windows): keyboard options are not being set correctly in Keyman 15 (regression)

4 participants

@rc-swag @mcdurdin @bharanidharanj @keyman-server