Expose LineMetrics in dart:ui #10670

GaryQian · 2019-08-06T21:53:31Z

This PR performs the tonic/custom transition for the new LibTxt metrics between native and dart.

Part of a series of changes to fix flutter/flutter#35994

Adds getLineMetrics() to dart:ui Paragraph. This is a catch-all "return all of the metrics at once" call that is a good point to start.

This blanket method will provide full access to the data we track. I am currently considering adding additional API to slice the data into more manageable chunks so that the data required can be queried more directly, reducing the amount of unused data passed through tonic.

The dart:ui tests will reside in the framework test suite due to a lack of direct dart:ui testing setup and will be in a separate PR (I will link shortly).

jonahwilliams · 2019-08-08T23:45:18Z

lib/ui/text.dart

 }

+/// Data class that holds the metrics for a single line in the [Paragraph].
+class LineMetrics {


Add this class to stub ui and file an issue to implement for the web

jonahwilliams · 2019-08-09T00:34:44Z

lib/stub_ui/lib/src/ui/text.dart

+  ///
+  /// This can potentially return a large amount of data, so it is not recommended
+  /// to repeatedly call this. Instead, cache the results.
+  List<LineMetrics> getLineMetrics();


These will need implementations in the _engine sub directory. In this case I would return an empty list similar to what the implementation for getBoxesForPlaceholders does.

cc @yjbanov

Let me know if any of the metrics here are difficult to obtain for web. Some of them are quite libtxt/skia specific. I can still remove some of them easily without hurting functionality too much.

cc @mdebbar

I'm unlikely to get to it before @mdebbar is back. @mdebbar is currently redesigning out text layout. The new design will give us much more information, such as this. Our current design won't give us per-line information like this.

yjbanov · 2019-08-09T17:07:16Z

lib/stub_ui/lib/src/ui/text.dart

+    this.lineNumber,
+    List<RunMetrics> rMetrics,
+    List<int> rMetricsIndexes,
+  ) : runMetrics = Map() {


Is it possible to use an immutable map here? If it's too expensive, we could use an immutable map in debug builds and a regular map in profile/release builds.

yjbanov · 2019-08-09T17:11:16Z

lib/ui/text.dart

+  /// metrics of each laid out line.
+  ///
+  /// This can potentially return a large amount of data, so it is not recommended
+  /// to repeatedly call this. Instead, cache the results.


Is this value available prior to calling layout? If not, let's document that.

Does the value depend on ParagraphConstraints passed to the layout method? If not, we could cache right here and not scare the user with the large amount of data disclaimer. Actually, even if it does depend on the ParagraphConstratins, we can still cache and invalidate the cache next time layout is called.

yjbanov · 2019-08-09T17:15:04Z

lib/ui/text/line_metrics.h

+  // Zero indexed line number.
+  size_t line_number = 0;
+
+  std::map<size_t, txt::RunMetrics> run_metrics;


The fields are immutable in Dart. Do they have to be publicly mutable in the engine?

yjbanov · 2019-08-09T17:19:50Z

lib/ui/text/paragraph.cc

 }

+std::vector<LineMetrics> Paragraph::getLineMetrics() {
+  return m_paragraphImpl->getLineMetrics();


This seems to be doing a lot of copying.

yjbanov · 2019-08-09T17:23:32Z

lib/ui/text/paragraph.h

  std::vector<TextBox> getRectsForPlaceholders();
  Dart_Handle getPositionForOffset(double dx, double dy);
  Dart_Handle getWordBoundary(unsigned offset);
+  std::vector<LineMetrics> getLineMetrics();


This looks like an expensive data structure, because it doesn't use pointers. Passing it around would involve, copying the vector, copying each LineMetrics object, and recursively copying std::map<size_t, txt::RunMetrics> that's inside it.

yjbanov · 2019-08-09T17:24:17Z

lib/ui/text/line_metrics.h

+
+namespace flutter {
+
+struct LineMetrics {


Docs? For example, it would be interesting to know why the memory layout is chosen to be flat.

jason-simmons · 2019-08-09T00:40:20Z

lib/ui/text.dart

+    this.lineNumber,
+    List<RunMetrics> rMetrics,
+    List<int> rMetricsIndexes,
+  ) : runMetrics = Map() {


use Map.fromIterables(rMetricsIndexes, rMetrics)

jason-simmons · 2019-08-09T00:45:16Z

lib/ui/text.dart

+    }
+  }
+
+  /// The index in the text buffer the line begins.


change this to "where the line begins" (same for endIndex, endExcludingWhitespace, endIncludingNewline)

jason-simmons · 2019-08-09T00:49:52Z

third_party/txt/tests/paragraph_unittests.cc

+  txt::ParagraphBuilderTxt builder(paragraph_style, GetTestFontCollection());
+
+  txt::TextStyle text_style;
+  // We must supply a font here, as the default is Arial, and we do not


I would remove this comment. This test seems similar to most other paragraph unit test which use Roboto because it is available as a test font.

goderbauer

Reviewed the dart files

goderbauer · 2019-08-09T17:10:15Z

lib/stub_ui/lib/src/ui/text.dart


+/// Data class that holds the metrics for a single line in the [Paragraph].
+class LineMetrics {
+


nit: remove empty line

goderbauer · 2019-08-09T17:10:56Z

lib/stub_ui/lib/src/ui/text.dart

+    }
+  }
+
+  /// The index in the text buffer the line begins.


Is this missing a "where" or some word like it before "the line begins"?

(Same comment for many of the following docs).

goderbauer · 2019-08-09T17:16:03Z

lib/stub_ui/lib/src/ui/text.dart

+    this.lineNumber,
+    List<RunMetrics> rMetrics,
+    List<int> rMetricsIndexes,
+  ) : runMetrics = Map() {


Suggested change

) : runMetrics = Map() {

) : runMetrics = <int, RunMetrics>{} {

goderbauer · 2019-08-09T17:17:24Z

lib/stub_ui/lib/src/ui/text.dart

+    List<int> rMetricsIndexes,
+  ) : runMetrics = Map() {
+    for (int i = 0; i < rMetrics.length; ++i) {
+      runMetrics[rMetricsIndexes[i]] = rMetrics[i];


Would Map.fromIterables be helpful here at all?

https://master-api.flutter.dev/flutter/dart-core/Map/Map.fromIterables.html

goderbauer · 2019-08-09T17:19:08Z

lib/stub_ui/lib/src/ui/text.dart

+    }
+  }
+
+  /// The index in the text buffer the line begins.


(maybe this is clear from context where this will be used, but...)

While reading through these API docs I kept wondering what "the test buffer" is that's mentioned here over and over.

goderbauer · 2019-08-09T17:35:04Z

lib/stub_ui/lib/src/ui/text.dart

+
+  /// Extent above baseline.
+  final double top;
+  /// Distance to reserve above baseline.


insert empty line after each property?

goderbauer · 2019-08-09T17:35:45Z

lib/stub_ui/lib/src/ui/text.dart

+  final double top;
+  /// Distance to reserve above baseline.
+  ///
+  /// Since this is the raw metric from the font, [ascent] is usually negative.


Is this only true for ascent? Or also for some of the other metrics here?

Let me double check, but it may also apply to top, although we dont track top separately so an abs is never applied to it.

goderbauer · 2019-08-09T17:36:10Z

lib/stub_ui/lib/src/ui/text.dart

+  final double avgCharWidth;
+  /// Maximum character width.
+  final double maxCharWidth;
+  /// Minimum x.


That is a good question, I am not totally sure myself, but I will find out. This is one of the metrics from SkFontMetrics, but we don't seem to use it anywhere. It is provided to us nonetheless.

There is some ambiguous naming going on where X refers to x-coordinate as well as x the ascii letter. I can try to clarify this in our API.

goderbauer · 2019-08-09T17:36:41Z

lib/stub_ui/lib/src/ui/text.dart

+  final double capHeight;
+  /// Underline thickness.
+  ///
+  /// Can be null.


When is it null, can we say?

(same for other properties)

goderbauer · 2019-08-09T17:38:35Z

lib/ui/text.dart

  middle,
 }

+/// Data class that holds the metrics for a single line in the [Paragraph].


All comments I made on the previous file also apply here.

@jonahwilliams I thought we no longer repeat doc comments in the stub impl... Is the doc comment duplication still necessary?

You don't need to repeat doc comments.

That said, I'm not enforcing this via a lint until there is an unfork, because the code in stub_ui is not the source of truth.

@GaryQian Can you remove the comments from the stubs then?

yjbanov · 2019-08-09T18:14:49Z

@GaryQian Heads up! I just pushed a change that renamed stub_ui to web_ui.

Hixie · 2019-08-12T19:46:18Z

lib/stub_ui/lib/src/ui/text.dart

+    }
+  }
+
+  /// The index in the text buffer the line begins.


what is this index? number of UTF-16 words?
We shouldn't add any new APIs that use UTF-16 word counts.

Hixie · 2019-08-12T19:46:53Z

lib/stub_ui/lib/src/ui/text.dart

