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.

Sign up for GitHub

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

Desktop: Seamless-Updates - fixing bugs #11121

Closed
wants to merge 7 commits into from
Closed

Desktop: Seamless-Updates - fixing bugs #11121

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5faee24
Seamless-Updates - implemented locking mechanism for checking updates
AliceHincu Sep 24, 2024
4f44d0e
Seamless-Updates - implemented locking mechanism for checking updates
AliceHincu Sep 24, 2024
8251159
Seamless-Updates - managing remote process in update service
AliceHincu Sep 24, 2024
a2940b7
Seamless-Updates - notify if update check is already in progress
AliceHincu Sep 25, 2024
d03c27c
Seamless-Updates - notification message is now shown everytime instea…
AliceHincu Sep 25, 2024
b9f018d
Seamless-Updates - notify if update check is already in progress
AliceHincu Sep 25, 2024
3f09c91
Merge branch 'dev' into seamless-updates
personalizedrefrigerator Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions packages/app-desktop/ElectronAppWrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import Logger, { LoggerWrapper } from '@joplin/utils/Logger';
import { PluginMessage } from './services/plugins/PluginRunner';
import AutoUpdaterService, { defaultUpdateInterval, initialUpdateStartup } from './services/autoUpdater/AutoUpdaterService';
import AutoUpdaterService, { CheckForUpdatesArgs, defaultUpdateInterval, initialUpdateStartup } from './services/autoUpdater/AutoUpdaterService';
import type ShimType from '@joplin/lib/shim';
const shim: typeof ShimType = require('@joplin/lib/shim').default;
import { isCallbackUrl } from '@joplin/lib/callbackUrlUtils';

import { BrowserWindow, Tray, screen } from 'electron';
import { BrowserWindow, IpcMainEvent, Tray, screen } from 'electron';
import bridge from './bridge';
const url = require('url');
const path = require('path');
Expand Down Expand Up @@ -332,8 +332,8 @@ export default class ElectronAppWrapper {
this.updaterService_.updateApp();
});

