Skip to content
3 changes: 2 additions & 1 deletion windows/src/desktop/kmshell/kmshell.dpr
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ uses
Keyman.System.UpdateStateMachine in 'main\Keyman.System.UpdateStateMachine.pas',
Keyman.System.DownloadUpdate in 'main\Keyman.System.DownloadUpdate.pas',
Keyman.System.ExecutionHistory in '..\..\..\..\common\windows\delphi\general\Keyman.System.ExecutionHistory.pas',
Keyman.Configuration.UI.UfrmStartInstall in 'main\Keyman.Configuration.UI.UfrmStartInstall.pas' {frmStartInstall};
Keyman.Configuration.UI.UfrmStartInstall in 'main\Keyman.Configuration.UI.UfrmStartInstall.pas' {frmStartInstall},
UtilNetworkConnection in 'util\UtilNetworkConnection.pas';

{$R VERSION.RES}
{$R manifest.res}
Expand Down
15 changes: 5 additions & 10 deletions windows/src/desktop/kmshell/kmshell.dproj
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@
<DCCReference Include="main\Keyman.Configuration.UI.UfrmStartInstall.pas">
<Form>frmStartInstall</Form>
</DCCReference>
<DCCReference Include="util\UtilNetworkConnection.pas"/>
<None Include="Profiling\AQtimeModule1.aqt"/>
<BuildConfiguration Include="Debug">
<Key>Cfg_2</Key>
Expand Down Expand Up @@ -418,27 +419,21 @@
<Platform value="Win64">False</Platform>
</Platforms>
<Deployment Version="3">
<DeployFile LocalName="bin\Win32\Debug\kmshell.rsm" Configuration="Debug" Class="DebugSymbols">
<Platform Name="Win32">
<RemoteName>kmshell.rsm</RemoteName>
<Overwrite>true</Overwrite>
</Platform>
</DeployFile>
<DeployFile LocalName="bin\Win32\Debug\kmshell.exe" Configuration="Debug" Class="ProjectOutput">
<Platform Name="Win32">
<RemoteName>kmshell.exe</RemoteName>
<Overwrite>true</Overwrite>
</Platform>
</DeployFile>
<DeployFile LocalName="bin\Win32\Debug\kmshell.exe" Configuration="Debug" Class="ProjectOutput">
<DeployFile LocalName="Profiling\AQtimeModule1.aqt" Configuration="Debug" Class="ProjectFile">
<Platform Name="Win32">
<RemoteName>kmshell.exe</RemoteName>
<RemoteDir>.\</RemoteDir>
<Overwrite>true</Overwrite>
</Platform>
</DeployFile>
<DeployFile LocalName="Profiling\AQtimeModule1.aqt" Configuration="Debug" Class="ProjectFile">
<DeployFile LocalName="bin\Win32\Debug\kmshell.rsm" Configuration="Debug" Class="DebugSymbols">
<Platform Name="Win32">
<RemoteDir>.\</RemoteDir>
<RemoteName>kmshell.rsm</RemoteName>
<Overwrite>true</Overwrite>
</Platform>
</DeployFile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ object frmStartInstall: TfrmStartInstall
object lblUpdateMessage: TLabel
Left = 64
Top = 89
Width = 313
Width = 303
Height = 34
Caption = 'An update to Keyman has been downloaded and is ready to install.'
Font.Charset = DEFAULT_CHARSET
Expand Down Expand Up @@ -146,6 +146,34 @@ object frmStartInstall: TfrmStartInstall
414B8B49C3A0A5C5A461D0D262FA3F82D7F60E256D51F10000000049454E44AE
426082}
end
object shpMeteredWarning: TShape
Left = 60
Top = 149
Width = 314
Height = 45
Brush.Color = 15132415
Pen.Color = clRed
Visible = False
end
object lblMeteredWarning: TLabel
Left = 64
Top = 150
Width = 303
Height = 43
AutoSize = False
Caption = 'Metered Warning'
Color = 15132415
Font.Charset = DEFAULT_CHARSET
Font.Color = clWindowText
Font.Height = -13
Font.Name = 'Segoe UI'
Font.Style = []
ParentColor = False
ParentFont = False
Transparent = False
Visible = False
WordWrap = True
end
object cmdInstall: TButton
Left = 229
Top = 200
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
{
Keyman is copyright (C) SIL Global. MIT License.

// TODO: #12887 Localise all the labels and captions.
}
unit Keyman.Configuration.UI.UfrmStartInstall;
interface
Expand All @@ -19,43 +17,90 @@ interface
Winapi.Messages,
Winapi.Windows,
UfrmKeymanBase,
UserMessages, Vcl.Imaging.pngimage;
UserMessages,
Vcl.Imaging.pngimage;

