-
Notifications
You must be signed in to change notification settings - Fork 735
[gui] Adding an about section in the Settings tab #4420
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
base: main
Are you sure you want to change the base?
Conversation
626223a
to
22835c7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4420 +/- ##
=======================================
Coverage 89.05% 89.05%
=======================================
Files 240 240
Lines 15243 15243
=======================================
Hits 13575 13575
Misses 1668 1668 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @tobe2098 , is this PR ready for a first round of review? Just some quick comments from looking at this quickly ![]() It looks like you followed the tray menu's style exactly, but I think it would look better if I think the version text would look nicer if it was either right-justified or centered. I'm slightly preferring centered since that aligns it with the Click to Copy tooltip and make it more obvious that this is not a setting you can change Since we're not limited to the ASCII of a tray menu, instead of (C) we should probably just use the actual copyright symbol © Food for thought: Why do we show both the multipass and multipassd versions to end users in the GUI on the tray menu and now here? In 99% of normal use cases they will be the same. It might be an interesting idea to always show just |
It is ready, thank you for the review. I think you are correct, homogeneous style is better. The different versions part is only really useful for us, since we can have the daemon running, rebuild and think that we are already in the new build fully, even though the daemon is still in the previous version. I agree that only showing it when it is different makes more sense, maybe with some warning color. The additional pop-up also makes sense, but I would keep the About section part. We can also apply the mismatch policy to the TrayMenu as well, since it has the same implications/uses. What do you think? |
22835c7
to
30e3cbd
Compare
This is looking a lot better, thanks @tobe2098 ! ![]() My only nitpick is to remove the colon from |
LGTM! +1 with @levkropp on the colon. |
30e3cbd
to
93b724c
Compare
Done! |
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.
I was testing this on KDE with Kubuntu 24.04, so it is entirely possible that this issue is with KDE and how Flutter handles tray icons on KDE. I will test these changes on GNOME now as well |
Alright so I tested it on GNOME as well, and the issue actually is that if you turn off the daemon and the GUI spins with "waiting for daemon", and THEN you run a new daemon with a different version than the client, the Settings tab updates to show both multipass and daemon version but the tray menu doesn't. This is probably because we're re-checking the provider for the multipass version for the settings page but not doing something similar for tray menu |
I'll see if I can fix it. I was testing the other way around, that is why I did not see it. |
This should keep the position consistent while having the same behavior as in the GUI |
24b6cd1
to
ba3ff71
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.
LGTM now! Working in GNOME (but still not KDE, but that's almost certainly something to do with how Flutter and KDE interact, and not with our code).
I am somewhat interested: In what sense does it not work? Does it still show up always or is it disordered? |
What do you think @amylily1011, do you find the new changes to the version reporting in the GUI agreeable? |
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.
I am somewhat interested: In what sense does it not work? Does it still show up always or is it disordered?
It is still never showing up when the daemon and client versions are different.
Also, merging this would break the riverpod PR, and we'd probably need to handle the way providers are implemented differently once that PR is merged and we rebase on main. Also I think there are some issues with daemonVersionProvider so let's wait to merge this for a little (sorry for the hasty approval)
No worries there. It can wait, it is nothing critical |
The UI LGTM! Thanks for the update 😃 |
ba3ff71
to
73a95eb
Compare
Additional styling changes also implemented.
Now the version is only shown in the case the daemon and multipass versions do not match.
73a95eb
to
5648d1f
Compare
The PR adds an About section, similar to the one found in the tray menu, that shows the multipass and multipassd versions and the Canonical copyright. Tested in Ubuntu and Windows 11.
Fixes #4201
MULTI-2294