[Material] modernize Typography._withPlatform with Dart 3 switch expression#181398
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
framework
Enderjua
changed the title
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request modernizes the Typography._withPlatform method using Dart 3's switch expressions, improving code readability and conciseness. The change groups Android and Fuchsia platforms together using the logical OR pattern, simplifying the platform-specific theme assignments. The logic and existing fallbacks remain unchanged, with local tests passing. The code adheres to the Flutter style guide.
| final (TextTheme blackResolved, TextTheme whiteResolved) = switch (platform) { | ||
| TargetPlatform.iOS => (black ?? blackCupertino, white ?? whiteCupertino), | ||
| TargetPlatform.android || TargetPlatform.fuchsia => ( | ||
| black ?? blackMountainView, | ||
| white ?? whiteMountainView, | ||
| ), | ||
| TargetPlatform.windows => (black ?? blackRedmond, white ?? whiteRedmond), | ||
| TargetPlatform.macOS => (black ?? blackRedwoodCity, white ?? whiteRedwoodCity), | ||
| TargetPlatform.linux => (black ?? blackHelsinki, white ?? whiteHelsinki), | ||
| null => (black!, white!), | ||
| }; |
There was a problem hiding this comment.
The black! and white! non-null assertions are used here, which could lead to runtime errors if black or white are unexpectedly null. While the assert statement on line 217 mitigates this, consider adding a more descriptive error message or handling the null case more gracefully to improve the developer experience.
For example, you could throw an [AssertionError] with a helpful message if the assert fails.
null => (black ?? (throw AssertionError('black cannot be null')), white ?? (throw AssertionError('white cannot be null'))),
There was a problem hiding this comment.
The existing assertion at the beginning of the method guarantees this case is safe;
assert(platform != null || (black != null && white != null));
| assert(platform != null || (black != null && white != null)); | ||
| switch (platform) { | ||
| case TargetPlatform.iOS: | ||
| black ??= blackCupertino; | ||
| white ??= whiteCupertino; | ||
| case TargetPlatform.android: | ||
| case TargetPlatform.fuchsia: | ||
| black ??= blackMountainView; | ||
| white ??= whiteMountainView; | ||
| case TargetPlatform.windows: | ||
| black ??= blackRedmond; | ||
| white ??= whiteRedmond; | ||
| case TargetPlatform.macOS: | ||
| black ??= blackRedwoodCity; | ||
| white ??= whiteRedwoodCity; | ||
| case TargetPlatform.linux: | ||
| black ??= blackHelsinki; | ||
| white ??= whiteHelsinki; | ||
| case null: | ||
| break; | ||
| } | ||
| return Typography._(black!, white!, englishLike, dense, tall); | ||
| final (TextTheme blackResolved, TextTheme whiteResolved) = switch (platform) { | ||
| TargetPlatform.iOS => (black ?? blackCupertino, white ?? whiteCupertino), | ||
| TargetPlatform.android || TargetPlatform.fuchsia => ( | ||
| black ?? blackMountainView, | ||
| white ?? whiteMountainView, | ||
| ), | ||
| TargetPlatform.windows => (black ?? blackRedmond, white ?? whiteRedmond), | ||
| TargetPlatform.macOS => (black ?? blackRedwoodCity, white ?? whiteRedwoodCity), | ||
| TargetPlatform.linux => (black ?? blackHelsinki, white ?? whiteHelsinki), | ||
| null => (black!, white!), |
There was a problem hiding this comment.
This switch statement is replaced with a switch expression, which is a more concise way to handle this logic in Dart 3. The use of a record to return both blackResolved and whiteResolved makes the code more readable.
Consider adding a comment explaining the purpose of the record and the switch expression for better maintainability.
| TargetPlatform.android || TargetPlatform.fuchsia => ( | ||
| black ?? blackMountainView, | ||
| white ?? whiteMountainView, | ||
| ), |
There was a problem hiding this comment.
The logical OR pattern TargetPlatform.android || TargetPlatform.fuchsia is used to group these platforms together, which simplifies the code and makes it more readable. This is a good use of Dart 3's new features.
Enderjua
force-pushed
the
refactor-typography-switch
branch
from
a0dd3b7 to
70ed92b
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
LGTM, thanks for the change. Can you request a test exemption on the discord?
|
test-exempt: code refactor with no semantic change |
dkwingsmt
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
flutter/flutter@bfc9041...def9ca9 2026-01-25 [email protected] Roll Skia from f1433eb44a50 to 2830fbe8bafe (1 revision) (flutter/flutter#181464) 2026-01-25 [email protected] Roll Fuchsia Linux SDK from 6xoKGIry6Y8T8x5Sa... to T4qTEa3T5CCSCIoJY... (flutter/flutter#181458) 2026-01-24 [email protected] Roll Skia from b6d396a151bc to f1433eb44a50 (1 revision) (flutter/flutter#181449) 2026-01-24 [email protected] Roll Dart SDK from 29918a54dd5c to 60553fc4c04f (1 revision) (flutter/flutter#181437) 2026-01-24 [email protected] Roll Fuchsia Linux SDK from n7NohL9DPpEuPjNt9... to 6xoKGIry6Y8T8x5Sa... (flutter/flutter#181438) 2026-01-24 [email protected] [Impeller] Fix perspective clips with a large perspective bias (flutter/flutter#181434) 2026-01-24 [email protected] Roll Dart SDK from e82d7ad1855e to 29918a54dd5c (4 revisions) (flutter/flutter#181435) 2026-01-24 [email protected] Roll Skia from 32b52343e757 to b6d396a151bc (4 revisions) (flutter/flutter#181431) 2026-01-24 [email protected] [Impeller] Fix interpolation error in Rect::TransformAndClipBounds (flutter/flutter#181420) 2026-01-23 [email protected] Roll Skia from 6d438894c2a8 to 32b52343e757 (2 revisions) (flutter/flutter#181419) 2026-01-23 [email protected] [Material] modernize Typography._withPlatform with Dart 3 switch expression (flutter/flutter#181398) 2026-01-23 [email protected] CupertinoSheetRoute with scrolling and dragging (flutter/flutter#177337) 2026-01-23 [email protected] Adds contents of keys file when a skia gold error occurs. (flutter/flutter#181401) 2026-01-23 [email protected] Roll Skia from e4bd0a355e68 to 6d438894c2a8 (3 revisions) (flutter/flutter#181405) 2026-01-23 [email protected] bump KGP and AGP max known versions (flutter/flutter#181325) 2026-01-23 [email protected] Roll Skia from db10db8bd55f to e4bd0a355e68 (3 revisions) (flutter/flutter#181391) 2026-01-23 [email protected] Roll Packages from 9010299 to 5af5f50 (4 revisions) (flutter/flutter#181388) 2026-01-23 [email protected] Look for project root for FeatureFlags manifest (flutter/flutter#180689) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ession (flutter#181398) Continuing the effort to clean up some of the older switch statements, I've moved over to `typography.dart` I found that `Typography._withPlatform` was using a standard switch block to handle platform-specific theme assignments. Since both `black` and `white` themes are resolved based on the same platform variable, I've refactored this into a single switch expression using a Record. This allowed me to group Android and Fuchsia together using the new logical OR pattern, which feels much cleaner. The logic and the existing fallbacks haven't changed at all—the `assert` at the start already guarantees we have the required themes if the platform is null, so I've handled that case directly in the expression. I've run the local tests in `typography_test.dart` and everything is passing. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. (I will sign it once the bot prompts me) - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
4 participants
Continuing the effort to clean up some of the older switch statements, I've moved over to
typography.dartI found that
Typography._withPlatformwas using a standard switch block to handle platform-specific theme assignments. Since bothblackandwhitethemes are resolved based on the same platform variable, I've refactored this into a single switch expression using a Record. This allowed me to group Android and Fuchsia together using the new logical OR pattern, which feels much cleaner.The logic and the existing fallbacks haven't changed at all—the
assertat the start already guarantees we have the required themes if the platform is null, so I've handled that case directly in the expression.I've run the local tests in
typography_test.dartand everything is passing.Pre-launch Checklist
///).