ThemeData issues with operator, copyWith and debugFillProperties

ThemeData Class has the following issues

1) Two properties are missing from `== operator` override

The properties:

focusColor
hoverColor

That exists as properties in ThemeData.raw and in hashCode are missing from operator. The lines:

        && other.focusColor== focusColor
        && other.hoverColor== hoverColor

should be added to operator.

The current operator implementation will cause issues. I had some, and it led me to investigate these overrides more closely, which is why I found this and the other issues in this report.

2) `copyWith` includes `brightness` parameter

The brightness is a getter in ThemeData that returns brightness from ThemeData.colorScheme. It can thus not be modified with a copyWith of ThemeData, nor does returned ThemeData.raw from the copyWith include any brightness value!

If somebody tries to modify theme brightness with this copyWith, they will not succeed and "might" be confused.

The brightness in the copyWith parameter list is probably a remnant from when brightness existed in ThemeData directly. It should however be removed in current implementation from copyWith as a parameter.

flutter/packages/flutter/lib/src/material/theme_data.dart

Line 1401 in f9c4b22

Brightness? brightness,

3) `debugFillProperties` has the following issues

The textSelectionTheme is added to it twice, as two consecutive rows:

flutter/packages/flutter/lib/src/material/theme_data.dart

Line 2028 in f9c4b22

    
           properties.add(DiagnosticsProperty<TextSelectionThemeData>('textSelectionTheme', textSelectionTheme, defaultValue: defaultData.textSelectionTheme, level: DiagnosticLevel.debug));

flutter/packages/flutter/lib/src/material/theme_data.dart

Line 2029 in f9c4b22

    
           properties.add(DiagnosticsProperty<TextSelectionThemeData>('textSelectionTheme', textSelectionTheme, defaultValue: defaultData.textSelectionTheme, level: DiagnosticLevel.debug));

The following ThemeData properties are missing from debugFillProperties:

visualDensity
primaryColorLight
primaryColorDark
splashFactory
fixTextFieldOutlineLabel
useTextSelectionTheme

For correct toString() and debug DevTools, functionality the extra entry of textSelectionTheme should be removed and the missing properties added.

The debugFillProperties also includes brightness, but since it is not a property of ThemeData it should be removed.

flutter/packages/flutter/lib/src/material/theme_data.dart

Line 1965 in f9c4b22

    
           properties.add(EnumProperty<Brightness>('brightness', brightness, defaultValue: defaultData.brightness, level: DiagnosticLevel.debug));

Observation: The order of the properties in debugFillProperties deviates significantly from the order of properties in operator, hashCode and ThemeData.raw it would be helpful if they were in the same order.

How to Reproduce

The above errors can be observed in the source code in all current versions from stable 2.5.2 and all channels up to master 2.6.0-12.0.pre.287.

Issue Visualized

The attached image table shows the issues. In order to easier compare and find what was going on with the
properties I copy-pasted the props in the image from ThemeData from master channel, into a table in order to compare and easier see what is going. I added it here as well, but the found issues are summarized above and fixes are straight forward.

In the table the debugFillProperites have been manually re-ordered into same order as the other methods properties, to be able to see the gaps more easily. I recommend putting them in the same order in the code as well.

Turns out that the parameters to copyWith and its return values, are not in the same order and neither of them agrees with the order used on ThemeData.raw, operator and hashCode. The copyWith params and return order that are off, in relation to them are marked in green. They are shown in the order they appeared in the code. They are all there though, so no issue, but again, same order would be nice.

@rydmike

Hi @rydmike, Thanks for the detailed issue. All three issues with the ThemeData class can be reproduced on stable 2.5.2 and the master 2.6.0-12.0.pre.287 channel.

Summing up the source code links from master

focusColor and hoverColor should be exactly below dividerColor in order
Brightness cannot be set using copywith constructor
duplicate debugFillProperties

flutter doctor -v

[✓] Flutter (Channel master, 2.6.0-12.0.pre.287, on macOS 11.6 20G165 darwin-arm, locale en-GB)
    • Flutter version 2.6.0-12.0.pre.287 at /Users/mahesh/Documents/flutter_master
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision f9c4b22721 (2 days ago), 2021-10-09 07:38:02 -0400
    • Engine revision 07ba8fd075
    • Dart version 2.15.0 (build 2.15.0-178.0.dev)

[✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0)
    • Android SDK at /Users/mahesh/Library/Android/sdk
    • Platform android-31, build-tools 31.0.0
    • ANDROID_HOME = /Users/mahesh/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.8+10-b944.6916264)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.0)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • CocoaPods version 1.10.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.2)
    • 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.8+10-b944.6916264)

[✓] IntelliJ IDEA Community Edition (version 2021.2.1)
    • IntelliJ at /Applications/IntelliJ IDEA CE.app
    • Flutter plugin version 60.1.4
    • Dart plugin version 212.5080.8

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

[✓] Connected device (3 available)
    • sdk gphone arm64 (mobile) • emulator-5554 • android-arm64  • Android 11 (API 30) (emulator)
    • macOS (desktop)           • macos         • darwin-arm64   • macOS 11.6 20G165 darwin-arm
    • Chrome (web)              • chrome        • web-javascript • Google Chrome 94.0.4606.71

• No issues found!

[✓] Flutter (Channel stable, 2.5.2, on macOS 11.6 20G165 darwin-arm, locale en-GB)
    • Flutter version 2.5.2 at /Users/mahesh/Documents/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 3595343e20 (11 days ago), 2021-09-30 12:58:18 -0700
    • Engine revision 6ac856380f
    • Dart version 2.14.3

[✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0)
    • Android SDK at /Users/mahesh/Library/Android/sdk
    • Platform android-31, build-tools 31.0.0
    • ANDROID_HOME = /Users/mahesh/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.8+10-b944.6916264)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 13.0, Build version 13A233
    • CocoaPods version 1.10.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.2)
    • 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.8+10-b944.6916264)

[✓] IntelliJ IDEA Community Edition (version 2021.2.1)
    • IntelliJ at /Applications/IntelliJ IDEA CE.app
    • Flutter plugin version 60.1.4
    • Dart plugin version 212.5080.8

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

[✓] Connected device (3 available)
    • sdk gphone arm64 (mobile) • emulator-5554 • android-arm64  • Android 11 (API 30) (emulator)
    • macOS (desktop)           • macos         • darwin-arm64   • macOS 11.6 20G165 darwin-arm
    • Chrome (web)              • chrome        • web-javascript • Google Chrome 94.0.4606.71

• No issues found!

Please note, hashCode also needs focusColor and hoverColor

@rydmike

@rydmike thanks for the detailed report. I have just submitted a PR that should resolve most of these issues if approved. The one I didn't address was the removal of brightness from copyWith. I understand what you are getting at, and it is a little odd. However it is still functionally equivalent to what was there when brightness was its own property.

The main reason it is still there however, is that we can't remove it immediately without potentially breaking existing apps that use it. We could deprecate it so that we would be able to remove it in the future. We will consider this going forward.

I also left the diagnostic properties in their current order. I am thinking we may want to do a separate ordering pass on this and either make them all alphabetical or something like what @guidezpl is proposing in #91497.

@darrenaustin

Hi @darrenaustin totally agree on your comment on the brightness and its copyWith. I missed the "deep" copyWith done for it on colorScheme. I noticed it the other day too and was about to add a comment that it should indeed be there. Good catch!

Normally one would not expect to find that in data class like this, but I totally get why it is there. It used to be a direct property in ThemeData. Now I see how nicely it was moved into colorScheme with the getter and the deep copy, even looked up its PR: #56956 last year in May, so not so super long ago. And sure removing that would break a lot of stuff, no doubt. Have to remember this neat setup to keep API intact myself too if I need it.

Thanks for adding the missing props to the == operator, when it lands that should fix my failing tests that I had to skip. The debugFillProperties are less critical, but nice to have all props there now too.

About the order, certainly not important for the functionality. Still I think when there are as many properties in class as in ThemeData that alphabetical order would make most sense, at least it makes it easier to check that you have not forgotten anything when you modify it. Which I guess is what happened here.

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

Navigation Menu

Repository navigation

ThemeData issues with `operator`, `copyWith` and `debugFillProperties` #91587

ThemeData Class has the following issues

1) Two properties are missing from `== operator` override

2) `copyWith` includes `brightness` parameter

3) `debugFillProperties` has the following issues

How to Reproduce

Issue Visualized

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ThemeData issues with operator, copyWith and debugFillProperties #91587

Description

ThemeData Class has the following issues

1) Two properties are missing from == operator override

2) copyWith includes brightness parameter

3) debugFillProperties has the following issues

How to Reproduce

Issue Visualized

Activity

maheshj01 commented on Oct 11, 2021

guidezpl commented on Oct 13, 2021

darrenaustin commented on Oct 13, 2021

rydmike commented on Oct 14, 2021

github-actions commented on Nov 5, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

ThemeData issues with `operator`, `copyWith` and `debugFillProperties` #91587

1) Two properties are missing from `== operator` override

2) `copyWith` includes `brightness` parameter

3) `debugFillProperties` has the following issues