feat(windows): preserving keyboard options in engine by rc-swag · Pull Request #5668 · keymanapp/keyman

rc-swag · 2021-09-03T03:59:40Z

Fixes #5653
Three functions are added SaveKeyboardOptionsCore UpdateKeyboardOptionsCore and RestoreKeyboardOptionsCore.
To facilitate in saving the keyboard processors keyboard options and then also restoring them.
Unit tests have also been added to test the added functions.

This functions was to update the keyboard options in the windows engine with the current options in the core However it is not needed as first thought. This is because the options will not be updated in first Non updatable call kmtip. This is due to the fact the actions will not be processed. A variation on the current core implementation. I am still pushing the branch as the unit test will be usefull.

Add save and restore methods for saving and restoring keyboard processor options.

keymanapp-test-bot · 2021-09-03T03:59:42Z

User Test Results

✅ TEST_SETOPTION (PASSED): the keyboard does alternately output "t2" and "t1" on each key a press.

rc-swag · 2021-09-07T04:18:46Z

User Testing

The test cases below uses the options_set.kmp keyboard. That keyboard outputs a toggled string to easily identify if the toggle has occurred twice for a single key press. The tester can use other keyboards where they are aware the "set
the option is used and verify the output is as expected.

Install options_set.kmp
Test cases

TEST_SETOPTION: Toggle option on K_A

press and release a
Expected result:
t2
press and release a
t1
... For each press and release of a
expect a toggle between t1 and t2
t2t1t2t1t2t1....

windows/src/engine/keyman32/appint/aiTIP.cpp

MakaraSok · 2021-09-07T08:43:44Z

TEST_SETOPTION: BLOCKED The keyboard doesn't show up on the keyboard menu, hence the test cannot be performed. Note that the build used is from this PR 15.0.96-alpha-test-5668

mcdurdin

I think there is some memory leaking happening with keyboard option keys and values

windows/src/engine/keyman32/keyboardoptions.cpp

windows/src/engine/keyman32/appint/aiTIP.cpp

Co-authored-by: Marc Durdin <marc@durdin.net>

mcdurdin

This is looking good despite all my comments ;-)

mcdurdin · 2021-09-22T03:46:56Z

windows/src/engine/keyman32/appint/aiTIP.cpp

+        DisposeKeyboardOptionsCore(&SavedKBDOptions);
+        SavedKBDOptions = NULL;


Do you want to move the second line of this into DisposeKeyboardOptionsCore, given we are using that pattern elsewhere?

windows/src/engine/keyman32/keyboardoptions.cpp

windows/src/engine/keyman32/keyboardoptions.h

windows/src/engine/keyman32/keyboardoptions.cpp

windows/src/engine/keyman32/keyboardoptions.h

mcdurdin · 2021-09-22T03:53:16Z

windows/src/engine/keyman32/tests/keyman-engine-tests/pch.cpp

-// Include the standard header and generate the precompiled header.
-//
-
+/**


All this really shouldn't be in pch.cpp, which should only be a stub for precompiled headers.

All this really shouldn't be in pch.cpp, which should only be a stub for precompiled headers.

Ok I will move it all out to gtest_main.cpp as that makes it clear what it really is after all. I could go with gtest_mem as the original authour did but gtest_main is even clearer.
I will revert pch.cpp to just being a stub.

rc-swag · 2021-09-22T04:10:05Z

TEST_SETOPTION: BLOCKED The keyboard doesn't show up on the keyboard menu, hence the test cannot be performed. Note that the build used is from this PR [15.0.96-alpha-test-5668]

I have probably have set this keyboard up incorrectly. It wants English Australia (en-AU). However, it doesn't get you to install it. On my system it is available.

@MakaraSok If under the configuration of this Options Set keyboard layout you select the Add language button and choose English United States ( which you already have installed ) does it then show up on the keyboard menu?

I will make it build it again also this time with US because the test is so basic it doesn't need en-AU.

Co-authored-by: Marc Durdin <marc@durdin.net>

mcdurdin · 2021-09-22T06:35:58Z

windows/src/engine/keyman32/tests/keyman-engine-tests/gtest_main.cpp

+ * @copyright (c) 2013 Stephan Brenner
+ * @license   This project is released under the MIT License.
+ *
+ * Adapted in 2021 from the gtest_mem.cpp


Let's add a link to the original repo

mcdurdin

LGTM pending user testing

darcywong00 · 2021-09-23T02:43:13Z

@mcdurdin - What are your thoughts of checking in the options_set.kmp test keyboard to windows/src/test/manual-tests/ ?

mcdurdin · 2021-09-23T03:38:48Z

@darcywong00, at this stage, no need -- it's attached to this PR.

rc-swag · 2021-09-24T00:55:05Z

@keymanapp-test-bot retest

I have changed the Options Set test keyboard to use "en-US"

MakaraSok · 2021-09-27T09:37:32Z

TEST_SETOPTION: PASSED the keyboard does alternately output "t2" and "t1" on each key a press.

…ws/preserving-keyboard-options-in-engine

rc-swag added 2 commits September 2, 2021 11:57

feat(windows): add save and restore options

8719a27

Add save and restore methods for saving and restoring keyboard processor options.

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Sep 3, 2021

github-actions bot added common/ common/core/ core/ Keyman Core developer/ windows/ windows/engine/ labels Sep 3, 2021

rc-swag changed the base branch from master to feat/windows/keyman-core-integration-2 September 3, 2021 04:00

feat(windows): formating of km_kbp_options_list_size

f56c183

mcdurdin changed the title ~~Feat/windows/preserving keyboard options in engine~~ feat(windows): preserving keyboard options in engine Sep 5, 2021

feat(windows): cast size_t to int for list size

a90a997

github-actions bot added the feat label Sep 7, 2021

keymanapp-test-bot bot added 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 Sep 7, 2021

rc-swag commented Sep 7, 2021

View reviewed changes

windows/src/engine/keyman32/appint/aiTIP.cpp Outdated Show resolved Hide resolved

mcdurdin requested changes Sep 10, 2021

View reviewed changes

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 13, 2021

rc-swag and others added 2 commits September 21, 2021 11:48

feat(windows): Apply suggestions from code review

00d4bcd

Co-authored-by: Marc Durdin <marc@durdin.net>

feat(windows): add memory leak tests - fix memory leaks

deb358d

mcdurdin requested changes Sep 22, 2021

View reviewed changes

rc-swag and others added 2 commits September 22, 2021 16:11

feat(windows): Apply suggestions from code review

64dbaad

Co-authored-by: Marc Durdin <marc@durdin.net>

feat(windows): Address review comments

1c405c5

mcdurdin reviewed Sep 22, 2021

View reviewed changes

mcdurdin approved these changes Sep 22, 2021

View reviewed changes

feat(windows): add link to mem leak github repo

71f97d4

keymanapp-test-bot bot added the user-test-required User tests have not been completed label Sep 24, 2021

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 27, 2021

Merge branch 'feat/windows/keyman-core-integration-2' into feat/windo…

8eeaef9

…ws/preserving-keyboard-options-in-engine

rc-swag merged commit 2cf1dcc into feat/windows/keyman-core-integration-2 Sep 27, 2021

rc-swag deleted the feat/windows/preserving-keyboard-options-in-engine branch September 27, 2021 11:22

mcdurdin added the has-user-test label Feb 12, 2022

rc-swag self-assigned this Jun 2, 2023

rc-swag added this to the A15S14 milestone Jun 2, 2023

		DisposeKeyboardOptionsCore(&SavedKBDOptions);
		SavedKBDOptions = NULL;

Uh oh!

Conversation

rc-swag commented Sep 3, 2021

Uh oh!

keymanapp-test-bot bot commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Uh oh!

rc-swag commented Sep 7, 2021

User Testing

Uh oh!

Uh oh!

MakaraSok commented Sep 7, 2021 • edited by mcdurdin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

mcdurdin Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

rc-swag Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcdurdin Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

rc-swag Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

rc-swag commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcdurdin Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

darcywong00 commented Sep 23, 2021

Uh oh!

mcdurdin commented Sep 23, 2021

Uh oh!

rc-swag commented Sep 24, 2021

Uh oh!

MakaraSok commented Sep 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

keymanapp-test-bot bot commented Sep 3, 2021 •

edited

Loading

MakaraSok commented Sep 7, 2021 •

edited by mcdurdin

Loading

rc-swag commented Sep 22, 2021 •

edited

Loading