Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

matanlurey
Copy link
Contributor

Copy link
Contributor

@jonahwilliams jonahwilliams 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 our text frame bounds computation is wrong, that is going to bubble up to more places too.
Can we either 1) make impeller::TextFrame use the bounds data from the SkTextFrame 2) fix the impeller::TextFrame computation?

@matanlurey
Copy link
Contributor Author

I'm still learning the words, so I'm not sure what difference between a TextBlob and a TextFrame is, but I'm not sure the TextFrame bounds are wrong, just potentially not appropriate in this case? Note that blob->bounds() is probably not the same thing as frame->bounds()?

@jonahwilliams
Copy link
Contributor

The impeller::TextFrame is the analog of the SkTextBlob

@jonahwilliams
Copy link
Contributor

(Not necessarily in this PR, FWIW)

@jonahwilliams
Copy link
Contributor

Actually in general TextFrame::GetBounds is super hot, so we might want to take the SkiaBounds in the constructor and use them cached, see #45131 (comment)

@matanlurey
Copy link
Contributor Author

Ah, sure! Filed flutter/flutter#133388.

@matanlurey matanlurey merged commit b5f1821 into flutter:main Aug 27, 2023
@matanlurey matanlurey deleted the impeller-stroked-text-offset branch August 27, 2023 00:50
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 27, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Aug 27, 2023
…133401)

flutter/engine@683d7dc...b5f1821

2023-08-27 [email protected] Use `SkTextBlob::bounds` instead of `TextFrame::getBounds()`. (flutter/engine#45148)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Impeller] PaintingStyle.stroke for Text: Rendered text has an offset
2 participants