Skip to content

Making TextPainter rounding hack disabled by default #132094

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
Merged
auto-submit merged 3 commits into from
Aug 10, 2023
Merged

Making TextPainter rounding hack disabled by default #132094

auto-submit merged 3 commits into from
Aug 10, 2023

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Aug 7, 2023 •
edited
Loading

Migrate tests in flutter/flutter. Once the tests here and in *_customer_testing are migrated, the default value of the migration flag will be changed from false to true, making the rounding hack disabled by default.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos a: internationalization Supporting other languages or locales. (aka i18n) f: cupertino flutter/packages/flutter/cupertino repository labels Aug 7, 2023
@LongCatIsLooong LongCatIsLooong force-pushed the text-painter-fix-tests branch 3 times, most recently from a8da0fe to 21f0ea8 Compare August 8, 2023 18:57
@github-actions github-actions bot added the engine flutter/engine related. See also e: labels. label Aug 8, 2023
@LongCatIsLooong LongCatIsLooong force-pushed the text-painter-fix-tests branch from 71583e4 to 18802bf Compare August 9, 2023 03:01
@LongCatIsLooong LongCatIsLooong changed the title Try making TextPainter rounding hack disabled by default Making TextPainter rounding hack disabled by default Aug 9, 2023
@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review August 9, 2023 04:57
@LongCatIsLooong LongCatIsLooong removed a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. a: internationalization Supporting other languages or locales. (aka i18n) d: api docs Issues with https://api.flutter.dev/ labels Aug 9, 2023
@LongCatIsLooong LongCatIsLooong mentioned this pull request Aug 9, 2023
Merged
8 tasks
justinmc
justinmc approved these changes Aug 9, 2023
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 👍. Looks like a lot of cruft going away because of this rounding thing. A super editor test is failing but I guess it has to be migrated too.

packages/flutter/lib/src/painting/text_painter.dart Outdated
Comment on lines 517 to 520
Copy link
Contributor

@justinmc justinmc Aug 9, 2023

Choose a reason for hiding this comment

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

Can you explain this again, are you planning to remove the assert wrapping?

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Aug 9, 2023

Choose a reason for hiding this comment

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

Yeah it will be removed once flutter/engine#44544 lands.

packages/flutter/test/cupertino/dialog_test.dart Outdated
Comment on lines -350 to -351
Copy link
Contributor

@justinmc justinmc Aug 9, 2023

Choose a reason for hiding this comment

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

Can you remove the TODO above here? I see that you mentioned the 99933 issue in some other tests.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems a: internationalization Supporting other languages or locales. (aka i18n) d: api docs Issues with https://api.flutter.dev/ labels Aug 9, 2023
@LongCatIsLooong LongCatIsLooong force-pushed the text-painter-fix-tests branch from 17ffa22 to 802ca4c Compare August 9, 2023 20:52
@LongCatIsLooong LongCatIsLooong force-pushed the text-painter-fix-tests branch from 802ca4c to e38a64f Compare August 9, 2023 21:22
@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023
@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Aug 10, 2023

Setting google testing to passing: the pr_test script failed for me and this change shouldn't affect google3.

@LongCatIsLooong LongCatIsLooong deleted the text-painter-fix-tests branch August 10, 2023 00:31
auto-submit bot pushed a commit to flutter/engine that referenced this pull request Aug 10, 2023
@LongCatIsLooong
Disable text rounding hack by default (#44544)
7b6be5c 
This depends on flutter/flutter#132094 and customer_testing migration.

I'll announce this change and add a g3 fix after this lands.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
f16c2b5 
…/flutter#132094)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
015dc38 
…/flutter#132094)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
43347f7 
…/flutter#132094)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
3ccae10 
…/flutter#132094)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
a1f4ff4 
…/flutter#132094)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
6eea098 
…/flutter#132094)
@LongCatIsLooong LongCatIsLooong mentioned this pull request Aug 10, 2023
Merged
8 tasks
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
be06749 
…/flutter#132094)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
c9d464a 
…/flutter#132094)
auto-submit bot pushed a commit that referenced this pull request Aug 10, 2023
@LongCatIsLooong
TextPainter migration cleanup (#132317)
e1fd2ac 
Undo temporary changes made in #132094
@HansMuller HansMuller mentioned this pull request Aug 11, 2023
Closed
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
@LongCatIsLooong @gaaclarke
Disable text rounding hack by default (flutter#44544)
d349902 
This depends on flutter/flutter#132094 and customer_testing migration.

I'll announce this change and add a g3 fix after this lands.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
@engine-flutter-autoroll
3f831b6 Making TextPainter rounding hack disabled by default (flutter…
2b15581 
…/flutter#132094)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

+1 more reviewer

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: internationalization Supporting other languages or locales. (aka i18n) a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. 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.

3 participants

@LongCatIsLooong @justinmc @HansMuller