type
// The 4 valid installation form scenarios plus a None case for validation
TInstallCase = (
icNone, // Not a valid case, can be used as check before calling creating form
icRestartRequiredMetered,
icRestartRequiredNotMetered,
icReadyToInstallNotMetered, // Metered warning never needed if ReadyToInstall
icNoInstallMessageMetered

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.

I found this install case name confusing; is the following name accurate?

Suggested change
icNoInstallMessageMetered
icReadyToDownloadWarnMetered

I guess my confusion extends to the dialog box as well -- would it be possible for the dialog box to say something to clarify that a download is required, something like this?

The update has not yet been downloaded.
and
You're on a metered connection. Downloading now may incur data charges.

);

TfrmStartInstall = class(TfrmKeymanBase)
cmdInstall: TButton;
cmdLater: TButton;
lblUpdateMessage: TLabel;
imgKeymanLogo: TImage;
shpMeteredWarning: TShape;
lblMeteredWarning: TLabel;
procedure FormCreate(Sender: TObject);
private
FRestartRequired: Boolean;
FScenario: TInstallCase;
public
constructor Create(AOwner: TComponent; const RestartRequired: Boolean); reintroduce;
constructor Create(
AOwner: TComponent;
const AScenario: TInstallCase); reintroduce;
end;

implementation

uses
MessageIdentifiers,
MessageIdentifierConsts;

{$R *.dfm}

constructor TfrmStartInstall.Create(AOwner: TComponent; const RestartRequired: Boolean);
constructor TfrmStartInstall.Create(
AOwner: TComponent;
const AScenario: TInstallCase);
begin
Assert(AScenario <> icNone, 'Invalid install case');
FScenario := AScenario;
inherited Create(AOwner);
FRestartRequired := RestartRequired;
end;

procedure TfrmStartInstall.FormCreate(Sender: TObject);
begin
inherited;
cmdInstall.Caption := MsgFromId(S_Update_Now);
cmdLater.Caption := MsgFromId(S_Later);
if FRestartRequired then
lblUpdateMessage.Caption := MsgFromId(S_Update_Restart_Req)
else
lblUpdateMessage.Caption := MsgFromId(S_Ready_To_Install);

// Default UI configuration state - metered warnings hidden initially
lblUpdateMessage.Visible := True;
lblMeteredWarning.Visible := False;
shpMeteredWarning.Visible := False;

case FScenario of
icRestartRequiredMetered:
begin
lblUpdateMessage.Caption := MsgFromId(S_Update_Restart_Req);
lblMeteredWarning.Caption := MsgFromId(S_Metered_Warning);
lblMeteredWarning.Visible := True;
shpMeteredWarning.Visible := True;
end;

icRestartRequiredNotMetered:
begin
lblUpdateMessage.Caption := MsgFromId(S_Update_Restart_Req);
end;

icReadyToInstallNotMetered:
begin
lblUpdateMessage.Caption := MsgFromId(S_Ready_To_Install);
end;

icNoInstallMessageMetered:
begin
lblUpdateMessage.Visible := False;
lblMeteredWarning.Caption := MsgFromId(S_Metered_Warning);
lblMeteredWarning.Visible := True;
shpMeteredWarning.Visible := True;
end;
end;
end;

end.
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ interface
KeymanPaths,
Keyman.System.ExecutionHistory,
Keyman.System.UpdateCheckResponse,
utilkmshell;
utilkmshell,
UtilNetworkConnection;

type
EUpdateStateMachine = class(Exception);
Expand Down Expand Up @@ -719,7 +720,9 @@ procedure UpdateAvailableState.EnterState;
begin
// Enter UpdateAvailableState
bucStateContext.SetRegistryState(usUpdateAvailable);
if bucStateContext.FAutomaticUpdate then

if bucStateContext.FAutomaticUpdate and
not UtilNetworkConnection.IsBackgroundUpdateBlocked then
begin
StartDownloadProcess;
end;
Expand Down Expand Up @@ -751,7 +754,8 @@ procedure UpdateAvailableState.HandleCheck;

function UpdateAvailableState.HandleKmShell;
begin
if bucStateContext.FAutomaticUpdate then
if bucStateContext.FAutomaticUpdate and
not UtilNetworkConnection.IsBackgroundUpdateBlocked then
begin
// we will use a new kmshell process to enable
// the download as background process.
Expand All @@ -773,6 +777,8 @@ procedure UpdateAvailableState.HandleAbort;

