Open IchordeDionysos opened 1 year ago
This sounds like this might have something to do with Flutter. @goderbauer ?
In general, using the more specific methods is the better choice as it will avoid unnecessary rebuilds.
To be discussed whether using MediaQuery.of(context) should be allowed when accessing multiple properties of the MediaQueryData.
This is an open question I would have as well. My feeling is that even if you access a handful of properties you should probably still use the specific getters so you don't rebuild on property changes that you don't care about. If - for some reason - you're interested in the entire MediaQueryData object, you would probably just have to add a // ignore:
.
@goderbauer yes makes sense would likely also simplify the lint implementation
It sounds like this would not be a particularly high value lint rule. Any lint rule based on following assignments and field accesses is very error-prone, must involve heuristics, and will have a high false positive rate.
I think it would actually be a very valuable lint check. (On LinkedIn, people assumed there will eventually be a lint or auto-fix for this useful feature.)
The performance implications are important enough that the Flutter team decided is was worth implementing the new xOf()
methods and recommending them in the first paragraphs of the MediaQuery class documentation.
As described in those links, MediaQuery is an object at the root of nearly every Flutter app with many fields that may change, but most consumers only care about one or a few fields.
For simplicity of implementation, maybe this could be an optional lint check that triggers any time MediaQuery.of(context)
is used. Then the author can either use the recommended methods OR explicitly ignore the lint. (as @goderbauer suggested)
That seems like it should be simple to implement and would allow authors to opt-in to reminders if they expect to usually use the recommended methods.
@srawlins I've changed the Good examples never to have MediaQuery.of(context)
be a good example. Does that change your analysis of the reliability of the lint?
I'm not too deep into how the system works under the hood to understand the concrete reason why this would be unreliable!
@IchordeDionysos Hmm are you saying this rule could ban all use of MediaQuery.of
and it would still be very useful? Like it is super rare that anyone should ever call that constructor?
@srawlins yes, as @goderbauer said:
In general, using the more specific methods is the better choice as it will avoid unnecessary rebuilds.
When you want to use the more specific methods for performance gains, always using them likely causes no harm!
Instead of doing this:
final EdgeInsets padding = MediaQuery.of(context).padding;
final double topPadding = padding.top + kToolbarHeight * 2;
final double bottomPadding = padding.bottom + kBottomNavigationBarHeight;
final Size size = MediaQuery.of(context).size;
final bool isNarrow = size.width < App.phoneWidthBreakpoint;
final double sideMargin = isNarrow ? 8 : App.edgeInsetsTablet;
it can be rewritten to:
final EdgeInsets padding = MediaQuery.paddingOf(context);
final double topPadding = padding.top + kToolbarHeight * 2;
final double bottomPadding = padding.bottom + kBottomNavigationBarHeight;
final Size size = MediaQuery.sizeOf(context);
final bool isNarrow = size.width < App.phoneWidthBreakpoint;
final double sideMargin = isNarrow ? 8 : App.edgeInsetsTablet;
There are arguably legitimate use-case for using MediaQuery.of(context)
, e.g.:
MediaQuery(
data: MediaQuery.of(context).copyWith(textScaler: TextScaler.noScaling),
child: child,
);
But those would just be ignored using // ignore: prefer_dedicated_media_query_functions
I'd argue this would be the only reason why calling MediaQuery.of(context)
can be justified!
In all other use cases, I've found with a quick search, using the dedicated functions is more performant with little impact on readability.
In that case, there can be a lint rule just banning MediaQuery.of. That's pretty trivial to write and maintain.
@goderbauer do you think such a rule could elevate to be a flutter recommended rule?
In flutter/flutter
all remaining uses should probably be transitioned to the following scheme
class TransformedMediaQuery extends StatelessWidget {
final Widget child;
final MediaQueryData Function(BuildContext context, MediaQueryData) transformation;
const TransformedMediaQuery({
required this.child,
required this.transformation,
Key? key
}) : super(key: key);
@override
Widget build(BuildContext context) {
return MediaQuery(
data: transformation(context, MediaQuery.of(context)),
child: child
);
}
}
And we only use MediaQuery.of(context)
from now on in this transformer widget. Note it takes context here as I integrate it with my own app settings. But in framework, the transformation could just act on MediaQueryData
alone.
prefer_dedicated_media_query_functions
Description
Avoid using
MediaQuery.of(context)
andMediaQuery.maybeOf(context)
.Details
Prefer using
MediaQuery.sizeOf(context)
and other dedicated of()-functions overMediaQuery.of(context)
.Kind
Does this enforce style advice? Guard against errors? Other?
Bad Examples
Good Examples
Discussion
Using
MediaQuery.maybeSizeOf(context)
is more performant than always usingMediaQuery.of(context)
as this reduces the number of unnecessary rebuilds widgets need to do.~To be discussed whether using
MediaQuery.of(context)
should be allowed when accessing multiple properties of the MediaQueryData.~ As per @goderbauer's suggestion: For simplicity sake any usage ofMediaQuery.of(context)
should be disallowed with this lint. If someone wants to useMediaQuery.of(context)
they could simply ignore the lint.Such a lint would help to migrate to the new dedicated MediaQuery functions and make it easier to enforce using them to encourage best practices.
Discussion checklist