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.

Sign up for GitHub

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

Jump to bottom

Remove redundant useMaterial3: true #163376

Merged
merged 16 commits into from
Mar 14, 2025
Merged

Remove redundant useMaterial3: true #163376

merged 16 commits into from
Mar 14, 2025
+464 −898

Conversation

2shrestha22
Copy link
Contributor

This PR removes redundant useMaterial3: true as described in #162818

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. a: internationalization Supporting other languages or locales. (aka i18n) f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Feb 15, 2025
@piedcipher
Copy link
Member

Linux Analyze is failing due to const constructor lint.

@2shrestha22
Copy link
Contributor Author

Linux Analyze is failing due to const constructor lint.

what should we do?

@piedcipher
Copy link
Member

Linux Analyze is failing due to const constructor lint.

what should we do?

Wherever const lint is missing, you may add it.

@piedcipher
Copy link
Member

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8722868083803840785/+/u/run_test.dart_for_analyze_shard_and_subshard_None/stdout?format=raw

You can search "error #" to look for failures.

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #163376 at sha 599f51b

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Feb 16, 2025
@2shrestha22
Copy link
Contributor Author

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #163376 at sha 599f51b

I don't understand this. Can anyone help?

@justinmc
Copy link
Contributor

FYI @QuncCccccc

@justinmc
Copy link
Contributor

This may overlap with: #162862

@justinmc justinmc mentioned this pull request Feb 20, 2025
Open
9 tasks
@QuncCccccc QuncCccccc self-requested a review February 20, 2025 23:17
QuncCccccc
QuncCccccc reviewed Feb 24, 2025
View reviewed changes
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks so much for helping the migration!

Overall this looks great. I haven't reviewed all of cases. But could you help do the following update:

  • ThemeData.light() is the same as ThemeData(), we can use ThemeData() to replace it.
  • ThemeData.light().copyWith(xxx: yyy) should also be the same as the ThemeData(xxx: yyy) which can be used to keep tests looking simpler:)

packages/flutter/test/material/about_test.dart Outdated
theme: ThemeData.light(
useMaterial3: true,
).copyWith(scaffoldBackgroundColor: scaffoldColor, cardColor: cardColor),
theme: ThemeData.light().copyWith(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light().copyWith(
theme: ThemeData().copyWith(

packages/flutter/test/material/action_icons_theme_test.dart Outdated
@@ -115,7 +115,7 @@ void main() {

await tester.pumpWidget(
MaterialApp(
theme: ThemeData.light(useMaterial3: true).copyWith(actionIconTheme: actionIconTheme),
theme: ThemeData.light().copyWith(actionIconTheme: actionIconTheme),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this can be simpler?

Suggested change
theme: ThemeData.light().copyWith(actionIconTheme: actionIconTheme),
theme: ThemeData(actionIconTheme: actionIconTheme),

packages/flutter/test/material/app_bar_test.dart Outdated
@@ -1523,7 +1515,7 @@ void main() {
await tester.pumpWidget(
MaterialApp(
key: GlobalKey(),
theme: ThemeData.light().copyWith(useMaterial3: true, appBarTheme: const AppBarTheme()),
theme: ThemeData.light().copyWith(appBarTheme: const AppBarTheme()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light().copyWith(appBarTheme: const AppBarTheme()),
theme: ThemeData(appBarTheme: const AppBarTheme()),

packages/flutter/test/material/badge_test.dart Outdated
@@ -12,7 +12,7 @@ void main() {

await tester.pumpWidget(
MaterialApp(
theme: ThemeData.light(useMaterial3: true),
theme: ThemeData.light(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light(),

Copy link
Contributor

Choose a reason for hiding this comment

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

I think by default, MaterialApp provides a light theme data:)

packages/flutter/test/material/badge_test.dart Outdated
@@ -53,7 +53,7 @@ void main() {

await tester.pumpWidget(
MaterialApp(
theme: ThemeData.light(useMaterial3: true),
theme: ThemeData.light(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light(),

packages/flutter/test/material/badge_test.dart Outdated
@@ -173,7 +173,7 @@ void main() {
});

testWidgets('Small Badge RTL defaults', (WidgetTester tester) async {
final ThemeData theme = ThemeData.light(useMaterial3: true);
final ThemeData theme = ThemeData.light();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final ThemeData theme = ThemeData.light();
final ThemeData theme = ThemeData();

packages/flutter/test/material/badge_test.dart Outdated
@@ -207,7 +207,7 @@ void main() {
});

testWidgets('Large Badge textStyle and colors', (WidgetTester tester) async {
final ThemeData theme = ThemeData.light(useMaterial3: true);
final ThemeData theme = ThemeData.light();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final ThemeData theme = ThemeData.light();
final ThemeData theme = ThemeData();

packages/flutter/test/material/badge_test.dart Outdated
@@ -236,7 +236,7 @@ void main() {
testWidgets('isLabelVisible', (WidgetTester tester) async {
await tester.pumpWidget(
MaterialApp(
theme: ThemeData.light(useMaterial3: true),
theme: ThemeData.light(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light(),

packages/flutter/test/material/badge_test.dart Outdated
@@ -258,7 +258,7 @@ void main() {

Widget buildFrame(Alignment alignment, [Offset offset = Offset.zero]) {
return MaterialApp(
theme: ThemeData.light(useMaterial3: true),
theme: ThemeData.light(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light(),

packages/flutter/test/material/badge_test.dart Outdated
@@ -356,7 +356,7 @@ void main() {

Widget buildFrame(Alignment alignment, [Offset offset = Offset.zero]) {
return MaterialApp(
theme: ThemeData.light(useMaterial3: true),
theme: ThemeData.light(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theme: ThemeData.light(),

@github-actions github-actions bot added a: animation Animation APIs a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels Feb 25, 2025
@2shrestha22
Copy link
Contributor Author

Hi @QuncCccccc I have made the changes you suggested. Thank you.

@dkwingsmt dkwingsmt requested a review from QuncCccccc March 12, 2025 18:39
QuncCccccc
QuncCccccc approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

Thanks a lot for helping remove this! Just 2 more nits then we are good to go!

LGTM:)

QuncCccccc
QuncCccccc reviewed Mar 13, 2025
View reviewed changes
packages/flutter/test/material/navigation_rail_theme_test.dart Outdated
@@ -58,7 +58,7 @@ void main() {
// This test can be removed when `useMaterial3` is deprecated.
await tester.pumpWidget(
MaterialApp(
theme: ThemeData.light().copyWith(useMaterial3: false),
theme: ThemeData(useMaterial3: false),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the failure is because of the copyWith cannot properly handle useMaterial3, see here. Let's just revert this part because in this PR we only wanted to handle cases for useMaterial3: true

justinmc
justinmc approved these changes Mar 14, 2025
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Thanks for doing all of this painstaking work to migrate the code!

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 14, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Mar 14, 2025
Merged via the queue into flutter:master with commit d261411 Mar 14, 2025
170 of 171 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 14, 2025
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 15, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
a492a8a
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 15, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
da871ae
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
48f20d2
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
92327d8
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
2f026b4
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 17, 2025
Open
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 17, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
9f50abb
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 20, 2025
Open
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 20, 2025
@engine-flutter-autoroll
d261411b4 Remove redundant useMaterial3: true (flutter/flutter#163376)
c0f75ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piedcipher piedcipher piedcipher left review comments

@QuncCccccc QuncCccccc QuncCccccc approved these changes

@justinmc justinmc justinmc approved these changes

@matanlurey matanlurey Awaiting requested review from matanlurey matanlurey is a code owner

Assignees

@2shrestha22 2shrestha22

Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: animation Animation APIs a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@2shrestha22 @piedcipher @justinmc @QuncCccccc