-
Notifications
You must be signed in to change notification settings - Fork 622
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
GUI Improvements #3504
base: main
Are you sure you want to change the base?
GUI Improvements #3504
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3504 +/- ##
=======================================
Coverage 88.81% 88.82%
=======================================
Files 253 253
Lines 14121 14121
=======================================
+ Hits 12542 12543 +1
+ Misses 1579 1578 -1 ☔ View full report in Codecov by Sentry. |
74a9cc5
to
2f093d9
Compare
Hey @andrei-toterman! I'm trying this out and when I click on a running instance to go to its "Shells" page, I'm not getting the automatic login to the shell. |
Hey, @townsend2010! I'll look into it, thanks for pointing it out! |
8739d4e
to
041046c
Compare
@@ -1,6 +1,6 @@ | |||
[Desktop Entry] | |||
Name=Multipass | |||
Exec=multipass.gui --autostarting | |||
Exec=multipass.gui |
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.
Do we need a separate file then?
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.
You mean separate from the data/multipass.gui.desktop
one? The autostart desktop file has to reside in the Flutter App's assets directory, so that it gets bundled with the app and can be accessed by the app. But I just tested to see if a symlink works. And it seems to work, since when the app is built, it copies the actual file to the app asset bundle, which is what we need. I'll change it.
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.
Yeah, I was thinking something like that (assuming that the regular desktop file has all that we need).
AsyncNotifierProvider.autoDispose<AutostartNotifier, bool>(() { | ||
if (Platform.isLinux) return LinuxAutostartNotifier(); | ||
if (Platform.isWindows) return WindowsAutostartNotifier(); | ||
if (Platform.isMacOS) return MacOSAutostartNotifier(); |
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.
Can we encapsulate any platform dependent code (like this one), as we do in C++?
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.
Not quite as in C++. IIUC, in C++ we decide at build time which platform implementation to use. But that is not possible in Flutter. We have to do the check at runtime and return our desired implementation based on that.
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, that is fine, but what I meant is to have any platform specific code - such as these if
s and the concrete AutostartNotifier
s - in a a separate file/folder, together with any other platform specific code. We then know that everything else is common to all platforms and that, if we we're looking for platform code, that is the only folder we need to be concerned with.
We always strived for this organization in the Multipass repo and it would be a shame to lose it now. Fortunately you already encapsulated platform behaviors in dedicated types, so it should be pretty easy to move.
} | ||
|
||
class WindowsAutostartNotifier extends AutostartNotifier { | ||
final link = Link( |
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.
Can we rely on links being enabled on Windows? Or does this create a Windows shortcut?
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.
Hmm, I didn't know symlinks could be disabled on Windows :/ I don't think I did anything special to enable them, so I'm not sure how it would work by default.
It does not create a shortcut. I'll try to see if I can disable symlinks and test how it reacts in that case.
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.
That may be why I didn't go this route when implementing autostart. I believe symlinks are disabled by default. Do you remember enabling them, perhaps when you first started working with our git repo there? Anyway, we can't expect users to have them on. However, that autostart folder may work with windows shortcuts. But yeah, better check first.
Future<void> _doSet(bool value) async { | ||
if (value) { | ||
final data = await rootBundle.load('assets/$plistFile'); | ||
await file.writeAsBytes(data.buffer.asUint8List()); |
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.
What happens if this fails?
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.
An exception is thrown. That will also run the finally
block in the base set
method, which will invalidate this provider, which updates the UI. Then the exception message will be displayed as a notification.
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.
Thanks for clarifying :)
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.
Thanks for all the time put into the GUI, @andrei-toterman! It looks great!
I don't have much for comments on the code, and have been focusing more on functional testing. I'm not sure if all the comments I've made are relevant to the changes made in this PR, though. If not, and they are things that will be changed in future iterations of the GUI, feel free to ignore.
@@ -0,0 +1 @@ | |||
/home/andrei/multipass/data/multipass.gui.desktop |
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.
Local files? Addressed to your home directory, Andrei.
|
||
@override | ||
Map<String, String> get drivers => const { | ||
'qemu': 'Qemu', |
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 think the official spelling is "QEMU"
Map<String, String> get drivers => const { | ||
'qemu': 'Qemu', | ||
'lxd': 'LXD', | ||
'libvirt': 'Libvirt', |
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.
And, here I think the spelling is all lower case.
|
||
@override | ||
Map<String, String> get drivers => const { | ||
'qemu': 'Qemu', |
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.
idem
final modifiers = [ | ||
if (alt) 'Alt', | ||
if (control) 'Ctrl', | ||
if (meta) 'Meta', |
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.
Is it possible to name this 'Cmd' or 'Win' depending on the platform. I haven't seen 'Meta' ever used in tutorials/documentation and I think it might be a bit confusing for some users.
@@ -5,8 +5,10 @@ import 'package:flutter_svg/flutter_svg.dart'; | |||
import 'package:url_launcher/url_launcher.dart'; | |||
|
|||
import '../dropdown.dart'; |
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.
Not part of this file, but I feel there should be more right margin padding in the settings list. Right now, when you move the cursor to the right side, the scroll bar will cover part of the settings value boxes. Needs to be changed in settings.dart
, I believe.
@@ -42,22 +45,10 @@ class VirtualizationSettings extends ConsumerWidget { | |||
value: networks.contains(bridgedNetwork) ? bridgedNetwork : null, |
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 feel "Virtual interface" is not a very descriptive key. There's no indication that it's for networks. Something like "Network interface" or "Virtual network interface" would be better. WDYT
ref | ||
.read(passphraseProvider.notifier) | ||
.set(value) | ||
.onError(ref.notifyError((e) => 'Failed to set passphrase: $e')); |
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.
When setting the passphrase/primary instance name, clicking the check mark applies the change (ie waiting for daemon), but the check mark persists making it unclear whether the change was applied or not.
@@ -42,22 +45,10 @@ class VirtualizationSettings extends ConsumerWidget { | |||
value: networks.contains(bridgedNetwork) ? bridgedNetwork : null, | |||
items: Map.fromIterable(networks), | |||
onChanged: (value) { | |||
ref.read(bridgedNetworkProvider.notifier).set(value!); | |||
ref.read(bridgedNetworkProvider.notifier).set(value!).onError( |
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.
On Windows (Hyper-V), in settings, the virtual interface setting does not show up right away. After a few seconds, and constantly scrolling down, it shows up. This happens every time I open the settings page.
); | ||
} | ||
|
||
void remove(int index) { |
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.
After suspending, if there is only one terminal session, there are difficulties closing it; attempting to close the tab will initially do nothing and then it will close about 30 seconds later.
public side of canonical/multipass-private#618