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

ThemeData.copyWith() doesn't update dependent themes #22913

Open
HansMuller opened this issue Oct 10, 2018 · 6 comments
Open

ThemeData.copyWith() doesn't update dependent themes #22913

HansMuller opened this issue Oct 10, 2018 · 6 comments
Labels
f: material design flutter/packages/flutter/material repository. found in release: 3.3 Found to occur in 3.3 found in release: 3.6 Found to occur in 3.6 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on P2 Important issues not at the top of the work list team-design Owned by Design Languages team triaged-design Triaged by Design Languages team

Comments

@HansMuller
Copy link
Contributor

Summary: Adding colors to ButtonThemeData (#22013) highlighted a general problem with ThemeData.copyWith: subordinate themes (like ThemeData.buttonTheme) aren't updated when a ThemeData property they depend on is changed.

ThemeData.copyWith has simple semantics: it updates the specified fields, but not dependent fields. For example there are 10 subordinate themes, each with its own set of ThemeData dependencies:

  colorScheme
  buttonTheme
  sliderTheme
  tabBarTheme
  chipTheme
  inputDecorationTheme
  pageTransitionsTheme
  textTheme
  primaryTextTheme
  accentTextTheme

None of the subordinate themes are changed if (say) a color they depend on is changed with ThemeData.copyWith(). For example:
myThemeData.copyWith(buttonColor: myButtonColor) doesn't change myThemeData.buttonTheme.

We could add a ThemeData.apply method, like TextTheme.apply but more complicated. The basic idea would be to update all of the subordinate themes that depend on ThemeData properties like primaryColor and accentColor.

To implement ThemeData.apply, we'd have to implement colorScheme.apply, buttonTheme.apply etc.

Documenting ThemeData.apply would be complicated itself because each subordinate theme has its own set of ThemeData dependencies.

That said, in the applications I've looked at - usually - where developers use ThemeData.copyWith they're usually careful to rebuild the subordinate theme they're trying to effect. The ButtonTheme is an exception because it's new, and see #22711.

For the moment I'm inclined to leave things as they are. Some improvements in the docs (including some examples) would help.

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 10, 2018
@zoechi zoechi added this to the Goals milestone Oct 10, 2018
@zoechi zoechi mentioned this issue Feb 18, 2019
@johnsonmh
Copy link
Contributor

Seems like the problem stems from the fact that copyWith calls ThemeData.raw. This misses out on all of the clever logic that happens in the factory ThemeData constructor. Could copyWith delegate to the regular ThemeData factory constructor rather than ThemeData.raw?

@nuvoPoint
Copy link

We have same issue. We created a light and a dark theme that should preferably inherit shared properties from a base themedata object. But using copyWith on the base theme will not work, so the only workaround for now is to have redundant properties in both the light and the dark theme.

@Vadaski
Copy link
Contributor

Vadaski commented Jun 20, 2019

Same issue here. I think its
I think this is because the reason associated with buttonTheme in ThemeData is not correctly told when copyWith.

The constructor of ThemeData did this, so it works fine.

buttonTheme ??= ButtonThemeData(
      colorScheme: colorScheme,
      buttonColor: buttonColor,
      disabledColor: disabledColor,
      highlightColor: highlightColor,
      splashColor: splashColor,
      materialTapTargetSize: materialTapTargetSize,
    );

Can we solve this problem? Because this behavior was used in the code of this codalabs and caused the wrong effect.

// TODO: Build a Shrine Theme (103)
final ThemeData _kShrineTheme = _buildShrineTheme();

ThemeData _buildShrineTheme() {
  final ThemeData base = ThemeData.light();
  return base.copyWith(
    accentColor: kShrineBrown900,
    primaryColor: kShrinePink100,
    buttonColor: kShrinePink100,
    scaffoldBackgroundColor: kShrineBackgroundWhite,
    cardColor: kShrineBackgroundWhite,
    textSelectionColor: kShrinePink100,
    errorColor: kShrineErrorRed,
    // TODO: Add the text themes (103)
    // TODO: Add the icon themes (103)
    // TODO: Decorate the inputs (103)
  );
}

@Realank
Copy link

Realank commented Jun 20, 2019

I think copyWith method or buttonTheme should be refactored due to current mechanism could cause side effect

@kf6gpe kf6gpe added the P2 Important issues not at the top of the work list label May 29, 2020
@pedromassangocode
Copy link

I was able to reproduce in 1.21.0-1.0.pre.

The ThemeData.copyWith() method doesn't currently update dependents theme as we can se at:

ThemeData copyWith({
Brightness brightness,
VisualDensity visualDensity,
Color primaryColor,
Brightness primaryColorBrightness,
Color primaryColorLight,
Color primaryColorDark,
Color accentColor,
Brightness accentColorBrightness,
Color canvasColor,
Color shadowColor,
Color scaffoldBackgroundColor,
Color bottomAppBarColor,
Color cardColor,
Color dividerColor,
Color focusColor,
Color hoverColor,
Color highlightColor,
Color splashColor,
InteractiveInkFeatureFactory splashFactory,
Color selectedRowColor,
Color unselectedWidgetColor,
Color disabledColor,
ButtonThemeData buttonTheme,
ToggleButtonsThemeData toggleButtonsTheme,
Color buttonColor,
Color secondaryHeaderColor,
Color textSelectionColor,
Color cursorColor,
Color textSelectionHandleColor,
Color backgroundColor,
Color dialogBackgroundColor,
Color indicatorColor,
Color hintColor,
Color errorColor,
Color toggleableActiveColor,
TextTheme textTheme,
TextTheme primaryTextTheme,
TextTheme accentTextTheme,
InputDecorationTheme inputDecorationTheme,
IconThemeData iconTheme,
IconThemeData primaryIconTheme,
IconThemeData accentIconTheme,
SliderThemeData sliderTheme,
TabBarTheme tabBarTheme,
TooltipThemeData tooltipTheme,
CardTheme cardTheme,
ChipThemeData chipTheme,
TargetPlatform platform,
MaterialTapTargetSize materialTapTargetSize,
bool applyElevationOverlayColor,
PageTransitionsTheme pageTransitionsTheme,
AppBarTheme appBarTheme,
BottomAppBarTheme bottomAppBarTheme,
ColorScheme colorScheme,
DialogTheme dialogTheme,
FloatingActionButtonThemeData floatingActionButtonTheme,
NavigationRailThemeData navigationRailTheme,
Typography typography,
CupertinoThemeData cupertinoOverrideTheme,
SnackBarThemeData snackBarTheme,
BottomSheetThemeData bottomSheetTheme,
PopupMenuThemeData popupMenuTheme,
MaterialBannerThemeData bannerTheme,
DividerThemeData dividerTheme,
ButtonBarThemeData buttonBarTheme,
BottomNavigationBarThemeData bottomNavigationBarTheme,
TimePickerThemeData timePickerTheme,
TextButtonThemeData textButtonTheme,
ElevatedButtonThemeData elevatedButtonTheme,
OutlinedButtonThemeData outlinedButtonTheme,
bool fixTextFieldOutlineLabel,
}) {
cupertinoOverrideTheme = cupertinoOverrideTheme?.noDefault();
return ThemeData.raw(
visualDensity: visualDensity ?? this.visualDensity,
primaryColor: primaryColor ?? this.primaryColor,
primaryColorBrightness: primaryColorBrightness ?? this.primaryColorBrightness,
primaryColorLight: primaryColorLight ?? this.primaryColorLight,
primaryColorDark: primaryColorDark ?? this.primaryColorDark,
accentColor: accentColor ?? this.accentColor,
accentColorBrightness: accentColorBrightness ?? this.accentColorBrightness,
canvasColor: canvasColor ?? this.canvasColor,
shadowColor: shadowColor ?? this.shadowColor,
scaffoldBackgroundColor: scaffoldBackgroundColor ?? this.scaffoldBackgroundColor,
bottomAppBarColor: bottomAppBarColor ?? this.bottomAppBarColor,
cardColor: cardColor ?? this.cardColor,
dividerColor: dividerColor ?? this.dividerColor,
focusColor: focusColor ?? this.focusColor,
hoverColor: hoverColor ?? this.hoverColor,
highlightColor: highlightColor ?? this.highlightColor,
splashColor: splashColor ?? this.splashColor,
splashFactory: splashFactory ?? this.splashFactory,
selectedRowColor: selectedRowColor ?? this.selectedRowColor,
unselectedWidgetColor: unselectedWidgetColor ?? this.unselectedWidgetColor,
disabledColor: disabledColor ?? this.disabledColor,
buttonColor: buttonColor ?? this.buttonColor,
buttonTheme: buttonTheme ?? this.buttonTheme,
toggleButtonsTheme: toggleButtonsTheme ?? this.toggleButtonsTheme,
secondaryHeaderColor: secondaryHeaderColor ?? this.secondaryHeaderColor,
textSelectionColor: textSelectionColor ?? this.textSelectionColor,
cursorColor: cursorColor ?? this.cursorColor,
textSelectionHandleColor: textSelectionHandleColor ?? this.textSelectionHandleColor,
backgroundColor: backgroundColor ?? this.backgroundColor,
dialogBackgroundColor: dialogBackgroundColor ?? this.dialogBackgroundColor,
indicatorColor: indicatorColor ?? this.indicatorColor,
hintColor: hintColor ?? this.hintColor,
errorColor: errorColor ?? this.errorColor,
toggleableActiveColor: toggleableActiveColor ?? this.toggleableActiveColor,
textTheme: textTheme ?? this.textTheme,
primaryTextTheme: primaryTextTheme ?? this.primaryTextTheme,
accentTextTheme: accentTextTheme ?? this.accentTextTheme,
inputDecorationTheme: inputDecorationTheme ?? this.inputDecorationTheme,
iconTheme: iconTheme ?? this.iconTheme,
primaryIconTheme: primaryIconTheme ?? this.primaryIconTheme,
accentIconTheme: accentIconTheme ?? this.accentIconTheme,
sliderTheme: sliderTheme ?? this.sliderTheme,
tabBarTheme: tabBarTheme ?? this.tabBarTheme,
tooltipTheme: tooltipTheme ?? this.tooltipTheme,
cardTheme: cardTheme ?? this.cardTheme,
chipTheme: chipTheme ?? this.chipTheme,
platform: platform ?? this.platform,
materialTapTargetSize: materialTapTargetSize ?? this.materialTapTargetSize,
applyElevationOverlayColor: applyElevationOverlayColor ?? this.applyElevationOverlayColor,
pageTransitionsTheme: pageTransitionsTheme ?? this.pageTransitionsTheme,
appBarTheme: appBarTheme ?? this.appBarTheme,
bottomAppBarTheme: bottomAppBarTheme ?? this.bottomAppBarTheme,
colorScheme: (colorScheme ?? this.colorScheme).copyWith(brightness: brightness),
dialogTheme: dialogTheme ?? this.dialogTheme,
floatingActionButtonTheme: floatingActionButtonTheme ?? this.floatingActionButtonTheme,
navigationRailTheme: navigationRailTheme ?? this.navigationRailTheme,
typography: typography ?? this.typography,
cupertinoOverrideTheme: cupertinoOverrideTheme ?? this.cupertinoOverrideTheme,
snackBarTheme: snackBarTheme ?? this.snackBarTheme,
bottomSheetTheme: bottomSheetTheme ?? this.bottomSheetTheme,
popupMenuTheme: popupMenuTheme ?? this.popupMenuTheme,
bannerTheme: bannerTheme ?? this.bannerTheme,
dividerTheme: dividerTheme ?? this.dividerTheme,
buttonBarTheme: buttonBarTheme ?? this.buttonBarTheme,
bottomNavigationBarTheme: bottomNavigationBarTheme ?? this.bottomNavigationBarTheme,
timePickerTheme: timePickerTheme ?? this.timePickerTheme,
textButtonTheme: textButtonTheme ?? this.textButtonTheme,
elevatedButtonTheme: elevatedButtonTheme ?? this.elevatedButtonTheme,
outlinedButtonTheme: outlinedButtonTheme ?? this.outlinedButtonTheme,
fixTextFieldOutlineLabel: fixTextFieldOutlineLabel ?? this.fixTextFieldOutlineLabel,
);
}

flutter doctor -v
[✓] Flutter (Channel dev, 1.21.0-1.0.pre, on Mac OS X 10.15.5 19F101, locale en-AO)
    • Flutter version 1.21.0-1.0.pre at /Users/pedro/dev/SDKs/flutter_dev
    • Framework revision f25bd9c55c (5 days ago), 2020-07-14 20:26:01 -0400
    • Engine revision 99c2b3a245
    • Dart version 2.9.0 (build 2.9.0-21.0.dev 20bf2fcf56)

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.0)
    • Android SDK at /Users/pedro/Library/Android/sdk
    • Platform android-30, build-tools 30.0.0
    • Java binary at: /Users/pedro/Library/Application
      Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/193.6514223/Android
      Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.5)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 11.5, Build version 11E608c
    • CocoaPods version 1.9.3

