Expose a few more glyph apis from `ui.Paragraph` by LongCatIsLooong · Pull Request #47698 · flutter/engine

LongCatIsLooong · 2023-11-06T00:32:33Z

Add 2 methods for querying glyph-related metrics

  GlyphInfo? getClosestGlyphInfoForOffset(Offset offset); 
  GlyphInfo? getGlyphInfoAt(int codeUnitOffset);

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

eyebrowsoffire

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!

eyebrowsoffire · 2023-11-06T18:12:18Z

lib/web_ui/skwasm/text/paragraph.cpp

+    SkScalar offsetX,
+    SkScalar offsetY,
+    SkScalar* graphemeLayoutBounds,
+    size_t* garphemeCodeUnitRange,


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.

eyebrowsoffire · 2023-11-06T18:18:34Z

lib/web_ui/skwasm/text/paragraph.cpp

+    SkScalar offsetY,
+    SkScalar* graphemeLayoutBounds,
+    size_t* garphemeCodeUnitRange,
+    bool* booleanFlags) {


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.

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)?

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.

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?

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.

eyebrowsoffire · 2023-11-06T18:36:21Z

lib/web_ui/skwasm/text/paragraph.cpp

+    Paragraph* paragraph,
+    SkScalar offsetX,
+    SkScalar offsetY,
+    SkScalar* graphemeLayoutBounds,


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).

eyebrowsoffire · 2023-11-06T18:39:20Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/text/raw_paragraph.dart

+external bool paragraphGetClosestGlyphInfoAtCoordinate(
+  ParagraphHandle handle,
+  double offsetX, double offsetY,
+  Pointer<Float> graphemeLayoutBounds,   // 4 floats, [LTRB]


You can do Pointer<RawRect> here, which is the same type but it's just a little more expressive.

eyebrowsoffire · 2023-11-06T18:42:29Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart

+      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 is a scope.convertRectFromNative helper you can use here.

eyebrowsoffire

Skwasm stuff looks good to me.

jason-simmons · 2023-11-07T18:18:32Z

lib/ui/text/paragraph.cc

-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) {


Pass this as a const reference

LongCatIsLooong · 2023-11-13T23:13:39Z

hi @mdebbar could you take a look and see if the web implementation makes sense?

lib/web_ui/lib/src/engine/text/layout_fragmenter.dart

mdebbar · 2023-11-17T19:09:32Z

lib/web_ui/lib/src/engine/text/canvas_paragraph.dart

+    if (lineNumber == null) {
+      return null;
+    }
+    final List<LayoutFragment> lineFragments = lines[lineNumber].fragments;


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?)

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?

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.

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.

mdebbar

LGTM! Thanks for making the changes to make this more robust.

…ine#47698)

…8918) 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

…tter#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

LongCatIsLooong added 6 commits September 20, 2023 21:17

Add more Paragraph API: glyphs

b678326

doc

627742c

wip

1a8bb68

wip

6655682

Merge remote-tracking branch 'upstream/main' into paragraph-api-glyph

d638302

wip

d33b67d

github-actions bot added the platform-web Code specifically for the web engine label Nov 6, 2023

LongCatIsLooong force-pushed the paragraph-api-glyph branch from 261615e to 5149a52 Compare November 6, 2023 00:58

tests

fdc3f54

LongCatIsLooong force-pushed the paragraph-api-glyph branch 3 times, most recently from 960e92b to 2de2458 Compare November 6, 2023 02:57

oops

b09fe04

LongCatIsLooong force-pushed the paragraph-api-glyph branch 3 times, most recently from 100db12 to 581c2fa Compare November 6, 2023 07:04

canvaskit binding

8f996ce

LongCatIsLooong force-pushed the paragraph-api-glyph branch from 581c2fa to 8f996ce Compare November 6, 2023 07:33

LongCatIsLooong marked this pull request as ready for review November 6, 2023 08:26

LongCatIsLooong requested review from eyebrowsoffire, jason-simmons and mdebbar and removed request for mdebbar November 6, 2023 08:27

eyebrowsoffire suggested changes Nov 6, 2023

View reviewed changes

review

336e08d

eyebrowsoffire approved these changes Nov 7, 2023

View reviewed changes

jason-simmons approved these changes Nov 7, 2023

View reviewed changes

review

1dabb94

Merge branch 'main' into paragraph-api-glyph

cbf8657

LongCatIsLooong force-pushed the paragraph-api-glyph branch from 963c2c3 to cbf8657 Compare November 17, 2023 18:37

mdebbar reviewed Nov 17, 2023

View reviewed changes

LongCatIsLooong force-pushed the paragraph-api-glyph branch from 8a3554b to a4f976b Compare November 21, 2023 09:25

Handle broken graphemes

dc2505a

LongCatIsLooong force-pushed the paragraph-api-glyph branch from a4f976b to dc2505a Compare November 21, 2023 10:09

undo textDirection change

778e982

LongCatIsLooong force-pushed the paragraph-api-glyph branch from 88c0fd5 to 778e982 Compare November 21, 2023 18:20

LongCatIsLooong requested a review from mdebbar November 21, 2023 18:38

fix failing test

4aab17d

mdebbar approved these changes Nov 22, 2023

View reviewed changes

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023

auto-submit bot merged commit 8194c23 into flutter:main Nov 22, 2023

engine-flutter-autoroll mentioned this pull request Nov 22, 2023

Roll Flutter Engine from 342fb003b11f to 8194c2383a56 (3 revisions) flutter/flutter#138909

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023

8194c23 Expose a few more glyph apis from ui.Paragraph (flutter/eng…

3a41a97

…ine#47698)

engine-flutter-autoroll mentioned this pull request Nov 22, 2023

Roll Flutter Engine from 342fb003b11f to 0e160b5d1f12 (4 revisions) flutter/flutter#138911

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023

8194c23 Expose a few more glyph apis from ui.Paragraph (flutter/eng…

2d0b1ed

…ine#47698)

engine-flutter-autoroll mentioned this pull request Nov 22, 2023

Roll Flutter Engine from 342fb003b11f to d02bf967844e (5 revisions) flutter/flutter#138913

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023

8194c23 Expose a few more glyph apis from ui.Paragraph (flutter/eng…

3d2a2ae

…ine#47698)

engine-flutter-autoroll mentioned this pull request Nov 22, 2023

Manual roll Flutter Engine from 342fb003b11f to f331e0afaf2e (9 revisions) flutter/flutter#138918

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2023

8194c23 Expose a few more glyph apis from ui.Paragraph (flutter/eng…

f2d49cf

…ine#47698)

LongCatIsLooong deleted the paragraph-api-glyph branch November 22, 2023 22:05

Conversation

LongCatIsLooong commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

eyebrowsoffire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyebrowsoffire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Nov 13, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LongCatIsLooong commented Nov 6, 2023 •

edited

Loading

LongCatIsLooong Nov 6, 2023 •

edited

Loading