feat(windows): honour Windows "metered connection" flag for downloading updates#16099
feat(windows): honour Windows "metered connection" flag for downloading updates#16099rc-swag wants to merge 9 commits into
Conversation
This adds a helper unit which uses the windows API to obtain a ConnectionProfile. This profile provides information about the connection status and connectivity statistics. This includes the connection costs, roaming, restricted etc. It also can check to see if background networking activity has been restricted. Using the recommened example this commit combines these to determine if a network is metered. It also provides access to the background network being restricted. Fixes: #13566
User Test ResultsTest specification and instructions
Results TemplateTest Artifacts |
If the download has already occured before calling the pop up then there is no reason to warn about the metered connection.
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
| if HasKeymanRun OR IsMetered then | ||
| begin | ||
| frmStartInstallNow := TfrmStartInstall.Create(nil, true); | ||
| frmStartInstallNow := TfrmStartInstall.Create(nil, HasKeymanRun, IsMetered); |
There was a problem hiding this comment.
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.
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
Keyman currenlty only supports Windows 10 and 11 so the exception was low risk. If it is not available we return false as metered connections werent possilble before this the functionality will be the consistent with those windows versions. i.e. always updating in the background
|
@Meng-Heng We need to test some scenarios again after my changes thanks. |
mcdurdin
left a comment
There was a problem hiding this comment.
This change looks really solid.
One thing: I am not that comfortable with the UI of the dialog -- the red background is not consistent with styling of the rest of the app and I think we should use a standard Warning icon instead. Happy for that to be a follow-up PR.
There was a problem hiding this comment.
Can we use the modern file naming scheme:
Keyman.Configuration.Util.NetworkConnection.pas?
| 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; |
There was a problem hiding this comment.
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;
| Profile: IConnectionProfile; | ||
| CostLevel: IConnectionCost; |
There was a problem hiding this comment.
Do we need to initialize COM before these functions are called? If so, we should call that out in the docs
| try | ||
| // Get the profile currently providing internet access | ||
| Profile := TNetworkInformation.GetInternetConnectionProfile; | ||
| if Profile <> nil then |
There was a problem hiding this comment.
| if Profile <> nil then | |
| if Assigned(Profile) then |
Normal Delphi pattern
| // If the WinRT network APIs are unavailable or throw (e.g. unusual | ||
| // Windows SKUs, containers, Network List Manager service stopped), | ||
| // treat the connection as non-metered so updates are not blocked. | ||
| Result := False; |
There was a problem hiding this comment.
Can we report errors on Sentry please? (Silent swallowing of the error is okay)
| except | ||
| on E: Exception do | ||
| // See IsMetered: default to not-restricted if the WinRT APIs throw. | ||
| Result := False; |
| FResult, InstallNow: Boolean; | ||
| frmStartInstallNow: TfrmStartInstall; | ||
| IsMetered: Boolean; | ||
| EInstallScenario: TInstallCase; |
There was a problem hiding this comment.
| EInstallScenario: TInstallCase; | |
| InstallCase: TInstallCase; |
The prefix E is usually reserved for exceptions. And why not call it InstallCase to match the type name?
There was a problem hiding this comment.
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.
| // We are ready to install Metered warning not needed even if on Metered connection | ||
| frmStartInstall := TfrmStartInstall.Create(nil, TInstallCase.icReadyToInstallNotMetered); |
There was a problem hiding this comment.
| // 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); |
| icRestartRequiredMetered, | ||
| icRestartRequiredNotMetered, | ||
| icReadyToInstallNotMetered, // Metered warning never needed if ReadyToInstall | ||
| icNoInstallMessageMetered |
There was a problem hiding this comment.
I found this install case name confusing; is the following name accurate?
| 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.
Noted with thanks, @rc-swag. I'll test this tomorrow. |

This adds a helper unit which uses the windows API to obtain a ConnectionProfile. This profile provides information about the connection status and connectivity statistics. This includes the connection costs, roaming, restricted etc. It also can check to see if background networking activity has been restricted.
Using the recommened example this commit combines these to determine if a network is metered. It also provides access to the background network being restricted.
There is a call that
IsBackgroundUpdateAllowedthat will return true if a background update is allowed.This is then use in the Update State Machine and also by update pop up.
UpdateStateMachine
Before downloading
IsBackgroundUpdateAllowedis calledUpdate Pop Up
This uses the
IsMeteredcall and adds a warning if to the user who is wanting to Install Now.This message would be better as a banner in the Update Configuration tab, rather than a delphi tab. This is the most efficent place to add the message for now.
Fixes: #13566
Build-bot: release:windows
User Testing
Mark connection as metered
For this we need to use windows setting to set the test machines network to metered. Right click the network connection icon in the Windows System tray and select
network & internet settingsSelected the connected work and the change the toggle forMetered connectiontoOnWindows 11

Windwos 10

TEST_RESTARTREQ_METERED
Use a keyboard update to test.
TEST_RESTARTREQ_NOT_METERED
This is to make sure a metered warning message is not present on non-metered connection.
TEST_NO_RESTARTREQ_METERED
This is to make sure a metered warning message is present but no other messages.
TEST_NO_READY_TO_INSTALL_NOT_METERED
This is to make sure a metered connection message is not shown when
we already have the download and are just in the WaitingRestart State.
TEST_NO_BACKGROUND_UPDATE
regeditupdate stateback tousIdlelast update check time. This needs to be done otherwise it will stay inusIdlefor 7 days.c:\Program Files (x86)\Keyman\Keyman Desktopkmshell.exe -cThis will run the configurationTEST_BACKGROUND_UPDATE
This is a regresion test.
OFFas explanined above.regeditupdate stateback tousIdlelast update check time. This needs to be done otherwise it will stay inusIdlefor 7 days.c:\Program Files (x86)\Keyman\Keyman Desktopkmshell.exe -cThis will run the configuration