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

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Nov 22, 2019

@mdebbar mdebbar added the platform-web Code specifically for the web engine label Nov 22, 2019
@mdebbar mdebbar requested a review from yjbanov November 22, 2019 23:10
@mdebbar mdebbar self-assigned this Nov 22, 2019
@auto-assign auto-assign bot requested a review from jason-simmons November 22, 2019 23:10
@GaryQian GaryQian self-requested a review November 22, 2019 23:18
final int endWithoutSpace = _excludeTrailing(
_text,
_lineStart,
lineEnd,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, since newlines are a subset of whitespace, we could pass endWithoutNewlines here instead of lineEnd, and avoid scanning from the end twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Done

@required int start,
@required int end,
}) {
assert(0 <= start && start < end && end <= _text.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: when this assert fails it will say that 0 <= start && start < end && end <= _text.length evaluated to false. I think breaking it up into separate asserts - assert(0 <= start); assert(start < end); assert(end <= _text.length) - would provide a more actionable error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also applied the same change to few other asserts in this file.


@override
String toString() {
return 'EngineLineMetrics(lineNumber: $lineNumber, text: "$text", hardBreak: $hardBreak, width: $width)';
Copy link
Contributor

Choose a reason for hiding this comment

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

This string could show up in user code (there was a recent test breakage when some web class went out of sync). Let's make sure we're consistent with the C++ version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only useful for tests now, so I'll hide it behind assertionsEnabled. The C++ version doesn't implement it. It just return the default Instance of 'LineMetrics'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add the same implementation in the C++ version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the method.

@mdebbar mdebbar requested a review from yjbanov November 27, 2019 19:51
@mdebbar mdebbar merged commit bc07e6a into flutter:master Nov 27, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 28, 2019
skia-flutter-autoroll pushed a commit to flutter/flutter that referenced this pull request Nov 28, 2019
* 29b2e91 Include uncommitted files when checking for code format issues. (flutter/engine#14035)

* 7a7118d Roll src/third_party/skia 861ac61cb1a9..c96f5108df28 (10 commits) (flutter/engine#14059)

* bc07e6a [web] Initial support for LineMetrics (flutter/engine#13985)

* bcb8267 Revert "Do not default to downstream affinity on iOS insertText (#13852)" (flutter/engine#14053)

* 4bdd15c more tests for enabling semantics (flutter/engine#14060)
@mdebbar mdebbar deleted the line_metrics branch April 15, 2021 17:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[web] Implement EngineParagraph.computeLineMetrics [web] implement Paragraph.longestLine

3 participants