procedure UpdateAvailableState.HandleInstallNow;
begin
// This is deliberate action therefore no
// need to check if background update is allowed.
bucStateContext.SetApplyNow(True);
// A new kmshell process will be used to download
StartDownloadProcess;
Expand Down
40 changes: 34 additions & 6 deletions windows/src/desktop/kmshell/main/UfrmMain.pas
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ implementation
utilexecute,
utilkmshell,
utilhttp,
UtilNetworkConnection,
utiluac,
utilxml,
KeymanPaths;
Expand Down Expand Up @@ -817,12 +818,34 @@ procedure TfrmMain.Update_ApplyNow;
ShellPath : string;
FResult, InstallNow: Boolean;
frmStartInstallNow: TfrmStartInstall;
IsMetered: Boolean;
EInstallScenario: TInstallCase;

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.

Suggested change
EInstallScenario: TInstallCase;
InstallCase: TInstallCase;

The prefix E is usually reserved for exceptions. And why not call it InstallCase to match the type name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Long story I initally called it InstallScenario, but then I found following that unwritten enum rule of the small letter preffix it became
isRestartRequiredMetered etc. Which when reading through the code again got confused with the actual word "is" so I changed the name as "ic" avoided the confusion. It is a shame because Scenario sounds better.

begin
InstallNow := True;
// Confirm User is ok that this will require a reset
if HasKeymanRun then
IsMetered := UtilNetworkConnection.IsMetered;

// If a restart is required (HasKeymanRun == True)
// OR it is a Metered connection warn the user and allow
// them to cancel their request to Install Now.
// Otherwise start installing with out pop-up warnings.
EInstallScenario := TInstallCase.icNone;
if HasKeymanRun and not IsMetered then
begin
frmStartInstallNow := TfrmStartInstall.Create(nil, true);
EInstallScenario := TInstallCase.icRestartRequiredNotMetered;
end
else if HasKeymanRun and IsMetered then
begin
EInstallScenario := TInstallCase.icRestartRequiredMetered;
end
else if (not HasKeymanRun) and IsMetered then
begin
EInstallScenario := TInstallCase.icNoInstallMessageMetered;
end;

// Render dialog if conditions require it
if EInstallScenario <> TInstallCase.icNone then
begin
frmStartInstallNow := TfrmStartInstall.Create(nil, EInstallScenario);
try
if frmStartInstallNow.ShowModal = mrOk then
InstallNow := True
Expand All @@ -833,18 +856,23 @@ procedure TfrmMain.Update_ApplyNow;
end;
end;

if InstallNow = True then
// Process installation execution execution path
if InstallNow then
begin
ShellPath := TKeymanPaths.KeymanDesktopInstallPath(TKeymanPaths.S_KMShell);
FResult := TUtilExecute.Shell(0, ShellPath, '', '-an');
if not FResult then
begin
TKeymanSentryClient.Client.MessageEvent(Sentry.Client.SENTRY_LEVEL_ERROR,
'TrmfMain: Shell Execute Update_ApplyNow Failed')
'TrmfMain: Shell Execute Update_ApplyNow Failed');
end
else
ModalResult := mrAbort;
begin
// If a splash screen is currently open when "Install Now" is executed,
// setting mrAbort ensures the splash screen is closed on the
// return of "Keyman Configuration".
ModalResult := mrAbort;
end;
end;
end;

Expand Down
3 changes: 2 additions & 1 deletion windows/src/desktop/kmshell/main/initprog.pas
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,8 @@ function ShouldSendToBUpdateSM(FSilent: Boolean; BUpdateSM: TUpdateStateMachine;
(FMode in [fmStart, fmSplash, fmMain, fmAbout,
fmHelp, fmShowHelp, fmSettings, fmBoot]) then
begin
frmStartInstall := TfrmStartInstall.Create(nil, false);
// We are ready to install Metered warning not needed even if on Metered connection
frmStartInstall := TfrmStartInstall.Create(nil, TInstallCase.icReadyToInstallNotMetered);
Comment on lines +680 to +681

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.

Suggested change
// We are ready to install Metered warning not needed even if on Metered connection
frmStartInstall := TfrmStartInstall.Create(nil, TInstallCase.icReadyToInstallNotMetered);
// We are ready to install Metered warning not needed even if on Metered connection
frmStartInstall := TfrmStartInstall.Create(nil, TInstallCase.icReadyToInstallNotMetered);

try
Result := frmStartInstall.ShowModal = mrOk;
finally
Expand Down
Loading