-
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 :)
public side of canonical/multipass-private#618