-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Initialize ThemeData.visualDensity using ThemeData.platform instead of defaultTargetPlatform
#124357
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
Initialize ThemeData.visualDensity using ThemeData.platform instead of defaultTargetPlatform
#124357
Conversation
…f defaultTargetPlatform
40912bd to
5f6b338
Compare
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [defaultDensityForPlatform] which returns a [VisualDensity] that is adaptive |
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.
Maybe add a note here that this looks at the true platform and not what is given from ambient ThemeData. The "see also" implies that, but it may be good to explicitly state it.
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.
The first sentence explicitly says it is based on defaultTargetPlatform. There's also no context involved here, so it's impossible for it to depend on the ambient ThemeData, so I'm concerned that if I add a reference to something that requires a context that people will get confused as to why I'm referencing it here (and they might wonder where a context comes into it).
I added a second reference to defaultTargetPlatform, perhaps that helps make it clearer?
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.
What I was referring to was confusion that started my whole thread. When I first saw that the buttons/ThemeData default to adaptivePlatformDensity, I thought that it would come from the platform specified by ambient ThemeData. When digging into it I realized it does not. This fix still does not make it come from an ambient theme data as we discussed because it can't. However, now ThemeData does not default to this, and it (and by proxy, buttons) will respect the density of the platform from the data.
adaptivePlatformDensity is no longer used as the default, but still exists, and still uses the defaultTargetPlatform to return a density. The doc comment above does explicitly state that, but without knowing what defaultTargetPlatform is, others could believe (like I did originally) that it respects a platform set by the theme. I think explicitly saying something like this instead would make it crystal clear:
/// Returns a [VisualDensity] that is adaptive based on the
/// [defaultTargetPlatform]. **This is the platform on which
/// the framework is currently executing**.
This takes straight from the defaultTargetPlatform docs saying:
"The TargetPlatform that matches the platform on which the framework is currently executing."
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.
Okay, I added something similar, and also a "See also" for defaultTargetPlatform.
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.
Perfect. I'm trying to save some headache in understanding for people who think like me 😄
HansMuller
left a comment
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.
LGTM
…m` instead of `defaultTargetPlatform` (flutter/flutter#124357)
…ad of `defaultTargetPlatform` (flutter#124357) Initialize `ThemeData.visualDensity` using `ThemeData.platform` instead of `defaultTargetPlatform`
…m` instead of `defaultTargetPlatform` (flutter/flutter#124357)
…m` instead of `defaultTargetPlatform` (flutter/flutter#124357)
Description
This changes the initialization of
ThemeData.visualDensityto use theThemeData'splatforminstead of always usingdefaultTargetPlatform. Before this change, setting the platform on theThemeDatahad an effect on all the platform-dependent properties except forvisualDensity.Related Issues
VisualDensity.adaptivePlatformDensityshould be based onThemeData.platforminstead ofdefaultTargetPlatform#123773Tests
defaultDensityForPlatformand verified that the theme data uses the passed-in platform.