-
Notifications
You must be signed in to change notification settings - Fork 6k
[web] Initial support for LineMetrics #13985
Conversation
| final int endWithoutSpace = _excludeTrailing( | ||
| _text, | ||
| _lineStart, | ||
| lineEnd, |
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, since newlines are a subset of whitespace, we could pass endWithoutNewlines here instead of lineEnd, and avoid scanning from the end twice.
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.
Great idea. Done
| @required int start, | ||
| @required int end, | ||
| }) { | ||
| assert(0 <= start && start < end && end <= _text.length); |
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.
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.
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.
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)'; |
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.
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.
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.
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'.
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.
Maybe add the same implementation in the C++ version?
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.
Removed the method.
fa4d1fb to
09b752d
Compare
* 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)
Part of flutter/flutter#33523
Fixes flutter/flutter#33613
Fixes flutter/flutter#39537