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 the params status flag equals ucrsUpdateReady before attempting to download the keyman setup file #13154

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Feb 6, 2025

In the case when a Keyboard was available for update but the keyman version wasn't the state machine correctly didn't try to install it. However the back ground download loop would try to download and empty Params.InstallURL. This commit now checks the Params.Status. This commit also removes the dead code for TDownloadUpdateParams record type. This code is left over from a progress ui callback. It was initially left for when UI callback maybe added back in. However, it is currently confusing deadcode so has been removed.

Fixes: #13153
Fixes: KEYMAN-WINDOWS-4W5

User Testing

TEST_IDLE_KMSHELL_UPDATEDS_KBDS_ONLY

To Test this PR we need it so there are no newer updates available. If there are let me know and
I will merge in the lastest master again.

  1. Download the Keyman attached to this PR
  2. Start Keyman.
  3. Open the Keyman Configuration dialog.
  4. Install the euro latin keyboard attached to this PR 3.0.0 sil_euro_latin.zip
  5. In Options tab under General, check "Automatically check for updates and download"
  6. Open Regedit WinR type regedit
  7. Go to the current user key update state found at (Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine).
  8. If required set the state back to usIdle
  9. Open Windows explorer at "C:\Users"yourusername"\AppData\Local\Keyman\UpdateCache" folder.
  10. Delete cache.json, any *.kmp and *.exe files.
  11. Open a command prompt (kbd>WinR type cmd)
  12. Change to the directory where kmshell is installed
    cd "c:\Program Files (x86)\Keyman\Keyman Desktop"
  13. Type kmshell.exe -buc
  14. In the Regedit window verify the update state has advanced to usUpdateAvailable then usDownloading and finally usWaitngRestart. Press F5 to refresh the view.

In the case when a Keyboard was available for update but the
keyman version wasn't the state machine correctly didn't try to install
it. However the back ground download loop would try to download and
empty Params.InstallURL. This commit now checks the Params.Status.
This commit also removes the dead code for TDownloadUpdateParams record
type. This code is left over from a progress ui callback. It was
initially left for when UI callback maybe added back in. However, it
is currently confusing deadcode so has been removed.
Fixes: #13153
@rc-swag rc-swag self-assigned this Feb 6, 2025
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Feb 6, 2025
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 6, 2025

User Test Results

Test specification and instructions

  • TEST_IDLE_KMSHELL_UPDATEDS_KBDS_ONLY (PASSED) (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the B18S1 milestone Feb 6, 2025
@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 Feb 6, 2025
…-keyman-file-status

# Keyman Conventional Commit suggestions:
#
# - Link to a Sentry issue with git trailer:
#     Fixes: _MODULE_-_ID_
# - Give credit to co-authors:
#     Co-authored-by: _Name_ <_email_>
# - Use imperative, present tense ('attach' not 'attaches', 'attached' etc)
# - Don't include a period at the end of the title
# - Always include a blank line before trailers
# - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes
@rc-swag rc-swag marked this pull request as ready for review February 6, 2025 23:55
@rc-swag rc-swag requested a review from ermshiperete as a code owner February 6, 2025 23:55
@rc-swag rc-swag changed the title fix(windows): check params status before download fix(windows): Check the params status flag before attempting to download the keyman setup file Feb 7, 2025
@rc-swag rc-swag changed the title fix(windows): Check the params status flag before attempting to download the keyman setup file fix(windows): check the params status flag before attempting to download the keyman setup file Feb 7, 2025
@rc-swag rc-swag changed the title fix(windows): check the params status flag before attempting to download the keyman setup file fix(windows): check the params status flag equals ucrsUpdateReady before attempting to download the keyman setup file Feb 7, 2025
@dinakaranr
Copy link

Test Results

I tested this issue with the attached Keyman"18.0.187-alpha-test-13154" build(07/02/2025) on Windows 10. Here I am sharing my observation.

  • TEST_IDLE_KMSHELL_UPDATEDS_KBDS_ONLY (Passed):
  1. Installed the Keyman-18.0.187-alpha-test-13154.exe build which is attached to this PR
  2. Start Keyman.
  3. Open the Keyman Configuration dialog.
  4. Install the Euro Latin keyboard attached (version 3.0.0)
  5. In the Options tab under General, check "Automatically check for updates and download"
  6. Open Regedit WinR type regedit
  7. Go to the current user key update state found at (Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine).
  8. If required set the state back to usIdle
  9. Open Windows Explorer at "C:\Users"yourusername"\AppData\Local\Keyman\UpdateCache" folder.
  10. Delete cache.json, any *.kmp and *.exe files.
  11. Open a command prompt (kbd>WinR type cmd)
  12. Change to the directory where kmshell is installed(cd "c:\Program Files (x86)\Keyman\Keyman Desktop")
  13. Type kmshell.exe -buc
  14. In the Regedit window. Press F5 to refresh the view.
  15. Verified that the "update state" is changing to usUpdateAvailable then usDownloading and finally usWaitngRestart.
    It works well.
    Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

bug(windows): Download Error reported to sentry incorrectly
3 participants