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

Automatic Update Checking #153

Merged
merged 1 commit into from
Jul 10, 2021
Merged

Conversation

QbDesu
Copy link
Contributor

@QbDesu QbDesu commented Jun 1, 2021

Automatically checks if updates for the installer are available using the Github Releases API. Clicking on download will terminate the installer and open a the release page in the browser. It requires future release tags to be valid semver versions (optionally prefixed with v.). The current installer version is taken from the package.json. For testing purposes changing the version in the package json to something below 1.0.0 should trigger the prompt. The update checking is done asynchronously because I noticed a significantly increased startup time if it's done synchronously before the installer window starts.

image

Implementation of the feature request from #151

@QbDesu
Copy link
Contributor Author

QbDesu commented Jun 3, 2021

the console logs will probably have to go anyway even though I think they are quite useful. But the current eslint config forbids using the console logging functions. (It also enforces CRLF line breaks despite the whole code base being LF only)

@ObserverOfTime
Copy link

There should be a toggle so package maintainers (see: AUR, Nix) can disable this without having to patch it out.

Copy link
Member

@rauenzi rauenzi left a comment

Choose a reason for hiding this comment

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

I'm glad to see from the code that this is a native dialog because my eyes were quite... shocked... by the screenshot in this PR.

src/main/index.js Outdated Show resolved Hide resolved
src/main/update_installer.js Outdated Show resolved Hide resolved
src/main/update_installer.js Outdated Show resolved Hide resolved
src/main/update_installer.js Outdated Show resolved Hide resolved
src/main/update_installer.js Outdated Show resolved Hide resolved
@QbDesu QbDesu force-pushed the update-checking branch 2 times, most recently from 91e5057 to 771ec74 Compare June 9, 2021 07:29
@QbDesu
Copy link
Contributor Author

QbDesu commented Jun 9, 2021

I'm glad to see from the code that this is a native dialog because my eyes were quite... shocked... by the screenshot in this PR.

Jup that's just a system dialog using my system's Adawaita Dark theme.
Do you mind the console logging statements? If so I can remove them of course.

There should be a toggle so package maintainers (see: AUR, Nix) can disable this without having to patch it out.

@ObserverOfTime I saw you are the maintainer of the AUR package.. since I don't know how the packaging works, do you have a suggestion on how it could be configured? I don't know what the goto way to do this is.

@ObserverOfTime
Copy link

You could introduce an environment variable like BD_AUTOUPDATE which can be set to a falsy value to disable it.

package.json Outdated Show resolved Hide resolved
src/main/update_installer.js Outdated Show resolved Hide resolved
});

if (result.response === 0) {
await shell.openExternal(latestRelease.html_url);
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, but should we take them to the release page, or should we go to the corresponding OS's asset download url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm personally fine with either, the added complexity is minimal. but that would also require hard-coding (or at least semi-hardcoding) the filenames for the OS files.

Copy link
Member

Choose a reason for hiding this comment

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

Well it wouldn't have to be complete hardcoded could do something like:

let asset = assets.find(a => a.name.toLowerCase().includes("linux"); // Linux default 
if (process.platform === "win32") asset = assets.find(a => a.name.toLowerCase().includes("windows");
else if (process.platform === "darwin") asset = assets.find(a => a.name.toLowerCase().includes("mac");

await shell.openExternal(asset.browser_download_url);

But I'm still not sure if that option is truly better.

Copy link
Contributor Author

@QbDesu QbDesu Jul 10, 2021

Choose a reason for hiding this comment

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

An added bonus of linking the release page would be showing a changelog as well, even though though I expect most people to not read it.
And maybe we can put little text snippet on there after new releases that smart screen migtht pop up but it's fine or something.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that

@rauenzi
Copy link
Member

rauenzi commented Jul 10, 2021

@ObserverOfTime is the implementation here sufficient for your needs?

@ObserverOfTime
Copy link

Yes, we can just wrap the installer in a script with env BD_SKIP_UPDATECHECK=1 ... once this is released.

/cc @IvarWithoutBones

@QbDesu QbDesu changed the base branch from main to development July 10, 2021 13:54
@rauenzi rauenzi merged commit 4e53347 into BetterDiscord:development Jul 10, 2021
@IvarWithoutBones
Copy link

Thanks for the ping! I'll make sure to wrap it upon the next release :)

@ObserverOfTime
Copy link

@IvarWithoutBones The next release is available now :)
Incidentally, you should change the homepage to betterdiscord.app

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

4 participants