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

[web] Implement Paragraph.getLineBoundary #14479

Merged
mdebbar merged 3 commits into from
Dec 13, 2019
Merged

[web] Implement Paragraph.getLineBoundary #14479

mdebbar merged 3 commits into from
Dec 13, 2019

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Dec 13, 2019

@mdebbar mdebbar added the platform-web Code specifically for the web engine label Dec 13, 2019
@mdebbar mdebbar self-assigned this Dec 13, 2019
@auto-assign auto-assign bot requested a review from cbracken December 13, 2019 18:14
gspencergoog
gspencergoog approved these changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d
(modulo test changes)

lib/web_ui/test/paragraph_test.dart

// "One single line".length == 15
for (int i = 0; i < 15; i++) {
expect(
Copy link
Contributor

@gspencergoog gspencergoog Dec 13, 2019

Choose a reason for hiding this comment

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

Add a reason: to this expect that tells what i is, so that it's easier to debug.

For example:
reason: 'failed at offset $i'

Copy link
Contributor Author

@mdebbar mdebbar Dec 13, 2019

Choose a reason for hiding this comment

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

Nice pro-tip! Thanks I'll add it.

lib/web_ui/test/paragraph_test.dart

// "First line\n".length == 11
for (int i = 0; i < 11; i++) {
expect(
Copy link
Contributor

@gspencergoog gspencergoog Dec 13, 2019

Choose a reason for hiding this comment

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

Add a reason:

lib/web_ui/test/paragraph_test.dart

// "Second line\n".length == 12
for (int i = 11; i < 23; i++) {
expect(
Copy link
Contributor

@gspencergoog gspencergoog Dec 13, 2019

Choose a reason for hiding this comment

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

Add a reason:

lib/web_ui/test/paragraph_test.dart

// "Third line".length == 10
for (int i = 23; i < 33; i++) {
expect(
Copy link
Contributor

@gspencergoog gspencergoog Dec 13, 2019

Choose a reason for hiding this comment

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

Add a reason:

yjbanov
yjbanov approved these changes Dec 13, 2019
View reviewed changes
lib/web_ui/lib/src/engine/text/paragraph.dart
if (lines != null) {
final int offset = position.offset;

for (int i = 0; i < lines.length; i++) {
Copy link
Contributor

@yjbanov yjbanov Dec 13, 2019

Choose a reason for hiding this comment

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

Would binary search be overkill in this case?

Copy link
Contributor Author

@mdebbar mdebbar Dec 13, 2019

Choose a reason for hiding this comment

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

Yes :)

Most paragraphs are 1-4 lines.

lib/web_ui/lib/src/engine/text/paragraph.dart Outdated
/// The index in the text where this line begins.
final int startIndex;

/// The index in the text where this line ends.
Copy link
Contributor

@yjbanov yjbanov Dec 13, 2019

Choose a reason for hiding this comment

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

Age-old question: is that the index of the last character or +1 from that? :)

Copy link
Contributor Author

@mdebbar mdebbar Dec 13, 2019

Choose a reason for hiding this comment

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

It's [start, end[
i.e. start is inclusive, end is exclusive.

I'll clarify it in the comment.

@mdebbar mdebbar merged commit 35adf37 into flutter:master Dec 13, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 17, 2019
@engine-flutter-autoroll
35adf37 [web] Implement Paragraph.getLineBoundary (flutter/engine#14479)
dc3bc87
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 17, 2019
Merged
iskakaushik pushed a commit to flutter/flutter that referenced this pull request Dec 18, 2019
@engine-flutter-autoroll @iskakaushik
Roll engine bdd4e4d..500f9ce (34 commits) (#47223)
b3e14cc 
* c260057 Roll src/third_party/dart fe9f30c3896e..8ad8abfb7463 (19 commits) (flutter/engine#14480)

* 47c1dde Roll src/third_party/skia d0840ecf5831..ea47b0e65802 (9 commits) (flutter/engine#14481)

* 35adf37 [web] Implement Paragraph.getLineBoundary (flutter/engine#14479)

* 00cbfd3 [web] Remove Paragraph._lines which isn't necessary anymore (flutter/engine#14485)

* d6172fd libtxt: cache fallback fonts found by a Minikin font collection (flutter/engine#14482)

* 1912478 adding firefox unit tests to font loading (flutter/engine#14487)

* 6c71f89 Introduce an auto-deletable SkiaObject; make SkPaint a SkiaObject (flutter/engine#14486)

* 2026b83 Roll src/third_party/dart 8ad8abfb7463..83eeab1e63d9 (13 commits) (flutter/engine#14488)

* b43d3fe Roll src/third_party/skia ea47b0e65802..cd9ad409a90d (7 commits) (flutter/engine#14490)

* 3c85b82 Roll fuchsia/sdk/core/linux-amd64 from QZVxo... to q0tup... (flutter/engine#14494)

* 3e3eb9c Roll src/third_party/dart 83eeab1e63d9..6c99171686be (5 commits) (flutter/engine#14492)

* cbe0e6c Roll src/third_party/skia cd9ad409a90d..cf0e3c63fd22 (1 commits) (flutter/engine#14495)

* a653bd8 Roll fuchsia/sdk/core/mac-amd64 from 5t09i... to esDH2... (flutter/engine#14493)

* 17d8882 Roll src/third_party/dart 6c99171686be..09a0b040dd9b (1 commits) (flutter/engine#14497)

* be9c7c9 Roll src/third_party/skia cf0e3c63fd22..9e7199561fc8 (1 commits) (flutter/engine#14499)

* c504b8f Roll src/third_party/skia 9e7199561fc8..ef363a9ce692 (1 commits) (flutter/engine#14500)

* cf20f87 Roll src/third_party/dart 09a0b040dd9b..ae3973da4023 (1 commits) (flutter/engine#14501)

* bb3166b Roll src/third_party/dart ae3973da4023..24b99838f433 (3 commits) (flutter/engine#14502)

* 87c1110 Roll src/third_party/skia ef363a9ce692..187cd367d388 (1 commits) (flutter/engine#14503)

* 45c6c3a Roll src/third_party/skia 187cd367d388..706eb5788e8b (1 commits) (flutter/engine#14505)

* d292380 Fix comments of iOS status bar height constant (flutter/engine#14496)

* 1ed7247 Roll src/third_party/dart 24b99838f433..2e1dd98e5e7f (1 commits) (flutter/engine#14507)

* 8712461 Allow custom embedders to post low memory notifications. (flutter/engine#14506)

* 1d1eae2 Roll fuchsia/sdk/core/linux-amd64 from q0tup... to O6ELR... (flutter/engine#14512)

* 8b47886 Roll src/third_party/dart c5c469088697..44ba80053396 (1 commits) (flutter/engine#14510)

* ca799fa Roll src/third_party/skia 706eb5788e8b..9b59953e77b9 (3 commits) (flutter/engine#14511)

* 8456411 Roll src/third_party/dart 44ba80053396..2eba06d26f71 (32 commits) (flutter/engine#14515)

* 46d76eb Roll src/third_party/skia 9b59953e77b9..d78a9b45b9e5 (23 commits) (flutter/engine#14516)

* 5f9e558 Forward low memory notifications from the shell to DartVM. (flutter/engine#14517)

* dc59758 Apply SmoothPointerDataDispatcher to Fuchsia (flutter/engine#14514)

* 9556f2c Roll src/third_party/dart 2eba06d26f71..b1afe2d0ebc1 (6 commits) (flutter/engine#14523)

* e829384 Roll fuchsia/sdk/core/linux-amd64 from O6ELR... to ZHhOi... (flutter/engine#14525)

* 0fbdeb8 Roll src/third_party/dart b1afe2d0ebc1..2d332ee1d3f1 (1 commits) (flutter/engine#14526)

* 500f9ce Roll src/third_party/skia 6153165d78f2..fdb2b7d53038 (2 commits) (flutter/engine#14528)
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
@mdebbar @filmil
[web] Implement Paragraph.getLineBoundary (flutter#14479)
2bf7f62
@mdebbar mdebbar deleted the line_boundary branch April 15, 2021 17:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cbracken cbracken Awaiting requested review from cbracken

2 more reviewers

@yjbanov yjbanov yjbanov approved these changes

@gspencergoog gspencergoog gspencergoog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mdebbar mdebbar

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[web] Implement getLineBoundary()

4 participants

@mdebbar @yjbanov @gspencergoog @googlebot