-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Expose a few more glyph apis from ui.Paragraph
#47698
Expose a few more glyph apis from ui.Paragraph
#47698
Conversation
261615e
to
5149a52
Compare
960e92b
to
2de2458
Compare
100db12
to
581c2fa
Compare
581c2fa
to
8f996ce
Compare
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.
Looks good! I'd like to request a couple minor adjustments to the types along the C boundary in skwasm, if you don't mind. Thanks!
lib/web_ui/skwasm/text/paragraph.cpp
Outdated
SkScalar offsetX, | ||
SkScalar offsetY, | ||
SkScalar* graphemeLayoutBounds, | ||
size_t* garphemeCodeUnitRange, |
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.
There is a typo in this name
Also, could you just put a comment next to the argument that explains it is an array of length two that represents the start and end.
lib/web_ui/skwasm/text/paragraph.cpp
Outdated
SkScalar offsetY, | ||
SkScalar* graphemeLayoutBounds, | ||
size_t* garphemeCodeUnitRange, | ||
bool* booleanFlags) { |
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.
Instead of returning a boolean here, you can directly return a skia::textlayout::TextDirection
, and we use that to index into the enum on the dart side. Check out textBoxList_getBoxAtIndex
as an example of this.
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.
The return value currently indicates whether the out parameters are valid. To add TextDirection
information to it I think we'd need to return a tristate value (LTR, RTL, invalid)?
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.
Sorry, to clarify, my suggestion is to return it by reference as you're doing, but instead of using a bool
just return a TextDirection
by reference. You can leave the actual return value as a normal boolean, indicating success/failure of the overall API call.
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.
Ah I just realized the parameters are called "booleanFlags" because there are a few more booleans I'd like to expose after this (e.g., isPlaceholder
, isEllipsis
) that I'd like to pack together. Would it be ok to keep this a bool*
for now?
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.
I'm fine with leaving it as-is for now. I think longer term maybe what we should be doing is actually declaring the C structs themselves on the dart side using ffi constructs and then actually just passing one struct (right by value or by reference) across the boundary. I have been meaning to experiment with that with these bindings but haven't really had much of a chance.
lib/web_ui/skwasm/text/paragraph.cpp
Outdated
Paragraph* paragraph, | ||
SkScalar offsetX, | ||
SkScalar offsetY, | ||
SkScalar* graphemeLayoutBounds, |
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.
I think just making this a SkRect *
, returning a single rect is a little more expressive. SkRect
is a plain old data struct with four floats, it's pretty safe to assume the memory layout. (We already do this for SkRect
objects in several places).
external bool paragraphGetClosestGlyphInfoAtCoordinate( | ||
ParagraphHandle handle, | ||
double offsetX, double offsetY, | ||
Pointer<Float> graphemeLayoutBounds, // 4 floats, [LTRB] |
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.
You can do Pointer<RawRect>
here, which is the same type but it's just a little more expressive.
final Pointer<Bool> outBooleanFlags = scope.allocBoolArray(1); | ||
return paragraphGetGlyphInfoAt(handle, codeUnitOffset, outRect, outRange, outBooleanFlags) | ||
? ui.GlyphInfo( | ||
ui.Rect.fromLTRB(outRect[0], outRect[1], outRect[2], outRect[3]), |
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.
There is a scope.convertRectFromNative
helper you can use here.
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.
Skwasm stuff looks good to me.
lib/ui/text/paragraph.cc
Outdated
Dart_Handle Paragraph::getWordBoundary(unsigned offset) { | ||
txt::Paragraph::Range<size_t> point = m_paragraph->GetWordBoundary(offset); | ||
Dart_Handle glyphInfoFrom(Dart_Handle constructor, | ||
skia::textlayout::Paragraph::GlyphInfo& glyphInfo) { |
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.
Pass this as a const
reference
hi @mdebbar could you take a look and see if the web implementation makes sense? |
963c2c3
to
cbf8657
Compare
if (lineNumber == null) { | ||
return null; | ||
} | ||
final List<LayoutFragment> lineFragments = lines[lineNumber].fragments; |
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.
I'm not sure if it's a good idea to calculate grapheme clusters only inside a single fragment. A grapheme cluster could start in one fragment, and end in another.
Here's an idea and let me know what you think:
- Calculate grapheme clusters on the entire paragraph.
- Find the grapheme cluster that contains
codeUnitOffset
. - Find the subset of fragments that intersect with that cluster (using
overlapsWith(...)
). - For each of these fragments, do
fragment.toTextBox(start: graphemeStart, end: graphemeEnd).toRect()
. - Calculate the union (?) of all the produced rects (using
Rect.expandToInclude(...)
maybe?). - For the direction, you can just use the direction from any of the intersecting fragments (I think all of them must have the same direction, because a grapheme cluster can't have multiple direction inside of it, right?)
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 grapheme cluster could start in one fragment, and end in another.
Ah interesting. When would this happen (so I can write a test for it)? When the codeunits that belong to the same grapheme are split into different TextSpans?
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.
When the codeunits that belong to the same grapheme are split into different TextSpans?
Yes, that's the one possibility I could think of. Not sure if there are others.
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.
Alright I ended up doing grapheme breaks per line (all breaks within a grapheme cluster are prohibited so grapheme clusters won't be broken into different lines I think). PTAL.
8a3554b
to
a4f976b
Compare
a4f976b
to
dc2505a
Compare
88c0fd5
to
778e982
Compare
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! Thanks for making the changes to make this more robust.
…ions) (#138918) Manual roll requested by [email protected] flutter/engine@342fb00...f331e0a 2023-11-22 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] pass const ref to binding helpers." (flutter/engine#48330) 2023-11-22 [email protected] Revert "Manual roll Dart SDK from f1fd14505782 to df958dc1ca7b (6 revisions)" (flutter/engine#48325) 2023-11-22 [email protected] [Impeller] cache render target properties on Render Pass. (flutter/engine#48323) 2023-11-22 [email protected] [Impeller] pass const ref to binding helpers. (flutter/engine#48318) 2023-11-22 [email protected] Roll Skia from 994558cd1fae to 30ecaac60b47 (1 revision) (flutter/engine#48324) 2023-11-22 [email protected] Roll Skia from 9086788fc341 to 994558cd1fae (1 revision) (flutter/engine#48322) 2023-11-22 [email protected] Expose a few more glyph apis from `ui.Paragraph` (flutter/engine#47698) 2023-11-22 [email protected] Roll Skia from efdec1f459ce to 9086788fc341 (2 revisions) (flutter/engine#48317) 2023-11-22 [email protected] Manual roll Dart SDK from f1fd14505782 to df958dc1ca7b (6 revisions) (flutter/engine#48316) 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://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
…ions) (flutter#138918) Manual roll requested by [email protected] flutter/engine@342fb00...f331e0a 2023-11-22 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] pass const ref to binding helpers." (flutter/engine#48330) 2023-11-22 [email protected] Revert "Manual roll Dart SDK from f1fd14505782 to df958dc1ca7b (6 revisions)" (flutter/engine#48325) 2023-11-22 [email protected] [Impeller] cache render target properties on Render Pass. (flutter/engine#48323) 2023-11-22 [email protected] [Impeller] pass const ref to binding helpers. (flutter/engine#48318) 2023-11-22 [email protected] Roll Skia from 994558cd1fae to 30ecaac60b47 (1 revision) (flutter/engine#48324) 2023-11-22 [email protected] Roll Skia from 9086788fc341 to 994558cd1fae (1 revision) (flutter/engine#48322) 2023-11-22 [email protected] Expose a few more glyph apis from `ui.Paragraph` (flutter/engine#47698) 2023-11-22 [email protected] Roll Skia from efdec1f459ce to 9086788fc341 (2 revisions) (flutter/engine#48317) 2023-11-22 [email protected] Manual roll Dart SDK from f1fd14505782 to df958dc1ca7b (6 revisions) (flutter/engine#48316) 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://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
Add 2 methods for querying glyph-related metrics
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.