Skip to content
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

proposal: prefer_dedicated_media_query_functions #4448

Open
5 tasks done
IchordeDionysos opened this issue Jun 9, 2023 · 10 comments
Open
5 tasks done

proposal: prefer_dedicated_media_query_functions #4448

IchordeDionysos opened this issue Jun 9, 2023 · 10 comments
Labels
lint-proposal P4 status-pending type-enhancement A request for a change that isn't a bug

Comments

@IchordeDionysos
Copy link

IchordeDionysos commented Jun 9, 2023

prefer_dedicated_media_query_functions

Description

Avoid using MediaQuery.of(context) and MediaQuery.maybeOf(context).

Details

Prefer using MediaQuery.sizeOf(context) and other dedicated of()-functions over MediaQuery.of(context).

Kind

Does this enforce style advice? Guard against errors? Other?

Bad Examples

Widget build(BuildContext context) {
  return Container(
    width: MediaQuery.of(context).size.width,
  );
}
Widget build(BuildContext context) {
  return Container(
    width: MediaQuery.maybeOf(context)?.size.width,
  );
}

Good Examples

Widget build(BuildContext context) {
  return Container(
    width: MediaQuery.sizeOf(context).width,
  );
}
Widget build(BuildContext context) {
  return Container(
    width: MediaQuery.maybeSizeOf(context)?.width,
  );
}

Discussion

Using MediaQuery.maybeSizeOf(context) is more performant than always using MediaQuery.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 of MediaQuery.of(context) should be disallowed with this lint.
If someone wants to use MediaQuery.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

  • List any existing rules this proposal modifies, complements, overlaps or conflicts with.
  • List any relevant issues (reported here, the SDK Tracker, or elsewhere).
  • If there's any prior art (e.g., in other linters), please add references here.
  • If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
  • If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.
@srawlins
Copy link
Member

srawlins commented Jun 9, 2023

This sounds like this might have something to do with Flutter. @goderbauer ?

@goderbauer
Copy link
Contributor

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:.

@IchordeDionysos
Copy link
Author

@goderbauer yes makes sense would likely also simplify the lint implementation

@srawlins
Copy link
Member

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.

@srawlins srawlins added the P4 label Sep 11, 2023
@KyleFin
Copy link

KyleFin commented Sep 11, 2023

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.

@IchordeDionysos
Copy link
Author

@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!

@srawlins
Copy link
Member

@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?

@IchordeDionysos
Copy link
Author

IchordeDionysos commented Sep 11, 2023

@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.

@srawlins
Copy link
Member

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?

@moffatman
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint-proposal P4 status-pending type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants