-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add a isSystemTextScaler matcher
#160120
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.
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
Add a isSystemTextScaler matcher
#160120
Conversation
| } | ||
|
|
||
| @override | ||
| double scaleFontSize(double unscaledFontSize) => textScaleFactor * unscaledFontSize; |
There was a problem hiding this comment.
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 }) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
goderbauer
left a comment
There was a problem hiding this 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 }) { |
There was a problem hiding this comment.
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)
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@LongCatIsLooong Heads up on this old PR if you plan to return to it after break. |
|
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. |
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:
PlatformDispatcher#159999 as ready for review.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.