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
Expand Up @@ -19,19 +19,28 @@ interface
Winapi.Messages,
Winapi.Windows,
UfrmKeymanBase,
UserMessages, Vcl.Imaging.pngimage;
UserMessages,
Vcl.Imaging.pngimage;

type
TfrmStartInstall = class(TfrmKeymanBase)
cmdInstall: TButton;
cmdLater: TButton;
lblUpdateMessage: TLabel;
imgKeymanLogo: TImage;
shpMeteredWarning: TShape;
lblMeteredWarning: TLabel;
procedure FormCreate(Sender: TObject);
private
FRestartRequired: Boolean;
FIsMetered: Boolean;
FReadyToInstall: Boolean;
public
constructor Create(AOwner: TComponent; const RestartRequired: Boolean); reintroduce;
constructor Create(
AOwner: TComponent;
const RestartRequired: Boolean;
const IsMetered: Boolean;
const ReadyToInstall: Boolean = False); reintroduce;
end;

implementation
Expand All @@ -41,10 +50,16 @@ implementation

{$R *.dfm}

constructor TfrmStartInstall.Create(AOwner: TComponent; const RestartRequired: Boolean);
constructor TfrmStartInstall.Create(
AOwner: TComponent;
const RestartRequired: Boolean;
const IsMetered: Boolean;
const ReadyToInstall: Boolean = False);
begin
inherited Create(AOwner);
FRestartRequired := RestartRequired;
FIsMetered := IsMetered;
FReadyToInstall := ReadyToInstall;
end;

procedure TfrmStartInstall.FormCreate(Sender: TObject);
Expand All @@ -56,6 +71,13 @@ procedure TfrmStartInstall.FormCreate(Sender: TObject);
lblUpdateMessage.Caption := MsgFromId(S_Update_Restart_Req)
else
lblUpdateMessage.Caption := MsgFromId(S_Ready_To_Install);

// Show warning if on a metered connection. If FReadyToInstall the update is
// already downloaded, so no use in displaying the warning.
lblMeteredWarning.Visible := FIsMetered and not FReadyToInstall;
shpMeteredWarning.Visible := FIsMetered and not FReadyToInstall;
if FIsMetered then
lblMeteredWarning.Caption := MsgFromId(S_Metered_Warning);
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
12 changes: 9 additions & 3 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,17 @@ procedure TfrmMain.Update_ApplyNow;
ShellPath : string;
FResult, InstallNow: Boolean;
frmStartInstallNow: TfrmStartInstall;
IsMetered: Boolean;
begin
InstallNow := True;
// Confirm User is ok that this will require a reset
if HasKeymanRun then
IsMetered := UtilNetworkConnection.IsMetered;
// If a restarted is required (HasKeymanRun == True)
Comment thread
rc-swag marked this conversation as resolved.
Outdated
// OR it is a Metered connection warn the user and allow
// them to cancel their request to Install Now.
// Otherwise start installing.
if HasKeymanRun OR IsMetered then
Comment thread
ermshiperete marked this conversation as resolved.
Outdated
begin
frmStartInstallNow := TfrmStartInstall.Create(nil, true);
frmStartInstallNow := TfrmStartInstall.Create(nil, HasKeymanRun, IsMetered);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From devin.ai:

Contradictory dialog messages when IsMetered=True and HasKeymanRun=False in Update_ApplyNow

In UfrmMain.pas:831, when HasKeymanRun is False but IsMetered is True, the form is created with RestartRequired=False and ReadyToInstall=False (default). This causes FormCreate at Keyman.Configuration.UI.UfrmStartInstall.pas:72-73 to show S_Ready_To_Install ("An update to Keyman has been downloaded and is ready to install") while simultaneously displaying the metered warning S_Metered_Warning ("You're on a metered connection. Downloading now may incur data charges") at line 77. These messages directly contradict each other: one says the update has already been downloaded, while the other warns about downloading costs. Before this PR, this code path didn't exist — the old code only showed the dialog when HasKeymanRun was true, always passing RestartRequired=true, so S_Ready_To_Install was never displayed here.

try
if frmStartInstallNow.ShowModal = mrOk then
InstallNow := True
Expand Down
5 changes: 4 additions & 1 deletion windows/src/desktop/kmshell/main/initprog.pas
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ implementation
UILanguages,
uninstall,
UpgradeMnemonicLayout,
UtilNetworkConnection,
utilfocusappwnd,
utilkmshell,
Keyman.System.UpdateStateMachine,
Expand Down Expand Up @@ -669,6 +670,7 @@ function ShouldSendToBUpdateSM(FSilent: Boolean; BUpdateSM: TUpdateStateMachine;
// UI elements from the state machine we have bring some of logic here.
var
frmStartInstall: TfrmStartInstall;
IsMetered: Boolean;
ValidateReadyToInstall: Boolean;
begin
ValidateReadyToInstall := BUpdateSM.ValidateReadyToInstall;
Expand All @@ -677,7 +679,8 @@ function ShouldSendToBUpdateSM(FSilent: Boolean; BUpdateSM: TUpdateStateMachine;
(FMode in [fmStart, fmSplash, fmMain, fmAbout,
fmHelp, fmShowHelp, fmSettings, fmBoot]) then
begin
frmStartInstall := TfrmStartInstall.Create(nil, false);
IsMetered := UtilNetworkConnection.IsMetered;
frmStartInstall := TfrmStartInstall.Create(nil, false, IsMetered, ValidateReadyToInstall);
try
Result := frmStartInstall.ShowModal = mrOk;
finally
Expand Down
94 changes: 94 additions & 0 deletions windows/src/desktop/kmshell/util/UtilNetworkConnection.pas

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.

Can we use the modern file naming scheme:

Keyman.Configuration.Util.NetworkConnection.pas?

Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
(*
* Keyman is copyright (C) SIL Global. MIT License.
*
* Notes: Enable checking for metered connection and background data restrictions.
*)
unit UtilNetworkConnection;

interface

(**
* Checks if the current internet connection is restricted, roaming, or over its
* data limit.
* This learn microsoft article shows how to combine network costs to determine
* if the connection is metered.
* https://learn.microsoft.com/en-us/uwp/api/windows.networking.connectivity.connectionprofile?view=winrt-28000
*
* @returns True if the connection is metered, False otherwise.
*)
function IsMetered: Boolean;

(**
* Checks if background data usage is explicitly restricted by the current network profile.
*
* @returns True if background data usage is restricted, False otherwise.
*)
function IsBackgroundDataRestricted: Boolean;

(**
* Determines whether background updates are blocked.
*
* @returns True if background updates are blocked, False if allowed.
*
* Note: Currently this checks for metered connection OR background
data usage restricted. If a configuration item is added that
provides the option to download on metered connections then
this should be updated to include that logic
*)
function IsBackgroundUpdateBlocked: Boolean;
Comment on lines +19 to +38

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.

These would be better in a class, because otherwise they land in the global namespace in Delphi, which is very cluttered already.

type
  TUtilNetworkConnection = class // or TNetworkConnection?
    class function IsMetered: Boolean; static;
    class function IsBackgroundDataRestricted: Boolean; static;
    class function IsBackgroundUpdateBlocked: Boolean; static;
  end;


implementation

uses
System.SysUtils,
Winapi.CommonTypes,
Winapi.WinRT,
Winapi.Networking.Connectivity;

function IsMetered: Boolean;
var
Profile: IConnectionProfile;
CostLevel: IConnectionCost;
Comment on lines +50 to +51

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.

Do we need to initialize COM before these functions are called? If so, we should call that out in the docs

begin
Result := False;
// Get the profile currently providing internet access
Profile := TNetworkInformation.GetInternetConnectionProfile;

if Profile <> nil then
begin
CostLevel := Profile.GetConnectionCost;
Result := (CostLevel.NetworkCostType <> NetworkCostType.Unrestricted)
or CostLevel.Roaming
or CostLevel.OverDataLimit;
end;
end;

function IsBackgroundDataRestricted: Boolean;
var
Profile: IConnectionProfile;
CostLevel: IConnectionCost;
DataRestriction: IConnectionCost2;
begin
Result := False;
Profile := TNetworkInformation.GetInternetConnectionProfile;
if Profile <> nil then
begin
CostLevel := Profile.GetConnectionCost;
if (CostLevel <> nil) and Supports(CostLevel, IConnectionCost2, DataRestriction) then
begin
Result := DataRestriction.BackgroundDataUsageRestricted;
Exit;
end;
end;
end;

// Currently this checks for metered connection OR background
// data usage restricted. If a configuration item is added that
// provides the option to download on metered connections then
// this should be updated to include that logic
function IsBackgroundUpdateBlocked: Boolean;
begin
Result := IsMetered OR IsBackgroundDataRestricted;
end;
Comment thread
ermshiperete marked this conversation as resolved.

end.
7 changes: 6 additions & 1 deletion windows/src/desktop/kmshell/xml/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -608,11 +608,16 @@
<!-- Introduced: 18.0.168 -->
<string name="S_Update_Restart_Req" comment="Update tab pop up - warns user restart will require restart ">Installing the update now will require a restart of your computer before you can use Keyman again. Update now anyway?</string>

<!-- Context: Configuration Dialog - Update on boot pop up -->
<!-- Context: Configuration Dialog - Update on pop up -->
<!-- String Type: PlainText -->
<!-- Introduced: 18.0.168 -->
<string name="S_Ready_To_Install" comment="Update on boot pop up - inform user update about to install">An update to Keyman has been downloaded and is ready to install.</string>

<!-- Context: Configuration Dialog - Update on pop up -->
<!-- String Type: PlainText -->
<!-- Introduced: 19.0.146 -->
<string name="S_Metered_Warning" comment="Update warning - inform user about being on a metered connection">You\'re on a metered connection. Downloading now may incur data charges.</string>

<!-- Context: Download Keyboard Dialog -->
<!-- String Type: PlainText -->
<!-- Introduced: 7.0.230.0 -->
Expand Down