flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.32k stars 27.53k forks source link

Proposal : MaterialApp - Make MediaQuery be available while constructing app Theme #80931

Open zs-dima opened 3 years ago

zs-dima commented 3 years ago

MaterialApp theme builder have no possibility to use MediaQuery - so it could not be used in real-life apps without ugly workarounds. App themes depends on current screen width / system theme type and etc. I found workarounds that have own issues:

1.

MaterialApp(
        home: child,
        builder: (context, child) => AnimatedTheme(
          data: appTheme(context),
          child: child!,

but it duplicate AnimatedTheme(data: ThemeData.light() creation in the MaterialApp

2.

MediaQuery(
  data: MediaQueryData.fromWindow(window),
  child: MaterialApp(
        home: child,
        theme: appTheme(context),

but it duplicate MediaQuery creation (or alternative widget as sample above has no MediaQuery changes notifications for the theme) in the MaterialApp Is there any simple way to disable duplicate AnimatedTheme creation for 1.? Could it be MaterialApp parameter or etc?

darshankawar commented 3 years ago

@zs-dima If I understand your issue correctly, you are unable to use MediaQuery.of(context) for Themes ?

zs-dima commented 3 years ago

@darshankawar thanks for reply

Yes, it is not possible to set theme fonts sizes depends on MediaQuery.of(context).size.width and etc without workarounds I mention above. But this workarounds have issues I mention. Looks MaterialApp could be improved to be able to work with themes correctly. Maybe add parameter to do not wrap child by additional AnimatedTheme internally.

darshankawar commented 3 years ago

@zs-dima Can you check out this SO link and see if it helps to answer your query / issue ?

zs-dima commented 3 years ago

@darshankawar sure it is not. I looked into MaterialApp source code and could see it force wrap child in the unneeded AnimatedTheme(data: ThemeData.light() Simple MaterialApp fix could be made child wrapping optional.

zs-dima commented 3 years ago

So MaterialApp force us to set theme as MaterialApp(theme: appTheme(context), but on other hand it create MediaQuery below theme by mistake - so we could not create theme based on actual device screen size and etc.

darshankawar commented 3 years ago

@zs-dima Thanks for digging in. What's your ask / proposal here to fix this limitation / behavior ?

zs-dima commented 3 years ago

My proposal to make MediaQuery be available while we constructing app Theme. So we could construct app Theme depends on device details and system settings MediaQueryData contains. And we could update Theme when device details changes (MediaQuery.of() behave in this way already)

Simplest solution from my point of view is to make optional wrapping MaterialApp child with AnimatedTheme internally. Then we will be able to use MaterialApp(builder: (context, child) => AnimatedTheme( without additional wrapping.

Other solution could be to move MediaQuery over theme construction - then it will be MaterialApp(themeBuilder: (context) => appTheme(context)

zs-dima commented 3 years ago

@darshankawar thanks a lot for looking into the issue details and trying to solve it.

AbhishekDoshi26 commented 3 years ago

I guess using MediaQuery.of(context) won't be possible in MaterialApp. This is because of(context) takes the context of parent widget. However MaterialApp is the root of widget tree and hence there is no Parent of it. I may be wrong in interpreting the behaviour of MaterialApp and of(context). What do you think @darshankawar ?

zs-dima commented 3 years ago

@AbhishekDoshi26 therefore I suggesting to use themeBuilder or be able to disable force-wrap AnimatedTheme above.

zs-dima commented 3 years ago

root of widget tree

I mean MaterialApp internal sub-tree - it could be constructed in better way Currently it is MaterialApp>AnimatedTheme>MediaQuery But it could be MaterialApp>MediaQuery>AnimatedTheme at least

zs-dima commented 3 years ago

@darshankawar thanks a lot for looking into this issue

as I could not see changes are not in dev still

It going to be MediaQuery>MaterialApp>AnimatedTheme

however it could be nice to have MaterialApp>MediaQuery>AnimatedTheme ideally

pedromassango commented 3 years ago

Reopening as the PR has been reverted (see https://github.com/flutter/flutter/pull/85223)