Skip to content

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Dec 11, 2024

This is for #159999. That PR breaks registered tests so a new matcher is added for soft transition & making it slightly easier to write tests that verify nothing is shadowing the system text scaler in the widget tree.

If this approach sounds plausible & gets merged, I'm going to:

  1. remake the breaking change announcement
  2. update the migration guide with the new matcher,
  3. migrate the registered tests and mark Wire up the system text scaler from PlatformDispatcher #159999 as ready for review.

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: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Dec 11, 2024
}

@override
double scaleFontSize(double unscaledFontSize) => textScaleFactor * unscaledFontSize;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for this?

///
/// If `withScaleFactor` is specified and non-null, this matcher also asserts
/// that the [TextScaler]'s' `textScaleFactor` equals `withScaleFactor`.
Matcher isSystemTextScaler({ double? withScaleFactor }) {
Copy link
Member

Choose a reason for hiding this comment

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

What is a non-system text scaler? Can you add something to the docs that describes the difference between a system text scaler and a non-system one?

And for my own understanding: Why should I care in my tests whether the text scaler is system or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A "system" one is one that directly reflects the user's preference while the developer could introduce a MediaQuery with a "non-system" scaler (with a fixed 1.0 scaling factor, for example). This is useful (I guess) to make sure the text scaler from a certain BuildContext is using the user's preference instead of using some fixed scales.

Copy link
Member

Choose a reason for hiding this comment

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

Wanna add that context to the doc? I think it would be useful to specify what cases would fail this check (i.e. when developers introduce their own text scaler in a widget)

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

///
/// If `withScaleFactor` is specified and non-null, this matcher also asserts
/// that the [TextScaler]'s' `textScaleFactor` equals `withScaleFactor`.
Matcher isSystemTextScaler({ double? withScaleFactor }) {
Copy link
Member

Choose a reason for hiding this comment

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

Wanna add that context to the doc? I think it would be useful to specify what cases would fail this check (i.e. when developers introduce their own text scaler in a widget)

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

@justinmc
Copy link
Contributor

@LongCatIsLooong Heads up on this old PR if you plan to return to it after break.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 3, 2025
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 3, 2025
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 3, 2025

autosubmit label was removed for flutter/flutter/160120, because - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 3, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Mar 3, 2025
Merged via the queue into flutter:master with commit ac71188 Mar 3, 2025
74 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 3, 2025
@LongCatIsLooong LongCatIsLooong deleted the isSystemTextScaler-matcher branch March 3, 2025 20:36
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels.

Projects

Development

Successfully merging this pull request may close these issues.

3 participants