🐧 Add native title bar option for better Linux window resizing - #387
🐧 Add native title bar option for better Linux window resizing#387BluewhaleYF wants to merge 6 commits into
Conversation
| "settingsUnknownRoom": "Unknown Room", | ||
| "settingsUpdate": "Update", | ||
| "settingsWindowOpacity": "Window Opacity", | ||
| "settingsLinuxNativeTitleBar": "Use Native Title Bar", |
There was a problem hiding this comment.
actually you can expand this to all OS, maybe some users don't like the customized titlebar?
that will be a great new customize settings.
Besides, you don't need re-open a PR,
if you push to your forked repo, the PR will updated automatically.
There was a problem hiding this comment.
OK, I can expand this function to Windows
The problem is that I don't have any machine running macOS, so maybe I need your help.
There was a problem hiding this comment.
Oops, looks like you forgot change the key of localization
| "settingsUnknownRoom": "Unknown Room", | ||
| "settingsUpdate": "Update", | ||
| "settingsWindowOpacity": "Window Opacity", | ||
| "settingsLinuxNativeTitleBar": "Use Native Title Bar", |
There was a problem hiding this comment.
Oops, looks like you forgot change the key of localization
| return prefs.getBool(kAppDeveloperMode) ?? false; | ||
| }); | ||
|
|
||
| final linuxNativeTitleBarProvider = |
There was a problem hiding this comment.
Actually, it's better to merge the key with the appsettings provider, since it just a single boolean key.
And you can use @riverpod annotation + riverpod_generator to reduce the loc
| ), | ||
| ), | ||
| ), | ||
| if (Platform.isLinux) |
There was a problem hiding this comment.
Since you've expanded this feature, why not expand this if conidtion as well?
| } | ||
| } | ||
|
|
||
| final useLinuxNativeTitleBar = Platform.isLinux |
There was a problem hiding this comment.
This will break on web, because web don't have all Platform.. Even running it will break the entire app.
Use !kIsWeb to skip following code to avoid run the non-exists platform api
|
Feel free to make this PR back to ready to merge state when you done the changes. |
- Merge the native title bar setting into the app settings provider - Replace manual provider implementation with @riverpod generator - Rename the localization key consistently - Expand platform checks for desktop support - Guard platform-specific APIs on web using !kIsWeb
| "settingsTtsVolume": "Volumen", | ||
| "settingsUnknownRoom": "Unknown Room", | ||
| "settingsWindowOpacity": "Window Opacity", | ||
| "settingsLinuxNativeTitleBar": "Usar barra de título nativa", |
There was a problem hiding this comment.
Bro forgot multi language
| const kAppFestivalFeatures = 'app_feastival_features'; | ||
| const kAppWindowSize = 'app_window_size'; | ||
| const kAppWindowOpacity = 'app_window_opacity'; | ||
| const kAppLinuxUseNativeTitleBarLegacy = 'app_linux_use_native_title_bar'; |
There was a problem hiding this comment.
Wait actually why there are two keys
| final useDesktopNativeTitleBar = | ||
| !kIsWeb && (Platform.isLinux || Platform.isWindows) | ||
| ? (prefs.getBool(kAppDesktopUseNativeTitleBar) ?? | ||
| prefs.getBool(kAppLinuxUseNativeTitleBarLegacy) ?? |
There was a problem hiding this comment.
You have not shipped this feature why you already worry about the backward compatibility?
| Logger.root.info( | ||
| "[SplashScreen] Desktop window is ready with size: ${initialSize.width}x${initialSize.height}" | ||
| "${isWayland ? " (Wayland frameless fix applied)" : ""}", | ||
| "${isWayland && (!Platform.isLinux || !useDesktopNativeTitleBar) ? " (Wayland frameless fix applied)" : ""}", |
There was a problem hiding this comment.
Seems you like writing code twice instead of adding a variable
But that will cause some issue if you change the condition and forgot the log
| final isDesktop = | ||
| !kIsWeb && (Platform.isWindows || Platform.isMacOS || Platform.isLinux); | ||
| !kIsWeb && | ||
| switch (currentPlatform) { |
There was a problem hiding this comment.
You can directly use the flutter built variable here
Why you don't like writing code twice here?
| if (kIsWeb || | ||
| !isDesktop || | ||
| !(currentPlatform == TargetPlatform.linux || | ||
| currentPlatform == TargetPlatform.windows)) { |
There was a problem hiding this comment.
Don't you think this code is ugly and duplicate?
| ShakeDetector? detector; | ||
| if (!kIsWeb && (Platform.isIOS || Platform.isAndroid) && shakeEnabled) { | ||
| if (!kIsWeb && | ||
| (currentPlatform == TargetPlatform.iOS || |
There was a problem hiding this comment.
The target platform is available in all platfrom so you don't need to add the web check
| static bool get isPlatformDesktop => | ||
| !kIsWeb && (Platform.isMacOS || Platform.isLinux || Platform.isWindows); | ||
| !kIsWeb && | ||
| switch (defaultTargetPlatform) { |
There was a problem hiding this comment.
This place as well
There was a problem hiding this comment.
And we prefer to use the dart io platform check
I forgot the reason, but follow the projects style is better
|
Upstream already included this feature |
2 participants
No description provided.