-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
chore(windows): remove legacy core and flag ⛏️ #8593
chore(windows): remove legacy core and flag ⛏️ #8593
Conversation
Removing the legacy code that was replaced by the core. Also removing the feature flag for core integeration.
User Test ResultsTest specification and instructions ✅ SUITE_BASELINE: Keyman for Windows Base Line Tests10 tests in 2 groups PASSED
✅ SUITE_CAPSLOCK:30 tests in 2 groups PASSED
✅ SUITE_TSF_APP:6 tests in 2 groups PASSED
🟥 SUITE_IMX_KEYBOARDS:
✅ SUITE_STORE_AND_CONTEXT:8 tests in 2 groups PASSED
Retesting Template
Test Artifacts
|
RefreshPreservedKeys(TRUE); | ||
return TRUE; | ||
// happy to use while(!done) pattern | ||
fail: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised there was previously a possible memory leak here in not deleting _td_>lpActiveKeyboard
I am using a goto here. If we don't like that I could maybe use while(!done) {} instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM although see my comment about removing Keyman_ForceKeyboard
and Keyman_StopForcingKeyboard
altogether 😁
SUITE_BASELINE: Keyman for Windows Base Line TestsGROUP_WIN10:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I wonder if there are other functions we can cleanup as a result of this (e.g. extended string management, etc). But those can come as separate work.
LoadDLLs(&_td->lpKeyboards[i]); | ||
|
||
LoadKeyboardOptions(&_td->lpKeyboards[i]); | ||
LoadKeyboardOptionsREGCore(&_td->lpKeyboards[i], _td->lpKeyboards[i].lpCoreKeyboardState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you didn't rename this LoadKeyboardOptionsREGCore
given you renamed LoadlpKeyboardCore
to LoadlpKeyboard
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The LoadlpKeyboardCore
was more about distinguishing it from the legacy equivalent function. The LoadKeyboardOptionsREGCore
is more about loading registry options to Core. However, it made me think and go over all the functions with Core. I will create PR to clean this up. #8703
@@ -385,9 +380,6 @@ void Globals::SetBaseKeyboardFlags(char *baseKeyboard, BOOL simulateAltGr, BOOL | |||
be changed until Keyman is restarted. | |||
*/ | |||
BOOL Globals::InitSettings() { | |||
/* Check for common core vs windows core */ | |||
f_CoreIntegration = Reg_GetDebugFlag(REGSZ_Flag_UseKeymanCore, TRUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant REGSZ_Flag_UseKeymanCore
should also be deleted from the relevant .h file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh missed that thanks
RefreshPreservedKeys(TRUE); | ||
return TRUE; | ||
// happy to use while(!done) pattern | ||
fail: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM although see my comment about removing Keyman_ForceKeyboard
and Keyman_StopForcingKeyboard
altogether 😁
_td->lpActiveKeyboard->IMDLLs = NULL; | ||
_td->lpActiveKeyboard->KeyboardOptions = NULL;*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get rid of this commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up PR, I think we should remove Keyman_ForceKeyboard
altogether. It's totally legacy now, as anyone wanting to use a keyboard directly should host Keyman Core (as Keyman Developer Debugger does). We only use it in some really old manual tests (I checked) -- which we can leave alone at present.
That will be MUCH tidier, because the whole messy ownership of _td->lpActiveKeyboard
problem goes away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Issue #8646
I think we can remove |
addins.cpp/addins.h should also be deleteable I think |
SUITE_CAPSLOCK:GROUP_WIN10:
|
@@ -252,13 +252,9 @@ LRESULT _kmnGetMessageProc(int nCode, WPARAM wParam, LPARAM lParam) | |||
Handle WM_UNICHAR messages for RichEdit control -- should we test RichEdit version? | |||
*/ | |||
|
|||
if(mp->message == WM_UNICHAR && Addin_ShouldProcessUnichar(mp->hwnd)) | |||
if(mp->message == WM_UNICHAR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcdurdin I blindly removed addins - I don't know what they did. Should WM_UNICHAR
be removed totally as it seems addins was the only function interested in the event. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think WM_UNICHAR
is very much dead at this point. But perhaps it should be removed entirely in a separate PR (and also remove Keyman_ForceKeyboard in a separate PR too) -- that way we have PR# to reference them for potential future questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR for removing WM_UNICHAR
#8677
SUITE_TSF_APP:GROUP_WIN10:
|
SUITE_IMX_KEYBOARDS:GROUP_WIN10:
|
SUITE_STORE_AND_CONTEXT:GROUP_WIN10:
|
SUITE_BASELINE: Keyman for Windows Base Line TestsGROUP_WIN11:
|
SUITE_CAPSLOCK:GROUP_WIN11:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-approving
// TODO: 5442 will remove these once windows core is deprecated | ||
BOOL StopOutput; | ||
int LoopTimes; | ||
// TODO: 5442 | ||
WORD vkey; // I934 | ||
WCHAR charCode; // I4582 | ||
BOOL windowunicode; // I4287 | ||
BOOL isDown; | ||
LPKEYBOARD lpkb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed now?
…e-functions chore(window): rename core functions ⛏️
…unichar chore(windows): remove WM_UNICHAR completely
chore(windows): remove addins ⛏️
…e/windows/8646/remove-force-keyboard
…ce-keyboard chore(windows): remove keyman_forcekeyboard
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
…legacy-keyman-core
The failed tests fail currently on Windows 11 with Keyman 15 and up. #8697 has been created to deal with this. Therefore can merge this PR. |
Changes in this pull request will be available for download in Keyman version 17.0.109-alpha |
Fixes: #5442
Removing the legacy code that was replaced by the core. Also removing the feature flag for core integration.
User Testing
This will require a full acceptance test just to insure coverage of all the functionality.
Keyman for Windows Acceptance Test Procedures
These test procedures are to be run before moving from alpha to beta, or beta to stable, or before PRs are merged into stable branches.
Results are annotated with results, use
>
at the start of a new line under the checkbox/Test to note the result.User Testing
Gather Assets for Testing
app/windows/src/test/manual-tests/caps-lock-stores
.Setup Steps
SUITE_BASELINE: Keyman for Windows Base Line Tests
Test cases
click to expand
TEST_INSTALL:
TEST_KEYBOARD_INSTALLATION_REMOTE:
TEST_INSTALL_PKG_DISK:
TEST_KEYBOARD_OUTPUT:
TEST_ON_SCREEN_KEYBOARD:
SUITE_CAPSLOCK:
Caps Lock
The test keyboard layouts are found in the keyman repo at
app/windows/src/test/manual-tests/caps-lock-stores
. There is a project file for the 3 keyboards used in this test. The project file can be used to build the keyboard packages, but you can conveniently use the.kmp
file zipped and included respectively below.The test cases below expect the usage of the
capslock.kmp.zip
keyboard. That keyboard outputs pass or fail if following the test cases.Prerequisites before each test
capslock.kmp
.Test cases
click to expand
TEST_CAPSLOCK-1: uppercase with virtual key
a
Expected result:
pass.
(with other keyboards uppercaseA
)TEST_CAPSLOCK-2: lowercase with virtual key
b
Shift
Expected result:
pass.
(with other keyboards lowercaseb
)TEST_CAPSLOCK-3: capslock ignored for numbers
3
Shift
Expected result:
pass.
(with other keyboards#
)TEST_CAPSLOCK-4: uppercase
c
Expected result:
pass.
(with other keyboards uppercaseC
)TEST_CAPSLOCK-5: lowercase
d
Shift
Expected result:
pass.
(with other keyboards lowercased
)CapsAlwaysOff
For these tests, use a keyboard with the
caps_always_off.kmp.zip
store set. We call this keyboard capsalwaysoff below.Any keyboard with that store set will work; if you don't have one at hand you can use the
caps_always_off.kmp
keyboard. The caps_always_off.kmp keyboard will prevent switching caps lock on. As a sanity check to verify that Keyman is actually active, pressing the keya
will outputncaps_little_a
, andShift+a
will outputncaps_shift_A
.Note: When testing in a virtual machine, use an on-screen keyboard (in VirtualBox: Input/Keyboard/Soft Keyboard) and observe the caps lock indicator of the on-screen keyboard. Using the hardware keyboard might show side effects with caps lock.
Prerequisites before each test
CapsAlwaysOff
store set, e.g.caps_always_off.kmp
.Test cases
click to expand
TEST_CAPSOFF-1: sanity check
a
Expected result:
ncaps_little_a
TEST_CAPSOFF-2: caps lock stays off
CapsLock
keya
Expected result:
ncaps_little_a
TEST_CAPSOFF-3: no caps lock while holding capslock key
CapsLock
keya
CapsLock
keyExpected result:
ncaps_little_a
TEST_CAPSOFF-4: no caps lock while holding capslock key
CapsLock
keyShift
keya
CapsLock
andShift
keysExpected result:
ncaps_shift_A
TEST_CAPSOFF-5: switching turns off caps lock
a
Expected result:
ncaps_little_a
SHIFT: CapsOnOnly/ShiftFreesCaps
For these tests, use a keyboard with the
CapsOnOnly
andShiftFreesCaps
stores set. We call this keyboard shift_frees_caps below.Any keyboard with these stores set will work; if you don't have one at hand you can use the
shift_frees_caps.kmp.zip
keyboard.The shift_frees_caps.kmp keyboard will enable caps lock by pressing the
CapsLock
key, and will turn capslock off by pressing theShift
key. The keyboard outputs pass or fail if following the test cases.Note: When testing in a virtual machine, use an on-screen keyboard (in VirtualBox: Input/Keyboard/Soft Keyboard) and observe the caps lock indicator of the on-screen keyboard. Using the hardware keyboard might show side effects with caps lock. Except for TEST_CAPSONLY-5 which can only be reliably tested on a hardware keyboard on host OS (not a VM). For windows 10 and windows 11 with a virtual box vm-onscreen keyboard, the following happens. The VM soft keyboard does NOT actually send the Shift Shift Key Stroke through but rather will change the keys pressed for example if an
a
is pressed the soft keyboard itself will change that key to aA
. This means we can't Test TEST_CAPONLY-5 on a soft keyboard.Prerequisites before each test
CapsOnOnly
andShiftFreesCaps
stores set, e.g.shift_frees_caps.kmp
.Test cases
click to expand
TEST_CAPSONLY-1: no caps
1
Expected result:
pass.
TEST_CAPSONLY-2: caps
CapsLock
2
Expected result:
pass.
TEST_CAPSONLY-3: caps doesn't toggle
CapsLock
CapsLock
6
Expected result:
pass.
TEST_CAPSONLY-4: shift turns off
CapsLock
Shift
3
Shift
Expected result:
pass.
TEST_CAPSONLY-5: shift by itself turns off
Be aware of limitations when testing this on virtual machines as noted above.
CapsLock
Shift
Expected result:
SUITE_TSF_APP:
Background: Some windows applications use the windows TSF these include: MS Word, Firefox, Fieldworks and the windows search field on the taskbar.
Setup Text Services Framework(TSF) testing with Keyman for Windows
Install Keyman Developer version 15 or greater.
Open the Keyman Developers Test page in Firefox. It must be
Firefox Browser
i. By default this is at
localhost:8008
so you could type this location in to the firefox address bar.ii. You can also access it by pressing the
Test package on web
button found in theCompile
tab for any keyboard package.On the
Keymand Developer Keyboard Test Page
use theKeyboard
dropdown to selectsystem keyboard
Use the Keyman for Windows icon located in the
![system_tray](https://user-images.githubusercontent.com/58423624/193028036-23fa2d10-8f9e-40ad-a3ba-9b68dda1532e.png)
System Tray
to select the desired Keyman for the Windows keyboard.Typing text into the input field will now use the TSF with the keyboard selected in step 4.
Test cases: double processing
click to expand
To run these tests the EuroLatin (SIL) Keyboard from here.
GROUP_WIN10
GROUP_WIN11
TEST_DOUBLE_PROCESSING_FIREFOX:
Setup Text Services Framework(TSF) testing with Keyman for Window
above selecting the EuroLatin Keyboard at step 4.a
b
c
d
Backspaceabc
TEST_DOUBLE_PROCESSING_SEARCHBAR:
a
b
c
d
Backspaceabc
TEST_DOUBLE_PROCESSING_NOTEPAD:
a
b
c
d
abcd
SUITE_IMX_KEYBOARDS
Test IMX keyboards still work after core integration. see tests suite on #5936 and also #6187
This is the keyboard to be tested. imsample.zip
If an IMTest keyboard is already installed you will need to uninstall it first, restart windows and then install the Imsample keyboard attached to this PR.
I have copied the tests from #5936 and made a few modifications.
The overall test we are aiming for is that in doing the tests below the keyboard does not cause a keyman or app crash.
Test cases
click to expand
IMSAMPLE_KEYBOARD_INLINE_MENU:
This keyboard uses the letters
aeom
to allow IMX input. This keyboard is a bit flakey being a demo and not polished.It also takes some setting up the following registry keys need to be added and set once installed.
The location of the registry keys are at.
Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine\Active Keyboards\imsample
and are of type REG_DWORD
The keys are
ShowIMWindow
andShowIMWindowAlways
they both need to be set to 0.In this mode IM window is not shown rather the app text is updated with a menu when either
aeom
is pressed.For example when type
he
the text will becomeh[1ɛ 2ɜ 3ə 4e 5ɘ]
then you type the number3
the text displayed should now behə
TEST_IMSAMPLE_INPUT:
IMTest
t
r
y
space
key thena
then type 1 to get the 1st optiontry æ
TEST_IMSAMPLE_INPUT_CONT:
... contd from above
###
optiontry æ###
TEST_IMSAMPLE_BACKSPACE:
Using Notepad or Libreoffice
f
e
[
]
should be deleted.f
IMSAMPLE_KEYBOARD_IM_WINDOW:
This keyboard uses the letters
![image](https://user-images.githubusercontent.com/58423624/157192172-2ba2d0e4-8917-49b3-9709-00d7e9014645.png)
aeom
to allow IMX input. This time a IM window should display.The first time you use the keyboard with an application the registry keys will be created and by default set to 1.
If the IM window is not appearing you will need to verify the value of two registry keys.
The location of the registry keys are at.
Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine\Active Keyboards\imsample
and are of type REG_DWORD
The keys are
ShowIMWindow
andShowIMWindowAlways
they both need to be set to 1.Note for the IM window you need to use the mouse pointer to select the options, you can't type the number.
TEST_IMSAMPLE_INPUT_IMW:
IMTest
t
r
y
space
key thena
then click the 1st optiontry æ
TEST_IMSAMPLE_INPUT_IMW_CONT:
... contd from above
###
optiontry æ###
SIMPLIFIED_CHINESE:
This keyboard will display the IMX window as soon as a string the matches the pinyin for one or more characters are typed.
For all these tests install the Simplified Chinese Keyboard cs-pinyin.cmp found here
TEST_SIMPLIFIED_CHINESE_SINGLE:
Using Notepad or equivalent.
o
- The IMX window should appearTEST_SIMPLIFIED_CHINESE_MULTIPLE:
Using Notepad or equivalent.
h
a
n
z
i
- The IMX window will appear and in the top left the lettershanzi
will be presentTEST_SIMPLIFIED_CHINESE_BACKSPACE_1:
Using Notepad or equivalent.
This is to test the backspace occurring mid pinyin sequence before a character is output to the app
h
a
n
z
i
- The IMX window will appear and in the top left the lettershanzi
will be presenthan
.TEST_SIMPLIFIED_CHINESE_BACKSPACE_2:
Using Notepad or equivalent.
This is to test the backspace of already output characters.
h
a
n
z
i
- The IMX window will appear and in the top left the lettershanzi
will be present.SUITE_STORE_AND_CONTEXT
Test the options keyboard stores and context updating as a result of keystroke input. Introduced after #7077 and #7809 there are more tests on those PRs.
Test cases
click to expand
The key referred to as "enter" is alternatively labelled "return" on some keyboards.
Keyboards for the following test are found in the zip file store_context_kbd.zip
TEST_21_OPTION_STORE:
Expected Result: "no foo.foo.no foo"
TEST_OUTPUT_KEYSTROKE
Expected Result: "abd3"
TEST_OUTPUT_KEYSTROKE_INVALID_CONTEXT
Expected Result:
"abc
3"
TEST_DEADKEY_AND_CONTEXT
Expected Result: "correct"