Add line boundary information to LineMetrics. #13727
Conversation
gspencergoog
force-pushed
the
add_line_boundary
branch
from
2b4c58f to
31f7bf8
Compare
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
Web changes look good to me as long as regular text field functionality doesn't regress on flutter web.
|
cc @Hixie The reason we did not include this in the initial version of LineMetrics is due to likely future changes away from utf16 codepoints to represent chars, instead relying on dart's representation. I'd run this through Ian first. |
|
cc @a-siva What is the status of any work on a new dart-string/unicode system? |
|
I don't think we should add this yet, because we don't want to be adding yet more APIs that add to our String API technical debt. Specifically, this API as defined would mean that this uses a different meaning for indices into strings than does String.indexOf et al. But, if we were to make it the same, it would mean that we were using UTF-16 words as indices in this API, which is a terrible technical choice. |
|
(If we did want to land this, we should make sure to implement it on Web before landing. We are at a point where it is no longer OK to only implement things on some platforms.) |
|
OK, but this will mean that we don't have line selection or line-by-line cursor movement in desktop keyboard editing. I'll remove that capability from flutter/flutter#44130 and file a bug to implement it someday when we have real line metrics. NB: I did talk to Mouad to ask if I needed to implement it on web, and we decided that I didn't, because this API won't be used on the web anyhow, since text field editing is handled by the browser. |
|
Actually I spoke with Ian, and we have an idea of how to implement this without adding as much technical debt, so reopening. |
|
Ok well we do need line-by-line cursor movement. Greg and I spoke in person. I suggested that we move TextRange from services to dart:ui and return that from the relevant APIs here. That way, we reduce this to a previously unsolved problem rather than adding any new technical debt. Also, we should test that this is all working in UTF-16 code units and not Unicode code points. |
|
I'll make a separate PR for moving TextRange into dart:ui, since that's a breaking change. |
|
Here's the PR for moving |
gspencergoog
force-pushed
the
add_line_boundary
branch
from
c001f10 to
a6811ed
Compare
gspencergoog
force-pushed
the
add_line_boundary
branch
from
a6811ed to
cf915bb
Compare
gspencergoog
force-pushed
the
add_line_boundary
branch
from
cf915bb to
55b56a1
Compare
| final List<int> boundary = _getLineBoundary(position.offset); | ||
| return TextRange(start: boundary[0], end: boundary[1]); | ||
| } | ||
| List<int> _getLineBoundary(int offset) native 'Paragraph_getLineBoundary'; |
There was a problem hiding this comment.
Is there a reason this is implemented in C++ and not in Dart directly? (Implementing it in Dart would reduce unnecessary work since web can also use the same implementation).
There was a problem hiding this comment.
I don't have the history on why that is, but I assume the reason is that we didn't want to have to implement, test, and maintain the code, and it already existed in the text library. If that's the case, then I suspect that with the advent of web, it makes sense to make the investment and implement, test, and maintain it (since we'll need to do that anyhow for web).
It's possible that it is somehow linked to the text processing library that we use, though, so I'll defer to the engine team on that.
There was a problem hiding this comment.
In this case, it would be far more efficient to compute it in C++ instead of serializing and passing all the LineMetrics to dart, decoding, and processing it there.
This also keeps implementations consistent throughout the Paragraph API. Methods such as GetRectsForRange requires a significant computation that can only be done at engine level, and splitting implementation between dart and C++ would make it harder to maintain.
gspencergoog
force-pushed
the
add_line_boundary
branch
from
55b56a1 to
48502a1
Compare
Convert the call to getWordBoundary to use a TextPosition, in preparation for landing flutter/engine#13727, which switches the desired API to the final desired API.
| /// | ||
| /// The newline (if any) is returned as part of the range. | ||
| /// | ||
| /// This can potentially be expensive, since it needs to compute the line |
There was a problem hiding this comment.
The line metrics are computed as part of layout, so as long as it is used after layout, this should not be particularly expensive. We currently don't do any additional computation beyond that of layout to calculate the LineMetrics.
What should be mentioned though is that this is invalid before layout.
There was a problem hiding this comment.
Actually, this warning can remain, as it depends on the implementation. Web or SkShaper may have more expensive implementations. It just so happens LibTxt computes this in layout.
5 participants
This exposes the line boundary information a line by adding
getLineBoundaryto return the indices corresponding to a line around aTextPosition. The information is already calculated when calculating line metrics, so that we can enable moving the selection/cursor to the beginning/end of a line in a text field.