+  /// This is equivalent to `ascent + descent`
+  final double height;
+
+  /// Width of the line.


this documentation violates our style guide

Hixie · 2019-08-12T19:47:37Z

lib/stub_ui/lib/src/ui/text.dart

+  /// The right edge can be obtained with `left + width`.
+  final double left;
+
+  /// The y position of the baseline for this line from the top of the paragraph.


We should be consistent in our style here: x-coordinate, y position, etc. Maybe see what we do today in the framework (e.g. it might be _x_ coordinate)

Hixie · 2019-08-12T19:48:31Z

lib/stub_ui/lib/src/ui/text.dart

+  /// The y position of the baseline for this line from the top of the paragraph.
+  final double baseline;
+
+  /// Zero indexed line number.


"Zero indexed line number" tells me nothing really. What is a line number?

Maybe "The number of this line in the overall paragraph, with the first line being index zero" or something?

Hixie · 2019-08-12T19:49:14Z

lib/stub_ui/lib/src/ui/text.dart

+  ///
+  /// Each entry will be keyed under the ending index the run applies to. Each
+  /// [RunMetrics] entry corresponds to the text covered by the range of
+  /// `[previous key, current key)`. For a given index `i`, the [RunMetrics]


generally speaking many of our developers don't know the [/( ... )/] convention. Please be explicit in prose instead.

Hixie · 2019-08-12T19:50:25Z

lib/stub_ui/lib/src/ui/text.dart

+class LineMetrics {
+
+  @pragma('vm:entry-point')
+  LineMetrics._(


this this constructor have to be private?

Might be worth having this be a public constructor with named arguments and having a private constructor with positional arguments for calling from C++. Having a way to create this class from Dart seems valuable (e.g. for testing).

Hixie · 2019-08-12T19:50:48Z

lib/stub_ui/lib/src/ui/text.dart

+  final Map<int, RunMetrics> runMetrics;
+}
+
+/// Data class that tracks the raw font metrics for a particular text buffer index.


what is a "Data class"?

Hixie · 2019-08-12T19:50:58Z

lib/stub_ui/lib/src/ui/text.dart

+/// Data class that tracks the raw font metrics for a particular text buffer index.
+class RunMetrics {
+  @pragma('vm:entry-point')
+  RunMetrics._(


same comment about constructors

Hixie · 2019-08-12T19:51:25Z

lib/stub_ui/lib/src/ui/text.dart

+  ///
+  /// Can be null.
+  final double strikeoutPosition;
+}


these docs all need much more detail. see the style guide for writing prompts.

Hixie · 2019-08-12T19:54:58Z

lib/stub_ui/lib/src/ui/text.dart

+  ///
+  /// This can potentially return a large amount of data, so it is not recommended
+  /// to repeatedly call this. Instead, cache the results.
+  List<LineMetrics> getLineMetrics();


i would call this computeLineMetrics to make it sound expensive. Things called getFoo should really be getters, and those look cheap.

GaryQian · 2019-08-16T00:13:31Z

Status Update: This is on hold momentarily as I fix a segfault in the tracking of the metrics in LibTxt.

Next, I'll strip this down to minimize UTF-16 index usage in anticipation for dart's updated text system.

GaryQian · 2019-08-22T22:05:53Z

Waiting on #11064 to land in order to continue on this.

GaryQian · 2019-08-24T00:47:29Z

Ok, this simplified version should be ready for another round of review. We no longer track anything that is connected to or indexed by UTF-16 codepoints. This means the RunMetrics class is on hold. We now just provide the main measurement metrics about the line itself.

goderbauer

LGTM for the .dart files with some minor nits

goderbauer · 2019-08-26T17:45:44Z

lib/ui/text.dart

  middle,
 }

+class LineMetrics {


Add a doc comment for the class and constructor?

goderbauer · 2019-08-26T17:46:37Z

lib/ui/text.dart

+  /// The [ascent] is provided as a positive value, even though it is typically defined
+  /// in fonts as negative. This is to ensure the signage of operations with these
+  /// metrics directly reflects the intended signage of the value. For example,
+  /// the y coordinate of the top edge of the line is 'baseline - ascent`.


should ' be a `?

Same question everywhere below.

goderbauer · 2019-08-26T17:50:12Z

lib/ui/text.dart

+    this.left = null,
+    this.baseline = null,
+    this.lineNumber = null,
+  }) {}


This should just be : instead of {}

goderbauer · 2019-08-26T17:50:17Z

lib/ui/text.dart

+    this.left,
+    this.baseline,
+    this.lineNumber,
+  ) {}


goderbauer · 2019-08-26T17:50:46Z

