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

Show in-app notification if an update is available #3621

Merged
merged 12 commits into from
Sep 12, 2024

Conversation

andrei-toterman
Copy link
Contributor

@andrei-toterman andrei-toterman commented Aug 2, 2024

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.

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.85%. Comparing base (ddf2fc9) to head (e655413).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
src/client/cli/cmd/info.cpp 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sharder996 sharder996 left a 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.

@sharder996
Copy link
Contributor

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.

@ricab
Copy link
Collaborator

ricab commented Sep 9, 2024

Live notifications would be better, to quickly get to people who just suspend their laptop and keep the GUI running.

sharder996
sharder996 previously approved these changes Sep 12, 2024
Copy link
Contributor

@sharder996 sharder996 left a 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 sharder996 added this pull request to the merge queue Sep 12, 2024
@andrei-toterman andrei-toterman removed this pull request from the merge queue due to a manual request Sep 12, 2024
@andrei-toterman
Copy link
Contributor Author

@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 😅

@sharder996 sharder996 added this pull request to the merge queue Sep 12, 2024
Merged via the queue into main with commit 793f914 Sep 12, 2024
12 of 13 checks passed
@sharder996 sharder996 deleted the update-available-notif branch September 12, 2024 22:02
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.

3 participants