Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Apr 6, 2023

Description

This changes the initialization of ThemeData.visualDensity to use the ThemeData's platform instead of always using defaultTargetPlatform. Before this change, setting the platform on the ThemeData had an effect on all the platform-dependent properties except for visualDensity.

Related Issues

Tests

  • Added tests for the new static defaultDensityForPlatform and verified that the theme data uses the passed-in platform.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 6, 2023
@gspencergoog gspencergoog force-pushed the init_platform_visual_density branch from 40912bd to 5f6b338 Compare April 6, 2023 21:58
///
/// See also:
///
/// * [defaultDensityForPlatform] which returns a [VisualDensity] that is adaptive
Copy link

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.

Copy link
Contributor Author

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?

Copy link

@Calpoog Calpoog Apr 7, 2023

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

Copy link
Contributor Author

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.

Copy link

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 😄

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gspencergoog gspencergoog added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Apr 7, 2023
@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2023
@auto-submit auto-submit bot merged commit 692a7ef into flutter:master Apr 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2023
@gspencergoog gspencergoog deleted the init_platform_visual_density branch April 7, 2023 17:32
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
…ad of `defaultTargetPlatform` (flutter#124357)

Initialize `ThemeData.visualDensity` using `ThemeData.platform` instead of `defaultTargetPlatform`
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants