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

updater: fix update is only available after download #1345

Merged
merged 3 commits into from
Feb 23, 2024
Merged

updater: fix update is only available after download #1345

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e31b97
updater: fix update is only available after download
juliangruber Feb 21, 2024
bf910b3
rename to `readyToUpdate`
juliangruber Feb 21, 2024
06ec56d
Revert "rename to `readyToUpdate`"
juliangruber Feb 23, 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
6 changes: 3 additions & 3 deletions main/updater.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ let updateNotification = null

let checkingManually = false

let updateAvailable = false
let updateDownloaded = false

/** @type {string | undefined} */
let nextVersion
Expand Down Expand Up @@ -54,7 +54,7 @@ module.exports = async function setupUpdater (
/** @type {import('./typings').Context} */ ctx
) {
ctx.getUpdaterStatus = function getUpdaterStatus () {
return { updateAvailable }
return { updateAvailable: updateDownloaded }
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to rename the places reading this property too. Otherwise, we are using different names ("update available" vs "update downloaded") for the same thing and this could create confusion in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about this too, but decided against this because I think the places outside this library shouldn't have to care about the finer details of updating. I could see however how updateDownloaded is more clear. I was just thinking that as an outsider, "update downloaded" could be more confusing than "update available" (ok it's downloaded, are there any other steps?). Maybe instead something like readyToUpdate?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with your reasoning. 👍🏻

I like the idea of renaming the property to readyToUpdate or perhaps updateReady.

}

ctx.openReleaseNotes = openReleaseNotes
Expand Down Expand Up @@ -131,7 +131,6 @@ function onUpdaterError (err) {
* @param {import('electron-updater').UpdateInfo} info
*/
function onUpdateAvailable ({ version /*, releaseNotes */ }) {
updateAvailable = true
nextVersion = version

log.info(`Update to version ${version} is available, downloading..`)
Expand Down Expand Up @@ -168,6 +167,7 @@ function onUpdateNotAvailable ({ version }) {
* @param {import('electron-updater').UpdateDownloadedEvent} event
*/
function onUpdateDownloaded (ctx, { version /*, releaseNotes */ }) {
updateDownloaded = true
log.info(`update to ${version} downloaded`)

const showUpdateDialog = () => {
Expand Down