lib/ui/text.dart


+class LineMetrics {
+  LineMetrics({
+    this.hardBreak = null,


Why the = null here? That's the default.

goderbauer · 2019-08-26T18:08:27Z

lib/ui/text.dart

  middle,
 }

+/// `LineMetrics` stores the measurements and statistics of a single line in the


This should be enclosed in [] instead of `

goderbauer · 2019-08-26T18:08:42Z

lib/ui/text.dart

+/// extent of the line instead of per-run or per-glyph metrics. For more detailed
+/// metrics, see [TextBox] and [Paragraph.getBoxesForRange].
+///
+/// `LineMetrics` should be obtained directly from the [Paragraph.computeLineMetrics]


[email protected]:flutter/engine.git/compare/ba1a303ae2d7...9f1eab2 git log ba1a303..9f1eab2 --no-merges --oneline 2019-08-26 [email protected] Expose LineMetrics in dart:ui (flutter/engine#10670) 2019-08-26 [email protected] Roll buildroot and update gn script for bitcode_marker (flutter/engine#11449) 2019-08-26 [email protected] Roll src/third_party/skia d617d5d29ca5..464eb78e444b (10 commits) (flutter/engine#11444) 2019-08-26 [email protected] Roll fuchsia/clang/mac-amd64 from RDjN2... to CGYQg... (flutter/engine#11442) 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] on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

GaryQian added brand new feature affects: text input Work in progress (WIP) Not ready (yet) for review! labels Aug 6, 2019

GaryQian self-assigned this Aug 6, 2019

googlebot added the cla: yes label Aug 6, 2019

GaryQian mentioned this pull request Aug 8, 2019

Expose enough of LibTxt to make custom text layout practical flutter/flutter#35994

Closed

5 tasks

GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label Aug 8, 2019

GaryQian requested review from Hixie, chinmaygarde, goderbauer and jason-simmons August 8, 2019 23:38

jonahwilliams reviewed Aug 8, 2019

View reviewed changes

jonahwilliams reviewed Aug 9, 2019

View reviewed changes

yjbanov reviewed Aug 9, 2019

View reviewed changes

jason-simmons reviewed Aug 9, 2019

View reviewed changes

goderbauer reviewed Aug 9, 2019

View reviewed changes

Hixie reviewed Aug 12, 2019

View reviewed changes

expose simplified dart:ui line metrics

df68fa9

GaryQian force-pushed the metricsdart branch from 2ccb0d3 to df68fa9 Compare August 23, 2019 19:43

GaryQian added 2 commits August 23, 2019 16:09

Cleanup, address comments

077fad2

web_ui mirror and const line metrics

f5c7865

Finish web_ui api mirror, remove comments

d1b9b0c

goderbauer reviewed Aug 26, 2019

View reviewed changes

Address comments, more docs

afe3620

goderbauer reviewed Aug 26, 2019

View reviewed changes

GaryQian added 3 commits August 26, 2019 11:11

Fix Web_ui

bd2e1b7

Fix nit

c2087c8

Update LICENSES

77f670b

GaryQian changed the title ~~Expose LineMetrics and RunMetrics in dart:ui~~ Expose LineMetrics in dart:ui Aug 26, 2019

GaryQian merged commit 9f1eab2 into flutter:master Aug 26, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 26, 2019

9f1eab2 Expose LineMetrics in dart:ui (flutter/engine#10670)

c9784e2

engine-flutter-autoroll mentioned this pull request Aug 26, 2019

Roll engine ba1a303ae2d7..9f1eab2f1f26 (4 commits) flutter/flutter#39278

Merged

This was referenced Aug 26, 2019

Update the ui.LineMetrics.height metric to be more useful to external users #11456

Merged

Expose text metrics in TextPainter. flutter/flutter#39282

Merged


		/// Data class that holds the metrics for a single line in the [Paragraph].
		class LineMetrics {

	) : runMetrics = Map() {
	) : runMetrics = <int, RunMetrics>{} {


		namespace flutter {

		struct LineMetrics {

Expose LineMetrics in dart:ui #10670

Expose LineMetrics in dart:ui #10670

Uh oh!

Conversation

GaryQian commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian Aug 9, 2019 • 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!

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!

goderbauer 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!

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!

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!

yjbanov commented Aug 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Aug 12, 2019 • 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!

GaryQian commented Aug 6, 2019 •

edited

Loading

GaryQian Aug 9, 2019 •

edited

Loading

Hixie Aug 12, 2019 •

edited

Loading

GaryQian commented Aug 22, 2019 •

edited

Loading