[✓] Chrome - develop for the web
    • CHROME_EXECUTABLE = /Applications/Google Chrome.app/Contents/MacOS/google-chrome-unsafe

[✓] Android Studio (version 4.0)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin version 47.1.2
    • Dart plugin version 193.7361
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)

[✓] Android Studio (version 4.0)
    • Android Studio at /Users/pedro/Library/Application
      Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/193.6514223/Android Studio.app/Contents
    • Flutter plugin version 47.1.2
    • Dart plugin version 193.7361
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)

[✓] VS Code (version 1.47.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.12.1

[✓] Connected device (3 available)
    • macOS (desktop)  • macos      • darwin-x64     • Mac OS X 10.15.5 19F101
    • Web Server (web) • web-server • web-javascript • Flutter Tools
    • Chrome (web)     • chrome     • web-javascript • Google Chrome 83.0.4103.116

• No issues found!

@pedromassangocode pedromassangocode added found in release: 1.21 Found to occur in 1.21 has reproducible steps The issue has been confirmed reproducible and is ready to work on labels Jul 20, 2020
@Hixie Hixie removed this from the None. milestone Aug 17, 2020
@exaby73
Copy link
Member

exaby73 commented Nov 28, 2022

This issue still exists. Updating labels to reflect the same

@exaby73 exaby73 added found in release: 3.3 Found to occur in 3.3 found in release: 3.6 Found to occur in 3.6 and removed found in release: 1.21 Found to occur in 1.21 labels Nov 28, 2022
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-design Owned by Design Languages team triaged-design Triaged by Design Languages team labels Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. found in release: 3.3 Found to occur in 3.3 found in release: 3.6 Found to occur in 3.6 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on P2 Important issues not at the top of the work list team-design Owned by Design Languages team triaged-design Triaged by Design Languages team
Projects
None yet
Development

No branches or pull requests

9 participants