-
Notifications
You must be signed in to change notification settings - Fork 835
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
chore: improve platform check by using constants and defaultTargetPlatform #2188
Conversation
if (isKeyboardOS(supportWeb: true)) { | ||
if (isKeyboardOS) { |
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 haven't understood what this check mean by keyboardOS
, I assume it's an operating system with a built-in physical keyboard. It checks if it's desktop or fuchsia OS and fuchsia OS doesn't come with a built-in keyboard AFAIK.
final platform = Theme.of(_state.context).platform; | ||
if (isAppleOS( | ||
platform: platform, | ||
supportWeb: true, | ||
)) { | ||
if (Theme.of(_state.context).isCupertino) { |
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.
Many of the checks here depend on ThemeData
from MaterialApp
. Usually, it should be for styling purposes so I created an extension called isCupertino
to be a bit clear.
We need to double-check if this platform-specific logic shouldn't be used when overriding the platform with an Apple operating system on Android or other platforms.
This PR clears up a lot of the confusion on implementing platform specific options. I have never understood Comment: The impact on tree-shaking is something I have not considered but will be very important when there is a platform-specific option that could increase program size by multi-megabytes. I note that text_selection.dart and delegate.dart are already using defaultTargetPlatform. |
If
The choice is a bit random, which leads to inconsistency since we will choose something different each time. I decided to stick to
Usually, this will be useful to help improve the performance a bit where using platform check extensively, for example, when building platform adaptive application with a lot of check-in
The |
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 your comments. Looks good.
Description
Improve the platform check by using
kIsWeb
anddefaultTargetPlatform
to allow it to be tested and tree shaken.kIsWeb
will always betrue
ifkIsWasm
is in case of Flutter/WASMdefaultTargetPlatform
overPlatform.operatingSystem
from `dart: is that it can be tested and overridden, and supports the web in case we want to check if the platform regardless if it's on a web browser.defaultTargetPlatform
has been updated to usevm:platform-const-if
pragma introduced in dart-lang/sdk@57a1168875 in Flutter #141105 which allow it to be computed as if it was a constant field in non-debug builds which means platform-specific code executed conditionally based ondefaultTargetPlatform
can be tree-shaken in release buildsisIos
which will check if the operating system is iOS which will betrue
even on the web,isIosApp
will require the platform to be an iOS app and non-web to betrue
:Also added
@pragma('vm:platform-const-if', !kDebugMode)
on each property,kIsWeb
is a constant field,defaultTargetPlatform
is constant only on non-debug builds, I'm uncertain if we need the pragma annotation for each field (e.g.isAndroidApp
orisMobileApp
) to be computed as constants and tree shaken same way if we usedefaultTargetPlatform
directly.If we take a look at the file
platform.dart
fromdart:io
, all fields that are usingoperatingSystem
:are also using
@pragma("vm:platform-const")
:Related Issues
I expect some regressions from this PR, as such it's not ready for merge yet.
Also see:
defaultTargetPlatform
docsType of Change