GUI Improvements #3504
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. |
andrei-toterman
force-pushed
the
desktop-gui
branch
10 times, most recently
from
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! |
andrei-toterman
force-pushed
the
desktop-gui
branch
from
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.
Do we need a separate file then?
There was a problem hiding this comment.
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.
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.
Can we encapsulate any platform dependent code (like this one), as we do in C++?
There was a problem hiding this comment.
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.
OK, that is fine, but what I meant is to have any platform specific code - such as these ifs and the concrete AutostartNotifiers - 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.
Can we rely on links being enabled on Windows? Or does this create a Windows shortcut?
There was a problem hiding this comment.
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.
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.
What happens if this fails?
There was a problem hiding this comment.
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.
public side of canonical/multipass-private#618