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

feat: tray & app menu item "Check for Updates..." #62

Merged
merged 1 commit into from Aug 2, 2022

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jul 27, 2022

Add "Check for Updates..." to the Tray menu. On macOS, add this item to the Application menu too. While the check is running, replace "Check for Updates" with a disabled item "Checking for Updates".

See #53

The implementation is heavily inspired by IPFS Desktop:

Out of scope

  • Reworking the OS-level notification
  • Showing a banner at the top of the web UI when an update is ready

Screenshots

A new macOS application menu item:

Screenshot 2022-07-27 at 14 48 56

While checking for updates:

Screenshot 2022-07-27 at 14 49 06

A new Tray menu item:

Screenshot 2022-07-27 at 14 50 35

While checking for updates:

Screenshot 2022-07-27 at 14 50 41

Various dialogs shown by the app:

Screenshot 2022-07-27 at 14 49 33

Screenshot 2022-07-27 at 14 51 43

Screenshot 2022-07-27 at 14 51 56

Screenshot 2022-07-27 at 14 52 05

The annoying notification we should remove later:

Screenshot 2022-07-27 at 14 51 24

@bajtos bajtos added the enhancement New feature or request label Jul 27, 2022
@bajtos bajtos self-assigned this Jul 27, 2022
main/app-menu.js Outdated Show resolved Hide resolved
Comment on lines +59 to +62
// TODO: replace this with autoUpdater.checkForUpdatesAndNotify()
autoUpdater.checkForUpdates()
Copy link
Member Author

Choose a reason for hiding this comment

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

This todo comment is coming from the original source code. I am not sure if it's still relevant. Do you remember @lidel why you would like to switch from autoUpdater.checkForUpdates() to autoUpdater.checkForUpdatesAndNotify()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I vaguely remember that the rationale was that it is less code to maintain, and that meant smaller surface for breaking autoupdates when upstream APIs change in subtle ways (happened in the past)

@bajtos bajtos mentioned this pull request Jul 27, 2022
3 tasks
main/app-menu.js Outdated Show resolved Hide resolved
main/app-menu.js Show resolved Hide resolved
main/app-menu.js Outdated Show resolved Hide resolved
main/updater.js Outdated Show resolved Hide resolved
@bajtos
Copy link
Member Author

bajtos commented Aug 1, 2022

@juliangruber I addressed your feedback in 172bf3b.

  • Added a code comment about Windows & Linux
  • Reworked update check interval ms('12h')

LGTY now?

When the check is running, replace "Check for Updates" with a disabled
item "Checking for Updates".

Signed-off-by: Miroslav Bajtoš <saturn@bajtos.net>
@bajtos bajtos merged commit 02aaf4d into main Aug 2, 2022
@bajtos bajtos deleted the feat-better-updater-ux branch August 2, 2022 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants