Skip to content

fix(windows): add null pointer checks imsample keyboard#6187

Merged
rc-swag merged 3 commits intobetafrom
fix/windows/imsample-null-pointer-checks
Mar 10, 2022
Merged

fix(windows): add null pointer checks imsample keyboard#6187
rc-swag merged 3 commits intobetafrom
fix/windows/imsample-null-pointer-checks

Conversation

@rc-swag
Copy link
Copy Markdown
Contributor

@rc-swag rc-swag commented Feb 1, 2022

tl;dr add null ptr checks to pointers before access.

The tester discovered crashed when loading the imsample keyboard after installing it. I went through and added checks throughout the code. I built in and attached to the PR the tester was verifying. I am just now checking in this branch.

Note: I made the coding style the current Keyman style. Normally on an old project like this, I would be happy to make the new code match the current code. If that is what you prefer please comment and I will change it.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Feb 1, 2022
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Feb 1, 2022

User Test Results

Test specification and instructions

✅ SUITE_IMSAMPLE_KEYBOARD_INLINE_MENU:

3 tests in 1 groups PASSED
  • TEST_IMSAMPLE_INPUT (PASSED): The expected result is indeed try æ.
  • TEST_IMSAMPLE_INPUT_CONT (PASSED): The expected result is indeed try æ###.
  • TEST_IMSAMPLE_BACKSPACE (PASSED): The expected result is indeed f.

✅ SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW:

2 tests in 1 groups PASSED
  • TEST_IMSAMPLE_INPUT_IMW (PASSED): After setting the values to 0, I got the expected output.
  • TEST_IMSAMPLE_INPUT_IMW_CONT (PASSED): After setting the values to 1, I got the expected output. (notes)

Test Artifacts

@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Feb 1, 2022

We will test after all
x@ -keymanapp-test-bot skipx

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Feb 1, 2022
@mcdurdin mcdurdin added this to the B15S1 milestone Feb 1, 2022
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Feb 1, 2022

The tester discovered crashed when loading the imsample keyboard after installing it.

keymanapp-test-bot skip

Hmm, given this was discovered in testing, shouldn't we be testing the fix?

Comment on lines +3 to +5
Size=15
gridx=30
gridy=30
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these changes related to the fix?

Copy link
Copy Markdown
Contributor Author

@rc-swag rc-swag Feb 1, 2022

Choose a reason for hiding this comment

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

Yes. - this was one of the main causes for crashes. adding it though masked the crash so I removed it again and tried to fix all the crashes (null pointers or divide by zero errors) before putting it back. The main reason I didn't have the errors the tester had was because I had put this in directly on my local drive and had not built it as part of the keyboard package

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Feb 1, 2022

The tester discovered crashed when loading the imsample keyboard after installing it.
keymanapp-test-bot skip

Hmm, given this was discovered in testing, shouldn't we be testing the fix?

Happy to. As I said in this description " I built in and attached to the PR the tester was verifying. I am just now checking in this branch."
In other words the fixes were verified in the PR the tester was blocked on the imsample keyboard crashing, and not loading the IM window. They then tested the fixes in this branch as I built it and attached it to the PR.

However, it was a quick fix, it still has many improvements to be made to this sample. For example the IM window display does not fit the grid of characters well after the base character is selected. For example type a or click a the menu of characters to choose from doesn't always fit well.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Feb 1, 2022
@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Feb 2, 2022

User Testing

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.

SUITE_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 and ShowIMWindowAlways 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 become h[1ɛ 2ɜ 3ə 4e 5ɘ] then you type the number 3 the text displayed should now be

  • TEST_IMSAMPLE_INPUT:
  • Install Imsample keyboard attached to this PR.
  • Select the keyboard its name is IMTest
  • Type some keys that are not the special keys e.g. t r y
    • Type the space key then a then type 1 to get the 1st option
  • Expected Result: try æ
  • continue to the next test
  • TEST_IMSAMPLE_INPUT_CONT:
    ... contd from above
  • Type 'm' and type 3 to get ### option
  • Expected Result: try æ###
  • TEST_IMSAMPLE_BACKSPACE:
    Using Notepad or Libreoffice
  • Type f
  • Type e
  • The screen should have f[1ɛ 2ɜ 3ə 4e 5ɘ]
  • Now press Backspace. the whole option menu including the square brackets [ ] should be deleted.
  • Expected result f

SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW:

This keyboard uses the letters aeom to allow IMX input. This time a IM window should display.
image

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 and ShowIMWindowAlways 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:
  • Install Imsample keyboard attached to this PR (if not done already)
  • Select the keyboard its name is IMTest
  • Type some keys that are not the special keys e.g. t r y
  • Type the space key then a then click the 1st option
  • Expected Result: try æ
  • continue to the next test
  • TEST_IMSAMPLE_INPUT_IMW_CONT:
    ... contd from above
  • Type 'm' and select the 3rd ### option
  • Expected Result: try æ###

@keymanapp-test-bot 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 Feb 2, 2022
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.

LGTM

Comment on lines +3 to +5
Size=15
gridx=30
gridy=30
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@mcdurdin mcdurdin modified the milestones: B15S1, B15S2 Feb 6, 2022
@MakaraSok
Copy link
Copy Markdown

MakaraSok commented Feb 8, 2022

SUITE_IMSAMPLE_KEYBOARD_INLINE_MENU

  • TEST_IMSAMPLE_INPUT: PASSED The expected result is indeed try æ.
  • TEST_IMSAMPLE_INPUT_CONT: PASSED The expected result is indeed try æ###.
  • TEST_IMSAMPLE_BACKSPACE: PASSED The expected result is indeed f.

@MakaraSok
Copy link
Copy Markdown

SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW

  • TEST_IMSAMPLE_INPUT_IMW: FAILED Pressing 1 doesn't produce the expected output.

  • TEST_IMSAMPLE_INPUT_IMW_CONT: FAILED Pressing 1 doesn't produce the expected output.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 9, 2022
@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Feb 9, 2022

  • TEST_IMSAMPLE_INPUT_IMW_CONT: FAILED Pressing 1 doesn't produce the expected output.

@MakaraSok
When you say "pressing 1" are you clicking it or typing the number 1 on the keyboard. Because for the IM window typing the numbers will not work you need to select the option in the IM window by clicking on it.

@keymanapp-test-bot retest SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW all

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Feb 10, 2022
@MakaraSok
Copy link
Copy Markdown

MakaraSok commented Feb 10, 2022

  • TEST_IMSAMPLE_INPUT_IMW_CONT: FAILED Pressing 1 doesn't produce the expected output.

@MakaraSok When you say "pressing 1" are you clicking it or typing the number 1 on the keyboard. Because for the IM window typing the numbers will not work you need to select the option in the IM window by clicking on it.

@keymanapp-test-bot retest SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW all

There was no instruction on how to select, hence pressing the number was used to select from the option list.

Retested and it shows that the IME doesn't seem to be active. Nothing pops up to select from.

@bharanidharanj
Copy link
Copy Markdown

@rc-swag Tried to test SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW part and here is my observations:
1.Installed Keyman 15.0.189-alpha-test-6187 and the Keyman Developer as it attached in the Test Artifacts link.
2. Downloaded and installed imsample keyboard.
3. I opened Reg Editor. I did not find REG_DWORD in the following location
Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine\Active Keyboards\imsample

So, I got stuck on the problem.

I have attached the Screenshot for reference.

However, I tried to follow the steps as it mentioned in the TEST_IMSAMPLE_INPUT_IMW. When I typed some keys like t r y, nothing will show in the Notepad application.

I need your help to test this issue. Or Did I missed out anything?

@MakaraSok
Copy link
Copy Markdown

@bharanidharanj You are supposed to add the two keys to the location in RegEditor.

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 and ShowIMWindowAlways they both need to be set to 1.

@rc-swag I've tried this a few times and it seems like the IM is inactive. Pressing any of these aeom won't bring up anything.

@bharanidharanj
Copy link
Copy Markdown

@bharanidharanj You are supposed to add the two keys to the location in RegEditor.

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 and ShowIMWindowAlways they both need to be set to 1.

@rc-swag I've tried this a few times and it seems like the IM is inactive. Pressing any of these aeom won't bring up anything.

@MakaraSok Ross and I tried to add the two keys in the above mentioned location in RegEditor. After adding the two keys, we noticed that the IM is still inactive. Typing any letter on the the Notepad does not show up anything. Anyhow, Ross said that he will look in to this issue and will update it.

@bharanidharanj
Copy link
Copy Markdown

SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW:

  • TEST_IMSAMPLE_INPUT_IMW (FAILED): As per Ross's suggestion I have created two Keys ShowIMWindow and ShowIMWindowAlways set the values into 0. But still the the IM Sample Keyboard is not working.

  • TEST_IMSAMPLE_INPUT_IMW_CONT (FAILED): As per Ross's suggestion I have created two Keys ShowIMWindow and ShowIMWindowAlways set the values into 0. But still the the IM Sample Keyboard is not working.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 18, 2022
@mcdurdin mcdurdin modified the milestones: B15S2, B15S3 Feb 19, 2022
@rc-swag rc-swag marked this pull request as draft February 19, 2022 10:42
@mcdurdin mcdurdin modified the milestones: B15S3, B15S4 Mar 4, 2022
@rc-swag rc-swag force-pushed the fix/windows/imsample-null-pointer-checks branch from 397086e to 50f8c7b Compare March 8, 2022 03:54
@rc-swag rc-swag changed the base branch from master to beta March 8, 2022 03:56
This commit adds a function that sets the IM window registry keys
needed for the IMSample keyboard.

It also adds a null pointer check on the lpActiveKeyboard in the imx
callback module.
@rc-swag
Copy link
Copy Markdown
Contributor Author

rc-swag commented Mar 8, 2022

@bharanidharanj and @MakaraSok
I have made some changes to this keyboard so it creates the registry setting the first time the keyboard is used.
You will need to uninstall the old IMTest Keyboard. Restart Windows. Then install the keyboard attached to this PR in the Test Instructions. Test specification and instructions

@keymanapp-test-bot retest SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW all

@rc-swag rc-swag marked this pull request as ready for review March 8, 2022 08:20
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Mar 8, 2022
@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 15.0.207-beta

1 similar comment
@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 15.0.207-beta

@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 15.0.209-beta

@bharanidharanj
Copy link
Copy Markdown

@bharanidharanj and @MakaraSok I have made some changes to this keyboard so it creates the registry setting the first time the keyboard is used. You will need to uninstall the old IMTest Keyboard. Restart Windows. Then install the keyboard attached to this PR in the Test Instructions. Test specification and instructions

@keymanapp-test-bot retest SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW all

@rc-swag Okay, Thanks.

@bharanidharanj
Copy link
Copy Markdown

SUITE_IMSAMPLE_KEYBOARD_IM_WINDOW:

  • TEST_IMSAMPLE_INPUT_IMW (PASSED): After setting the values to 0, I got the expected output.
  • TEST_IMSAMPLE_INPUT_IMW_CONT (PASSED): After setting the values to 1, I got the expected output.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 10, 2022
@rc-swag rc-swag merged commit 514e585 into beta Mar 10, 2022
@rc-swag rc-swag deleted the fix/windows/imsample-null-pointer-checks branch March 10, 2022 09:15
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.

5 participants