-
Notifications
You must be signed in to change notification settings - Fork 658
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
Show in-app notification if an update is available #3621
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3621 /- ##
==========================================
- Coverage 88.86% 88.85% -0.01%
==========================================
Files 254 254
Lines 14266 14269 3
==========================================
Hits 12677 12679 2
- Misses 1589 1590 1 ☔ View full report in Codecov by Sentry. |
8420eaf
to
145e60c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and I see the update notification. However, it only shows up on launching the GUI. If I leave the GUI running long enough for DefaultUpdatePrompt::is_time_to_show()
to return true again, I don't get another notification telling me about a newer version.
As a follow-up, I'm not sure which is the preferred behaviour. Showing an update notification once on launch, or as per the update prompt timer. Multipass doesn't update that often so getting "live" update notifications might not be an issue. |
Live notifications would be better, to quickly get to people who just suspend their laptop and keep the GUI running. |
the notifier makes sure that, if needed, only one update notification is on screen for a given version
145e60c
to
6a3f021
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Noticed that the update notification is not showing up in start
and restart
via the CLI even though we are piping the update info through the grpc reply. Addressing that is outside the scope of this PR, so I will create a bug report for it.
@sharder996 sorry to bother, but I just realized I was over-complicating things a little when extracting the update info from the rpc reply, so I pushed an update. Again, sorry for interrupting the merge 😅 |
Show in-app notification if an update is available
This PR adds a notification if there is a Multipass update available.
There is also a refactoring for the update entry in the settings page.