-
Notifications
You must be signed in to change notification settings - Fork 30.1k
Description
Consider improving efficiency for M2 and M3 ListTile defaults
In master channel (3.7.0-13.0.pre.80) we have the following default themes for ListTile in M2 and M3.
class _LisTileDefaultsM2 extends ListTileThemeData {
_LisTileDefaultsM2(this.context, ListTileStyle style)
: _themeData = Theme.of(context),
_textTheme = Theme.of(context).textTheme,
super(
shape: const Border(),
style: style,
);
final BuildContext context;
final ThemeData _themeData;
final TextTheme _textTheme;
@override
Color? get tileColor => Colors.transparent;
@override
TextStyle? get titleTextStyle {
switch (style!) {
case ListTileStyle.drawer:
return _textTheme.bodyLarge;
case ListTileStyle.list:
return _textTheme.titleMedium;
}
}
@override
TextStyle? get subtitleTextStyle => _textTheme.bodyMedium;
@override
TextStyle? get leadingAndTrailingTextStyle => _textTheme.bodyMedium;
@override
Color? get selectedColor => _themeData.colorScheme.primary;
@override
Color? get iconColor {
switch (_themeData.brightness) {
case Brightness.light:
// For the sake of backwards compatibility, the default for unselected
// tiles is Colors.black45 rather than colorScheme.onSurface.withAlpha(0x73).
return Colors.black45;
case Brightness.dark:
return null; // null, Use current icon theme color
}
}
}
class _LisTileDefaultsM3 extends ListTileThemeData {
const _LisTileDefaultsM3(this.context)
: super(shape: const RoundedRectangleBorder());
final BuildContext context;
@override
Color? get tileColor => Theme.of(context).colorScheme.surface;
@override
TextStyle? get titleTextStyle => Theme.of(context).textTheme.bodyLarge;
@override
TextStyle? get subtitleTextStyle => Theme.of(context).textTheme.bodyMedium;
@override
TextStyle? get leadingAndTrailingTextStyle => Theme.of(context).textTheme.labelSmall;
@override
Color? get selectedColor => Theme.of(context).colorScheme.primary;
@override
Color? get iconColor => Theme.of(context).colorScheme.onSurface;
}
Is there any reason why the M3 generated theme default could not do what the M2 handwritten theme does? Meaning, look up the theme of context just once instead of 6 times?
Also M2 theme could do it just once too, and reuse the already traversed inherited theme for Theme.of(context) for the _textTheme it needs as well.
These are of course small optimizations, but if used frequently in the framework for defaults for all widgets, they may add up. Especially since widgets often use their default values.
Maybe consider Theme.of(context) usage review for defaults?
Maybe consider reviewing this for all generated M3 themes and M2 themes that have been rewritten to extend the default null theme, instead of doing it inline in the widget code. This has been observed in other widget defaults as well.
DPT BTW: The rewrite of M2 default value to also extend the all null sub-theme data defaults with none null values is really nice. It makes it much easier to find the default values. Thanks for doing this as part of the M3 implementations as well.
Challenge with current M3 tileColor default
The new M3 spec default background (Theme.of(context).colorScheme.surface) for ListTile is a poor choice. Past M2 transparent color is a better choice. Consider using transparent also in M3 as default.
Rationale:
With the new M3 default, the developer has to determine what elevation a surface has (Card, Popup, Dialog, Drawer, etc) and set the ListTile elevation to same value to get it to fit color wise on the used background, so it gets same elevation tint as the surface it is on.
Or alternatively always create a sub-theme and there define the tileColor color to be transparent again, or set it to transparent on widget level. All three options work, but are extra work. If not done, the ListTile will not match the background color in many use cases.
Considering that ListTile is often used on elevated surfaces like Card, Drawer and PopupMenuButton, that get tint elevation in M3, and one typically want the ListTile to fit seamlessly on such backgrounds. This no longer works out of the box with current defaults and it seems a bit counter productive.
This also creates a design break when migrating an existing app from M2 to M3, they will not fit on backgrounds where they worked previously in M2. Sure, it can easily be fixed by defining a custom sub-theme for ListTile that sets the color back to Colors.transparent that it used by default before. But I'm sure many will stumble on this and wonder about why it has to be this way. Consider reviewing with the Material team if Colors.transparent would still not be a better default for ListTile.tileColor in M3, at least for the Flutter implementation. Maybe in others too.
Flutter doctor
flutter doctor -v
[!] Flutter (Channel master, 3.7.0-13.0.pre.80, on macOS 13.0.1 22A400 darwin-arm64, locale en-US)
• Flutter version 3.7.0-13.0.pre.80 on channel master at /Users/rydmike/fvm/versions/master
• Upstream repository https://github.com/flutter/flutter.git
• Framework revision bf2701d2a8 (7 hours ago), 2022-12-27 09:54:34 -0500
• Engine revision 9d69a91bba
• Dart version 3.0.0 (build 3.0.0-65.0.dev)
• DevTools version 2.20.0
• If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform
update checks and upgrades.
[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0)
• Android SDK at /Users/rydmike/Library/Android/sdk
• Platform android-33, build-tools 33.0.0
• Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
• Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)
• All Android licenses accepted.
[✓] Xcode - develop for iOS and macOS (Xcode 14.2)
• Xcode at /Applications/Xcode.app/Contents/Developer
• Build 14C18
• CocoaPods version 1.11.3
[✓] Chrome - develop for the web
• Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome
[✓] Android Studio (version 2021.3)
• Android Studio at /Applications/Android Studio.app/Contents
• Flutter plugin can be installed from:
🔨 https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
🔨 https://plugins.jetbrains.com/plugin/6351-dart
• Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)
[✓] IntelliJ IDEA Community Edition (version 2022.3.1)
• IntelliJ at /Applications/IntelliJ IDEA CE.app
• Flutter plugin version 71.2.6
• Dart plugin version 223.8214.16
[✓] VS Code (version 1.73.1)
• VS Code at /Applications/Visual Studio Code.app/Contents
• Flutter extension version 3.54.0
[✓] Connected device (2 available)
• macOS (desktop) • macos • darwin-arm64 • macOS 13.0.1 22A400 darwin-arm64
• Chrome (web) • chrome • web-javascript • Google Chrome 108.0.5359.124
[✓] HTTP Host Availability
• All required HTTP hosts are available
Metadata
Metadata
Assignees
Labels
Type
Projects
Status