Conversation
When waiting restart on a handlekmshell event and returning to idle kmshell execution should continue
It became apparent that we need to abort the "waitingtorestart" state before promting the user if they want to install. Otherwise it is confusing as to why nothing happens and Keyman just starts. In doing that it was easier to refactor ReadyToInstall to validate the cache, and check that automatic updates was still set. It can now also trigger an abort clearing the cache and returning to idle. This has slight flow on effect in that the HandleKMShell event can still be called following in the exectution flow.
Test ResultsI tested this PR in the "Keyman-19.0.111-alpha-test-14644" build on Windows 10. Here I am sharing my observation.
|
mcdurdin
reviewed
Sep 8, 2025
Comment on lines
+573
to
+580
| // Validate the cache otherwise we could | ||
| // prompt the caller for update but then fail. | ||
| // Also combined #14295 moved check here from initprog.pas | ||
| // The setting for automatic updates may have been set to disable | ||
| // after updates were checked and downloaded. The user should | ||
| // not be prompted for an update in this case and the update should | ||
| // abort. | ||
| if not (TUpdateCheckStorage.CheckMetaDataForUpdate and Self.GetAutomaticUpdates) then |
Member
There was a problem hiding this comment.
I'm finding this a bit hard to parse. Can you review? If possible, it may be easier to avoid the not test?
Contributor
Author
There was a problem hiding this comment.
Ok that comment didn't help, I think that was cause for confusion? I reworked it and function block comment.
If I remove the not it will make need to be nested which is even harder to parse.
if TUpdateCheckStorage.CheckMetaDataForUpdate and GetAutomaticUpdates then
begin
if not HasKeymanRun then
Exit(True);
Exit(False);
end
else
begin
RemoveCachedFiles;
CurrentState(IdleState);
Exit(False);
end;
mcdurdin
approved these changes
Sep 9, 2025
windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas
Outdated
Show resolved
Hide resolved
Co-authored-by: Marc Durdin <marc@durdin.net>
Collaborator
|
Changes in this pull request will be available for download in Keyman version 19.0.116-alpha |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When waiting restart on a handlekmshell event and returning to idle kmshell execution should continue
This also refacted the change in #14295, check if automatic updates have been turned off before pompting user. Therefore will have tests for that also.
Fixes: #14578
User Testing
TEST_KMSHELL_CONTINUE_EXECUTION
We need to test that if a handle kmshell event occurs when in the WaitingRestart state, but the cache is now corrupt that when returning to idle, kmshell execution continues.
Install the PR version of Keyman on this PR
Win + Rtyperegedit. GoComputer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engineand change theupdate statetousWaitingRestartC:\Users\{username}\AppData\Local\Keyman\UpdateCache.. If there is a cache.json file remove it. If there are other files they can stay there.Start with Windowsit will make it easier to verify the change.update statein the registry returns toidleWe need to verify that changes from #14295 still work
TEST_UPDATE_CANCELED_NO_PROMPT
Install the Keyman for Windows Build associated with this PR.
usWaitingRestartgo straight to step 8. Which should be go toOptionsand uncheckAutomatically check for updates and downloads].Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman EngineusWaitingRestartafter going through the other states ->usUpdateAvailable->usDownloadloading->usWaitingRestartOptionsand uncheckAutomatically check for updates and downloads.Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engineupdate stateflag returns tousIdleTEST_UPDATE_PROMPT
This is a regression to test to ensure the "normal" case still works.
Optionsand checkAutomatically check for updates and downloads.usWaitingRestartgo straight to step 8Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman EngineusWaitingRestartafter going through the other states ->usUpdateAvailable->usDownloadloading->usWaitingRestart