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

Disable GUI notification for newer version #2931

Merged
merged 6 commits into from Sep 6, 2021

Conversation

tarkah
Copy link
Contributor

@tarkah tarkah commented Aug 27, 2021

I'm packaging Mullvad for Solus OS and it'd be nice to disable the version check and GUI notification. Package updates are shipped to users every Friday, so the notice is really unnecessary, especially because they can't just "update" via a .deb or .rpm.

I've added a feature flag on the daemon, disable-version-check, which just replaces the app version request with an "empty" response that won't show as an update.

This seemed like the best place to disable this, as the VersionUpdater is pretty well tied into the daemon.

I've tested by patching this against 2021.3 and successfully did not see any version update notification. The output of mullvad version is:

Current version: 2021.3
	Is supported: false
	No newer version is available

Git checklist:


This change is Reviewable

@tarkah
Copy link
Contributor Author

tarkah commented Aug 27, 2021

I just noticed the IS_DEV_BUILD flag that "disables" the updater, so I changed this to use that same code path

@faern
Copy link
Member

faern commented Sep 1, 2021

Am I understanding you correctly that the annoying part with the current app is that users get update notifications in the GUI?

If so. I think this better be solved in the GUI only. Change the GUI so it ignores version update broadcasts in one way or another. Also, make it configurable at runtime rather than build time. Then you can easily set an environment variable in your GUI launch .desktop file or similar instead of having to touch the daemon code.

@tarkah
Copy link
Contributor Author

tarkah commented Sep 1, 2021

Yup, the ultimate goal is to not have a notification in the GUI. I believe the only place it shows via CLI is on version, which isn't a concern.

From a packaging perspective, the build option works great, is simple, and eliminates any network requests that won't be needed at runtime.

However, I agree a runtime option for disabling in GUI is probably the right way to think about this, especially as it enables anybody to easily disable the version check. I'll dig into the GUI code and take a second shot at this. Appreciate the feedback!

@tarkah
Copy link
Contributor Author

tarkah commented Sep 1, 2021

It appears this would be the correct place to return early, to avoid setting the upgrade version and notifying.

private setLatestVersion(latestVersionInfo: IAppVersionInfo) {
const suggestedIsBeta =
latestVersionInfo.suggestedUpgrade !== undefined &&
IS_BETA.test(latestVersionInfo.suggestedUpgrade);
const upgradeVersion = {
...latestVersionInfo,
suggestedIsBeta,
};
this.upgradeVersion = upgradeVersion;
// notify user to update the app if it became unsupported
const notificationProviders = [
new UnsupportedVersionNotificationProvider({
supported: latestVersionInfo.supported,
consistent: this.currentVersion.isConsistent,
suggestedUpgrade: latestVersionInfo.suggestedUpgrade,
suggestedIsBeta,
}),
new UpdateAvailableNotificationProvider({
suggestedUpgrade: latestVersionInfo.suggestedUpgrade,
suggestedIsBeta,
}),
];
const notificationProvider = notificationProviders.find((notificationProvider) =>
notificationProvider.mayDisplay(),
);
if (notificationProvider) {
this.notificationController.notify(notificationProvider.getSystemNotification());
}
if (this.windowController) {
IpcMainEventChannel.upgradeVersion.notify(this.windowController.webContents, upgradeVersion);
}
}

This is called when the daemon is first connected and on daemon subscription:

// fetch the latest version info in background
void this.fetchLatestVersion();

private subscribeEvents(): SubscriptionListener<DaemonEvent> {
const daemonEventListener = new SubscriptionListener(
(daemonEvent: DaemonEvent) => {
if ('tunnelState' in daemonEvent) {
this.setTunnelState(daemonEvent.tunnelState);
} else if ('settings' in daemonEvent) {
this.setSettings(daemonEvent.settings);
} else if ('relayList' in daemonEvent) {
this.setRelays(
daemonEvent.relayList,
this.settings.relaySettings,
this.settings.bridgeState,
);
} else if ('wireguardKey' in daemonEvent) {
this.handleWireguardKeygenEvent(daemonEvent.wireguardKey);
} else if ('appVersionInfo' in daemonEvent) {
this.setLatestVersion(daemonEvent.appVersionInfo);
}

@tarkah tarkah force-pushed the feature/disable-version-check branch from fafb049 to e9aba0d Compare September 1, 2021 16:06
@tarkah
Copy link
Contributor Author

tarkah commented Sep 1, 2021

I've updated this to the proposed GUI change using DISABLE_VERSION_CHECK=1 env var.

I've backported this to 2021.3 and tested, it works as expected.

  • Running without the variable shows the notification:
/usr/share/mullvad/mullvad-vpn
  • Running with the variable doesn't show the notification:
DISABLE_VERSION_CHECK=1 /usr/share/mullvad/mullvad-vpn
  • .desktop entry can be updated with the env var:
/usr/bin/env DISABLE_VERSION_CHECK=1 /usr/share/mullvad/mullvad-vpn %U
  • CLI still reports the new version:
❯ mullvad version
Current version: 2021.3
        Is supported: true
        Suggested update: 2021.4
        Latest stable version: 2021.4

gui/src/main/index.ts Outdated Show resolved Hide resolved
@tarkah tarkah changed the title Add disable-version-check feature Disable version check in GUI Sep 1, 2021
@faern faern requested a review from raksooo September 1, 2021 18:19
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

Could you add this environment variable to the readme? It would fit well under this heading: https://github.com/mullvad/mullvadvpn-app#supported-environment-variables

@tarkah tarkah requested a review from raksooo September 2, 2021 16:06
@faern
Copy link
Member

faern commented Sep 3, 2021

Can we please prefix the env variable name with MULLVAD_? All our other env vars start with MULLVAD_ or TALPID_ depending on which component they belong to. I don't want to risk overlapping with other env vars spuriously defined on the system the user is running on.

CHANGELOG.md Outdated Show resolved Hide resolved
gui/src/main/index.ts Outdated Show resolved Hide resolved
@tarkah tarkah force-pushed the feature/disable-version-check branch from edc7961 to d77e1e6 Compare September 6, 2021 00:27
@tarkah
Copy link
Contributor Author

tarkah commented Sep 6, 2021

Can we please prefix the env variable name with MULLVAD_? All our other env vars start with MULLVAD_ or TALPID_ depending on which component they belong to. I don't want to risk overlapping with other env vars spuriously defined on the system the user is running on.

Good recommendation, we definitely don't want to risk any overlaps.

I've renamed the variable to MULLVAD_DISABLE_UPDATE_NOTIFICATION as per @raksooo recommendation. Changelog and Readme have both been updated.

@tarkah tarkah requested a review from raksooo September 6, 2021 00:29
@tarkah tarkah changed the title Disable version check in GUI Disable GUI notification for newer version Sep 6, 2021
@tarkah tarkah force-pushed the feature/disable-version-check branch from d77e1e6 to 3ccd551 Compare September 6, 2021 00:33
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the contribution! 😄

@raksooo raksooo merged commit 593434e into mullvad:master Sep 6, 2021
@tarkah tarkah deleted the feature/disable-version-check branch September 6, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants