Fix the ThemeData.copyWith toggleButtonsTheme argument type by rxlabz · Pull Request #40994 · flutter/flutter

rxlabz · 2019-09-20T20:27:17Z

Simple fix for the toggleButtonsTheme type of the ThemeData.copyWith method.

Related Issues

…meData

fluttergithubbot · 2019-09-20T20:27:19Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

shihaohong · 2019-09-23T23:33:12Z

Thanks for the contribution! We should really be writing a test or two to prevent #40992 from occurring again. Would you be willing to take that on?

rxlabz · 2019-09-24T20:45:03Z

My pleasure !
Here a first test, a ThemeData.copyWith test passing all arguments and verifying if all values are correctly set in the returned instance : 6553ce4
Should I add more tests ?

shihaohong · 2019-09-25T06:59:02Z

packages/flutter/test/material/theme_data_test.dart

    expect(theme.applyElevationOverlayColor, isTrue);
  });

+  testWidgets('ThemeData.copyWith return appropriate value', (WidgetTester tester) async {


What I find interesting is that this tests for whether or not all the values are properly overwritten, but not whether or not the argument type is correct. A side effect of this test is that the previous broken behavior will not be allowed. We should write a test that more directly verifies whether or not the argument types are correct/expected.

(This test is also absolutely needed by the way... we should probably create a test in a separate PR that ensures that ThemeData.copyWith properly overwrites individual properties while retaining the original values of the other properties)

Thanks for the review.
I'm not sure about how to test the argument type correctness. Do you know where I can find an example of test of this kind please ?
I'm very surprised that analyzer can't detect the argument type error because of the fallback value.

// analyzer detects the type error List<int> getList(String arg) => arg ; // valid for the analyzer List<int> getList(String arg) => arg ?? [0];

Isn't it possible to change the analyzer configuration to avoid this behavior ?

That is indeed strange, I hadn't considered that the analyzer should've caught this problem to begin with. This might actually be a Dart analyzer bug... could you file an issue?

We could just move forward with the test that you've already created, since that would catch the issue that we ran into, but also catch regressions in copyWith's behavior. @HansMuller, what do you think?

The new test looks OK to me.

This sounds like either an analyzer issue or something subtle about Dart that we don't understand. In this case, neither return value is a List<int>:

List<int> getList(String arg) => arg ?? [0];

I think the [0] is a List<dynamic>, <int>[0] would be a List<int>.

Filing a Dart analyzer issue sounds like a good idea.

It's a known issue : dart-lang/sdk#25368, dart-lang/sdk#36964

For now disabling implicit-cast seems the only option but in near future implicit casts will be disabled to migrate to NNBD cf. dart-lang/sdk#36964 (comment)

shihaohong · 2019-09-26T02:40:36Z

packages/flutter/test/material/theme_data_test.dart

+      typography : darkTheme.typography ,
+      cupertinoOverrideTheme : darkTheme.cupertinoOverrideTheme ,
+      snackBarTheme : darkTheme.snackBarTheme ,
+      bottomSheetTheme : darkTheme.bottomSheetTheme ,


You're missing popupMenuTheme

packages/flutter/test/material/theme_data_test.dart

shihaohong · 2019-09-26T02:43:52Z

packages/flutter/test/material/theme_data_test.dart

+    expect(themeDataCopy.cupertinoOverrideTheme, equals(darkTheme.cupertinoOverrideTheme));
+    expect(themeDataCopy.snackBarTheme, equals(darkTheme.snackBarTheme));
+    expect(themeDataCopy.bottomSheetTheme, equals(darkTheme.bottomSheetTheme));
+    expect(themeDataCopy.popupMenuTheme, equals(darkTheme.popupMenuTheme));


This unexpectedly passed. I think you might, unfortunately, have to use custom ThemeData values instead of light vs dark theme, since some of these properties are identical in both themes. This means that the existing test will not properly catch some regressions

Right ! Now it's 2 distincts ThemeData.raw instances.

Co-Authored-By: Shi-Hao Hong <[email protected]>

shihaohong

Thank you for thoroughly customizing each copy of ThemeData, this is a significant improvement to our ThemeData.copyWith testing!

Some stylistic nits for now... it seems like some trivial pre-submit tests are failing, so let's wait until those pass before I do a complete check of every property.

packages/flutter/test/material/theme_data_test.dart

Co-Authored-By: Shi-Hao Hong <[email protected]>

shihaohong · 2019-09-27T10:09:29Z

Seems to still be failing the analyze test because of whitespace at the end of some of your lines. Make sure that this passes [and all failing tests after, if any]

▌06:59:13▐ ELAPSED TIME: 0.019s for grep --line-number --extended-regexp [[:blank:]]$ packages/flutter/lib/src/material/theme_data.dart packages/flutter/test/material/theme_data_test.dart in .
Whitespace detected at the end of source code lines.

rxlabz · 2019-09-27T16:24:10Z

I fixed the analyzer step. Now the 2 failings steps seems not related to my MR.

shihaohong

LGTM

packages/flutter/test/material/theme_data_test.dart

Co-Authored-By: Shi-Hao Hong <[email protected]>

rxlabz · 2019-09-30T15:41:00Z

All green !

shihaohong · 2019-09-30T15:52:49Z

@rxlabz Thank you for the contribution! I just merged :)

…40994) * set the copyWith toggleButtonsTheme argument type to ToggleButtonsThemeData * add a ThemeData.copyWith test

set the copyWith toggleButtonsTheme argument type to ToggleButtonsThe…

a0d4dc9

…meData

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 20, 2019

googlebot added the cla: yes label Sep 20, 2019

add a ThemeData.copyWith test

6553ce4

rxlabz changed the title ~~Fix the copyWith toggleButtonsTheme argument type~~ Fix the ThemeData.copyWith toggleButtonsTheme argument type Sep 24, 2019

shihaohong reviewed Sep 25, 2019

View reviewed changes

shihaohong reviewed Sep 26, 2019

View reviewed changes

rxlabz and others added 3 commits September 26, 2019 21:17

Update test description

fce3678

Co-Authored-By: Shi-Hao Hong <[email protected]>

code format

b4d75ed

Co-Authored-By: Shi-Hao Hong <[email protected]>

better test

b0825f7

shihaohong reviewed Sep 27, 2019

View reviewed changes

packages/flutter/test/material/theme_data_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/theme_data_test.dart Outdated Show resolved Hide resolved

rxlabz and others added 2 commits September 27, 2019 08:51

code format

94a8a3e

Co-Authored-By: Shi-Hao Hong <[email protected]>

code format

d49e23f

Co-Authored-By: Shi-Hao Hong <[email protected]>

fix white spaces

4c6feeb

shihaohong approved these changes Sep 29, 2019

View reviewed changes

packages/flutter/test/material/theme_data_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/theme_data_test.dart Outdated Show resolved Hide resolved

shihaohong added the waiting for tree to go green label Sep 29, 2019

rxlabz and others added 2 commits September 29, 2019 17:20

code format

75fbb52

Co-Authored-By: Shi-Hao Hong <[email protected]>

code format

a49293a

Co-Authored-By: Shi-Hao Hong <[email protected]>

shihaohong merged commit d122f09 into flutter:master Sep 30, 2019

rxlabz deleted the fix-copywith-togglebuttonstheme branch September 30, 2019 16:37

hanneskuettner mentioned this pull request Oct 7, 2019

ToggleButtonTheme? #42095

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Conversation

rxlabz commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Uh oh!

fluttergithubbot commented Sep 20, 2019

Uh oh!

shihaohong commented Sep 23, 2019

Uh oh!

rxlabz commented Sep 24, 2019

Uh oh!

shihaohong Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxlabz Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihaohong Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

HansMuller Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxlabz Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

shihaohong Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shihaohong Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxlabz Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shihaohong commented Sep 27, 2019

Uh oh!

rxlabz commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rxlabz commented Sep 30, 2019

Uh oh!

shihaohong commented Sep 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rxlabz commented Sep 20, 2019 •

edited

Loading

shihaohong Sep 25, 2019 •

edited

Loading

rxlabz Sep 25, 2019 •

edited

Loading

HansMuller Sep 25, 2019 •

edited

Loading

shihaohong Sep 26, 2019 •

edited

Loading

rxlabz commented Sep 27, 2019 •

edited

Loading