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

feat(windows): kmshell switch handling for the installing state #12956

Draft
wants to merge 1 commit into
base: feat/windows/test-online-update
Choose a base branch
from

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Jan 21, 2025

This improves the switch handling around the installing state. Previously, the SM did a no-op and let the commands go through. This now only sends the commands through to the SM when installing that it should receive. This then allows us to detect when we are in incorrect state of have Keyman installation but the state machine is still in the installing state.

Also handle the case where the SM seems to be stuck in the installing
state. MSI may have failed.
@rc-swag rc-swag self-assigned this Jan 21, 2025
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Jan 21, 2025
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jan 21, 2025

User Test Results

Test specification and instructions

ERROR: user tests have not yet been defined

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S20 milestone Jan 21, 2025
@rc-swag rc-swag requested a review from mcdurdin January 21, 2025 05:03
@rc-swag rc-swag changed the base branch from master to feat/windows/test-online-update January 21, 2025 05:36
UserCanceled := False;
if BUpdateSM.ReadyToInstall and
(not FSilent and (FMode in [fmStart, fmSplash, fmMain, fmAbout, fmHelp,
fmShowHelp, fmSettings])) then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marc I have added fmShowHelp, and fmSettings as gui's where we could notify the user we can pop keyman update is ready to install. select update, or close.

However, I am still not happy that we are missing the all-important case when Keyman starts with Windows which is simply kmshell.exe -s I propose we update this to have a switch -on-boot so kmshell.exe -s -on-boot then we could determine this is a case where the UI can be run. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@marc

This isn't me!

Copy link
Member

Choose a reason for hiding this comment

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

kmshell -s means 'silent' so that means no UI is intended to be shown there... We could change that to kmshell.exe -boot, but we'd have to fixup existing installations -- we'd need to make sure that that change was applied during the upgrade (this is quite doable, just more work and testing).

I think that'd be good because it'd be clearer than the current -s which could be used any time and is ambiguous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat user-test-missing User tests have not yet been defined for the PR windows/config/ windows/
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants