-
Notifications
You must be signed in to change notification settings - Fork 650
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
change window minimumSize to 750x450, and default size based on screen resolution #3663
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3663 +/- ##
==========================================
- Coverage 88.88% 88.86% -0.03%
==========================================
Files 254 254
Lines 14256 14269 +13
==========================================
+ Hits 12672 12680 +8
- Misses 1584 1589 +5 ☔ View full report in Codecov by Sentry. |
eb52311
to
f2fd89e
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, modulo the inline suggestion.
src/client/gui/lib/main.dart
Outdated
if (screen != null && screen.frame.size.width > 1600 | ||
&& screen.frame.size.height > 900) { |
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.
Resolutions tend to stick to round numbers, so maybe use >=
.
f2fd89e
to
b384852
Compare
src/client/gui/lib/main.dart
Outdated
|
||
await setupLogger(); | ||
// Get the current screen size | ||
final screen = await getCurrentScreen(); |
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.
Why is this removing the logger setup?
Also, since we only care about the screen size, not the entire screen object, you could do something like
final screenSize = await getCurrentScreen().then((screen) {
return screen?.frame.size;
});
so that you don't have to do the .frame.size
thing multiple times
src/client/gui/lib/main.dart
Outdated
Size windowSize; | ||
|
||
await windowManager.ensureInitialized(); | ||
const windowOptions = WindowOptions( | ||
if (screen != null && screen.frame.size.width >= 1600 | ||
&& screen.frame.size.height >= 900) { | ||
windowSize = const Size(1400, 822); // For screens larger than 1600x900 | ||
} else { | ||
windowSize = const Size(750, 450); // Default window size | ||
} |
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.
Why is this removing the initialization of the window manager?
Also, you could make windowSize
final and initialize it with a ternary operator instead of the if-else
.
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, @levkropp! It looks much better now. But I noticed something is missing. You added a plugin, which also modified the plugin registry files for each platform. And those changes must also be committed.
So you'll have to do something like this in the src/client/gui
directory:
flutter config --enable-macos-desktop --enable-windows-desktop
flutter pub get
This should modify some files inside the linux
, macos
and windows
directories related to registered plugins. Please commit those as well.
If you don't already have flutter in your PATH, you can add the multipass/3rd-party/flutter/bin
directory to your path. (And by this I mean of course the entire path of that directory)
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.
Alright, LGTM. Thanks, @levkropp!
close #3590
We can use the window_size flutter plugin from google's flutter desktop embeddings repository https://github.com/google/flutter-desktop-embedding/tree/main/plugins/window_size
to check the size of the screen, and
We'll need to add this plugin to pubspec.yaml, but it's
an official plugin from Googlethe closest-to-official Flutter plugin that handles screen size and it seems that the only reason that this plugin needs to be added at all is because Google has not yet added this functionality to the Flutter framework itself