ipcMain.on('check-for-updates', () => {
void this.updaterService_.checkForUpdates(true);
ipcMain.on('check-for-updates', (_event: IpcMainEvent, _message: string, args: CheckForUpdatesArgs) => {
void this.updaterService_.checkForUpdates(true, args.includePreReleases);
});

// Let us register listeners on the window, so we can update the state
Expand Down Expand Up @@ -478,12 +478,11 @@ export default class ElectronAppWrapper {
if (shim.isWindows() || shim.isMac()) {
if (!this.updaterService_) {
this.updaterService_ = new AutoUpdaterService(this.win_, logger, devMode, includePreReleases);
this.startPeriodicUpdateCheck();
}
}
}

private startPeriodicUpdateCheck = (updateInterval: number = defaultUpdateInterval): void => {
public startPeriodicUpdateCheck = (updateInterval: number = defaultUpdateInterval): void => {
this.stopPeriodicUpdateCheck();
this.updatePollInterval_ = setInterval(() => {
void this.updaterService_.checkForUpdates(false);
Expand Down
15 changes: 10 additions & 5 deletions packages/app-desktop/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,12 +405,17 @@ class Application extends BaseApplication {
}

private setupAutoUpdaterService() {
// since the remote process doesn't stop running after app is closed, we need to initialize the service, even if its flag is set to false, or else it will throw an error.
bridge().electronApp().initializeAutoUpdaterService(
Logger.create('AutoUpdaterService'),
Setting.value('env') === 'dev',
Setting.value('autoUpdate.includePreReleases'),
);
Comment on lines +425 to +429
Copy link
Owner

Choose a reason for hiding this comment

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

Likewise we can't have this running if the user didn't enable the feature. That's the purpose of this flag, to enable it in a controlled way. I don't really understand the logic here, why we need to run this if the flag is not on? If the user needs to restart the app after enabling the flag, we just document it. The flag is a temporary, advanced feature anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

In the explanation below, by saying "default flag" I refer to the flag previously used for auto updates, together with the existing logic from before. By saying "implemented flag", I refer to the flag I added for my service.

Explanation: let's say the user has the default flag set to true and the implemented flag set to false. If he/she goes to Options to set the implemented flag to true, and then goes back into the application to manually check for an update without closing or quitting the app, the above error will be shown, because the service was never initialized in the first place (since the implemented flag was initially false).

I even tried to add this check so we could avoid this situation:
if (typeof this.updaterService_ != 'undefined' && this.updaterService_ && this.updaterService_?.checkForUpdates)
Unfortunately it still throws the error.

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure about the technical details, but it should be possible to turn on your feature flag and it only uses your new updater service, or turn it off and it only uses the old method. The fact that something is not working when one of the flag is on or off means some logic is missing somewhere


// since the remote process doesn't stop running after app is closed, the period check starts only if the flag is set to true and the app is quit from the system tray.
// if the user sets the flag to true and closes the app but does not quit the app from the system tray, the periodic check won't start. The manual check will work.
if (Setting.value('featureFlag.autoUpdaterServiceEnabled')) {
bridge().electronApp().initializeAutoUpdaterService(
Logger.create('AutoUpdaterService'),
Setting.value('env') === 'dev',
Setting.value('autoUpdate.includePreReleases'),
);
bridge().electronApp().startPeriodicUpdateCheck();
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/app-desktop/gui/MenuBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ function useMenu(props: Props) {

function _checkForUpdates() {
if (Setting.value('featureFlag.autoUpdaterServiceEnabled')) {
ipcRenderer.send('check-for-updates');
ipcRenderer.send('check-for-updates', '', { includePreReleases: Setting.value('autoUpdate.includePreReleases') });
} else {
void checkForUpdates(false, bridge().window(), { includePreReleases: Setting.value('autoUpdate.includePreReleases') });
}
Expand Down
29 changes: 15 additions & 14 deletions packages/app-desktop/gui/UpdateNotification/UpdateNotification.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { themeStyle } from '@joplin/lib/theme';
import NotyfContext from '../NotyfContext';
import { UpdateInfo } from 'electron-updater';
import { ipcRenderer, IpcRendererEvent } from 'electron';
import { AutoUpdaterEvents } from '../../services/autoUpdater/AutoUpdaterService';
import { NotyfEvent, NotyfNotification } from 'notyf';
import { AutoUpdaterEvents, UpdateNotificationMessage } from '../../services/autoUpdater/AutoUpdaterService';
import { NotyfNotification } from 'notyf';
import { _ } from '@joplin/lib/locale';
import { htmlentities } from '@joplin/utils/html';
import shim from '@joplin/lib/shim';
Expand All @@ -16,11 +16,11 @@ interface UpdateNotificationProps {

export enum UpdateNotificationEvents {
ApplyUpdate = 'apply-update',
UpdateNotAvailable = 'update-not-available',
Dismiss = 'dismiss-update-notification',
}

const changelogLink = 'https://github.com/laurent22/joplin/releases';
const notificationDuration = 5000; // 5 seconds

window.openChangelogLink = () => {
shim.openUrl(changelogLink);
Expand Down Expand Up @@ -87,10 +87,10 @@ const UpdateNotification = ({ themeId }: UpdateNotificationProps) => {
notificationRef.current = notification;
}, [notyf, theme]);

const handleUpdateNotAvailable = useCallback(() => {
const handleNotificationMessage = useCallback((_event: IpcRendererEvent, args: UpdateNotificationMessage) => {
if (notificationRef.current) return;

const noUpdateMessageHtml = htmlentities(_('No updates available'));
const noUpdateMessageHtml = htmlentities(_('%s', args.message));

const messageHtml = `
<div class="update-notification" style="color: ${theme.color2};">
Expand All @@ -105,28 +105,29 @@ const UpdateNotification = ({ themeId }: UpdateNotificationProps) => {
x: 'right',
y: 'bottom',
},
duration: 5000,
duration: notificationDuration,
});

notification.on(NotyfEvent.Dismiss, () => {
notificationRef.current = null;
});

notificationRef.current = notification;

setTimeout(() => {
if (notificationRef.current === notification) {
notificationRef.current = null;
}
}, notificationDuration);
}, [notyf, theme]);

useEffect(() => {
ipcRenderer.on(AutoUpdaterEvents.UpdateDownloaded, handleUpdateDownloaded);
ipcRenderer.on(AutoUpdaterEvents.UpdateNotAvailable, handleUpdateNotAvailable);
ipcRenderer.on(AutoUpdaterEvents.NotificationMessage, handleNotificationMessage);
document.addEventListener(UpdateNotificationEvents.ApplyUpdate, handleApplyUpdate);
document.addEventListener(UpdateNotificationEvents.Dismiss, handleDismissNotification);

return () => {
ipcRenderer.removeListener(AutoUpdaterEvents.UpdateDownloaded, handleUpdateDownloaded);
ipcRenderer.removeListener(AutoUpdaterEvents.UpdateNotAvailable, handleUpdateNotAvailable);
ipcRenderer.removeListener(AutoUpdaterEvents.NotificationMessage, handleNotificationMessage);
document.removeEventListener(UpdateNotificationEvents.ApplyUpdate, handleApplyUpdate);
};
}, [handleApplyUpdate, handleDismissNotification, handleUpdateDownloaded, handleUpdateNotAvailable]);
}, [handleApplyUpdate, handleDismissNotification, handleUpdateDownloaded, handleNotificationMessage]);


return (
Expand Down
49 changes: 45 additions & 4 deletions packages/app-desktop/services/autoUpdater/AutoUpdaterService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ export enum AutoUpdaterEvents {
Error = 'error',
DownloadProgress = 'download-progress',
UpdateDownloaded = 'update-downloaded',
NotificationMessage = 'notify-with-message',
}

export interface CheckForUpdatesArgs {
includePreReleases: boolean;
}
export interface UpdateNotificationMessage {
message: string;
}

export const defaultUpdateInterval = 12 * 60 * 60 * 1000;
Expand Down Expand Up @@ -53,6 +61,7 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {
private includePreReleases_ = false;
private allowDowngrade = false;
private isManualCheckInProgress = false;
private isUpdateInProgress = false;

public constructor(mainWindow: BrowserWindow, logger: LoggerWrapper, devMode: boolean, includePreReleases: boolean) {
this.window_ = mainWindow;
Expand All @@ -62,15 +71,28 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {
this.configureAutoUpdater();
}

public checkForUpdates = async (isManualCheck = false): Promise<void> => {
public checkForUpdates = async (isManualCheck = false, includePreReleases = this.includePreReleases_): Promise<void> => {
if (this.isUpdateInProgress) {
this.logger_.info('Update check already in progress. Waiting for the current check to finish.');
if (isManualCheck) {
this.sendNotification('Update check already in progress.');
}
return;
}

this.lockUpdateProcess();
this.isManualCheckInProgress = isManualCheck;

try {
this.isManualCheckInProgress = isManualCheck;
autoUpdater.allowPrerelease = includePreReleases;
await this.checkForLatestRelease();
} catch (error) {
this.logger_.error('Failed to check for updates:', error);
if (error.message.includes('ERR_CONNECTION_REFUSED')) {
this.logger_.info('Server is not reachable. Will try again later.');
}
} finally {
this.isManualCheckInProgress = false;
}
};

Expand Down Expand Up @@ -134,7 +156,6 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {
assetUrl = assetUrl.substring(0, assetUrl.lastIndexOf('/'));
autoUpdater.setFeedURL({ provider: 'generic', url: assetUrl });
await autoUpdater.checkForUpdates();
this.isManualCheckInProgress = false;
} catch (error) {
this.logger_.error(`Update download url failed: ${error.message}`);
}
Expand All @@ -145,6 +166,7 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {
};

private configureAutoUpdater = (): void => {
this.logger_.info('Initiating ...');
autoUpdater.logger = (this.logger_) as Logger;
if (this.devMode_) {
this.logger_.info('Development mode: using dev-app-update.yml');
Expand All @@ -171,9 +193,10 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {

private onUpdateNotAvailable = (_info: UpdateInfo): void => {
if (this.isManualCheckInProgress) {
this.window_.webContents.send(AutoUpdaterEvents.UpdateNotAvailable);
this.sendNotification('Update is not available.');
}

this.unlockUpdateProcess();
this.logger_.info('Update not available.');
};

Expand All @@ -187,6 +210,7 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {

private onUpdateDownloaded = (info: UpdateInfo): void => {
this.logger_.info('Update downloaded.');
this.unlockUpdateProcess();
void this.promptUserToUpdate(info);
};

Expand All @@ -197,4 +221,21 @@ export default class AutoUpdaterService implements AutoUpdaterServiceInterface {
private promptUserToUpdate = async (info: UpdateInfo): Promise<void> => {
this.window_.webContents.send(AutoUpdaterEvents.UpdateDownloaded, info);
};

private lockUpdateProcess = (): void => {
this.logger_.info('Locking update process');
this.isUpdateInProgress = true;
};

private unlockUpdateProcess = (): void => {
this.logger_.info('Unlocking update process');
this.isUpdateInProgress = false;
};

private sendNotification = (message: string): void => {
const notificationMessage: UpdateNotificationMessage = {
message: message,
};
this.window_.webContents.send(AutoUpdaterEvents.NotificationMessage, notificationMessage);
};
}
6 changes: 3 additions & 3 deletions packages/lib/models/settings/builtInMetadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,8 +1127,8 @@ const builtInMetadata = (Setting: typeof SettingType) => {
},


autoUpdateEnabled: { value: false, type: SettingItemType.Bool, storage: SettingStorage.File, isGlobal: true, section: 'application', public: false, appTypes: [AppType.Desktop], label: () => _('Automatically check for updates') },
'autoUpdate.includePreReleases': { value: false, type: SettingItemType.Bool, section: 'application', storage: SettingStorage.File, isGlobal: true, public: true, appTypes: [AppType.Desktop], label: () => _('Get pre-releases when checking for updates'), description: () => _('See the pre-release page for more details: %s', 'https://joplinapp.org/help/about/prereleases') },
autoUpdateEnabled: { value: true, type: SettingItemType.Bool, storage: SettingStorage.File, isGlobal: true, section: 'application', public: false, appTypes: [AppType.Desktop], label: () => _('Automatically check for updates') },
'autoUpdate.includePreReleases': { value: false, type: SettingItemType.Bool, section: 'application', storage: SettingStorage.File, isGlobal: true, public: true, appTypes: [AppType.Desktop], label: () => _('Get pre-releases when checking for updates'), description: () => _('See the pre-release page for more details: %s. Restart app (quit app from system tray) to start getting them', 'https://joplinapp.org/help/about/prereleases') },
Copy link
Owner

Choose a reason for hiding this comment

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

We talked about this before - we can't have this enabled by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flag is not the flag for my service.

https://github.com/laurent22/joplin/pull/11079/files

I set it to false in the last PR, and I switched it back to true here (like how it was before). I also hid it.

Would you like to have both flags set to false ? In this case, when the user triggers a manual check, what should happen ?

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I missed you switched that off - but why did you do this? That's exactly what we did not want to happen, we talked about it and the whole reason why we have an extra feature flag for you. Now everybody who got this version will no longer be notified of new versions. I'm going to turn that back on and create another prerelease.


'autoUploadCrashDumps': {
value: false,
Expand Down Expand Up @@ -1559,7 +1559,7 @@ const builtInMetadata = (Setting: typeof SettingType) => {
storage: SettingStorage.File,
appTypes: [AppType.Desktop],
label: () => 'Enable auto-updates',
description: () => 'Enable this feature to receive notifications about updates and install them instead of manually downloading them. Restart app to start receiving auto-updates.',
description: () => 'Enable this feature to receive notifications about updates and install them instead of manually downloading them. Restart app (quit app from system tray) to start receiving auto-updates.',
show: () => shim.isWindows() || shim.isMac(),
section: 'application',
isGlobal: true,
Expand Down
Loading