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.

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

fix(windows): check IM window will be in a visible location #11967

Merged
merged 4 commits into from
Aug 27, 2024

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Jul 16, 2024

The IM display call back is used to show the IM window. With this change deadcode is removed and a check is made to ensure the IM window has completley visible coordinates for the monitor that has focus. If not and only if not it will be position close to where the caret in the application that has focus.

USER Testing

TEST_IMX_VISIBLE

To do this test, you need two monitors.

  1. Install the Keyman for Windows Build with this PR

  2. Install the Simplified Chinese cs_pinyin keyboard

  3. Open Notepad

  4. Type hanzi

  5. Ensure the IM pop-up appears and is visible

  6. In the Windows settings for monitors if it is not already set primary monitor to be on the right of the second monitor. In Windows 10 this is the setting
    image

  7. Go to Notepad and again type

  8. Insure the IMX is still visible

The IM display call back is used to show the IM window. With this change
deadcode is removed and a check is made to ensure the IM window is
completley visible coordinates for the monitor that has focus. If not
it will be position close to where the caret in the application that has
focus.
@rc-swag rc-swag added this to the A18S6 milestone Jul 16, 2024
@rc-swag rc-swag self-assigned this Jul 16, 2024
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Jul 16, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jul 16, 2024

@darcywong00 darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024
@rc-swag rc-swag marked this pull request as ready for review August 1, 2024 03:11
@rc-swag rc-swag requested a review from ermshiperete as a code owner August 1, 2024 03:11
windows/src/engine/keyman32/calldll.cpp Outdated Show resolved Hide resolved
windows/src/engine/keyman32/calldll.cpp Outdated Show resolved Hide resolved
windows/src/engine/keyman32/calldll.cpp Outdated Show resolved Hide resolved
windows/src/engine/keyman32/calldll.cpp Outdated Show resolved Hide resolved
@darcywong00 darcywong00 modified the milestones: A18S7, A18S8 Aug 2, 2024
@darcywong00 darcywong00 modified the milestones: A18S8, A18S9 Aug 17, 2024
@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 27, 2024
@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman 18.0.99-alpha-test-11967" build on the Windows 10 OS. I'm sharing my observation here.

  • TEST_IMX_VISIBLE (Passed):
  1. Installed the "Keyman-18.0.99.apk" file and gave all permissions to the application.
  2. Installed the "Simplified Chinese" keyboard.
  3. Open the notepad and then select the Chinese pinyin keyboard.
  4. Type the "h a n z i"
  5. Here, I verified that the popup appeared and displayed the suggestion of a word list.

I already had a two-monitor system hence, the display monitor was "extend these displays" mode.
I tried to type in the second monitor and It works well. Thank you.
Please refer to the screenshot below

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 27, 2024
@rc-swag rc-swag merged commit 72c894b into master Aug 27, 2024
6 checks passed
@rc-swag rc-swag deleted the fix/windows/kmdisplayim-validated-im-visibility branch August 27, 2024 23:21
@keyman-server
Copy link
Collaborator

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

@mcdurdin
Copy link
Member

Do we need to cherry-pick this to stable-17.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants