-
Notifications
You must be signed in to change notification settings - Fork 636
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
Desktop gui #3282
Desktop gui #3282
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3282 +/- ##
==========================================
- Coverage 88.89% 88.80% -0.10%
==========================================
Files 254 252 -2
Lines 14084 14019 -65
==========================================
- Hits 12520 12449 -71
- Misses 1564 1570 +6 ☔ View full report in Codecov by Sentry. |
58070cf
to
945db0e
Compare
8c34ede
to
ad72159
Compare
a36aef4
to
b99e118
Compare
5475c53
to
c00c70d
Compare
65aff18
to
900d203
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.
Hey @andrei-toterman!
Again, fantastic work here! I'm submitting this review as-is because I pushed some changes for the Snap package and it seems I can't view some of my saved comments. I will follow up with more comments as necessary.
Hey @andrei-toterman, I think since the original GUI was removed, we can go back to using Also, I'm still thinking about the name of this, both the executable and the app itself. For the time being, let's rename it back to |
I have also made quite a few changes to the snap package to trim it down. It went from ~82MB down to ~64MB. There is still some room for improvement, but I think this is sufficient for now. |
snap/snapcraft.yaml
Outdated
flutter pub get | ||
flutter build linux --release --verbose --target lib/main.dart | ||
organize: | ||
/root/parts/desktop-gui/build/build/linux/x64/release/bundle: bin/ |
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'm not a real fan of all of the parts of the GUI bundle being dumped into bin/
. It's ok for now, but we should do better in another PR.
A UI "issue" I've found is that when you are in the list of instances and select one or more instances and then do an action such as start, stop, etc., the selections stay active and must be de-selected even after the action completes. I think that once the action is complete, the instances should automatically be de-selected. @vikorama, what do you think? |
Very good catch @townsend2010! I think the instance selection should stay selected as long as the user stays on the 'All instances' page. For example a flow like this: I select 5 running instances, pause them to free up recourses, do what I need to do in other apps, get back to Multipass and run the selected instances again. I think that the selection should get de-selected when:
What do you think? |
Hey @vikorama! I think what you propose makes perfect sense! Thank you for your input! |
7e094ab
to
5bf2846
Compare
Hey, @vikorama! Thanks for the feedback! Indeed, most of the changes that you mentioned are not in the latest package. They are implemented, but due to various CI shenanigans, we couldn't obtain the packages with those changes. Hopefully they will be ready today. |
f8cf4be
to
d979278
Compare
1b862a7
to
c7c66df
Compare
@andrei-toterman As discussed, let's keep the context switcher as is for now. I agree that it's better to avoid it jumping around due to different name lengths. We might need to improve the visual design of the top panel in the future anyways, and it'd be easier to do it when we collect feedback after the first launch. |
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.
Ok, this looks great in it's current form. Let's get it in!!! 🚀
Fixes #2895