Expose text boundary combiner class #112085

chunhtai · 2022-09-21T16:37:57Z

More public classes

fixes #111751

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

packages/flutter/lib/src/services/text_boundary.dart

LongCatIsLooong · 2022-09-22T00:46:06Z

packages/flutter/lib/src/services/text_boundary.dart

+      return position;
+    }
+
+    for (index -= 1; index >= 0; index -= 1) {


nit: the first term index -= 1 isn't that common in for loops. Consider using something like

while ((index -= 1) >= 0)

I think it would be easier to read if there's an _effectiveTextOffset method that turns caret locations to int offsets?

while loop makes sense, I am not sure how _effectiveTextOffset should be used to implement this for loop. Can you elaborate?

Something like (I thought you added a similar function in the previous PR?):
int _toTextOffset(TextPosition position) => max(0, min(upstream ? position.offset - 1 : position.offset, textLength - 1));
Then:

int offset = _toTextOffset(position); while (offset >= 0 && !TextLayoutMetrics.isWhitespace(_text.codeUnitAt(offset)) { offset -= 1; } return Offset(offset: offset + 1, affinity: offset < 0 ? upstream : downstream); // Is the affinity correct?

The reason I don't want to change the the position to text offset before the first

if (position.affinity == TextAffinity.downstream && !TextLayoutMetrics.isWhitespace(_text.codeUnitAt(index))) { return position; }

is that I need to preserve the affinity of the input if the input already point to a non whitespace character.

An example lets say you have string without any white space 'abcd'.

The expected behavior is the following:

getLeadingTextBoundaryAt(TextPosition(3, upstream)) = TextPosition(3, upstream) getLeadingTextBoundaryAt(TextPosition(2, downstream)) = TextPosition(2, downstream)

however your code will produce

getLeadingTextBoundaryAt(TextPosition(3, upstream)) = TextPosition(3, upstream) getLeadingTextBoundaryAt(TextPosition(2, downstream)) = TextPosition(3, upstream)

The problem is you can't tell between TextPosition(3, upstream) and TextPosition(2, downstream) because they are the same in text offset. If you convert it first before the return, you can't differentiate them.

I could do the conversion after the first return, but at that point, I don't care about affinity anymore because I only need to find first non whitespace character and do (index+1 upstream).

LongCatIsLooong · 2022-09-22T00:51:23Z

packages/flutter/lib/src/services/text_boundary.dart

+
+  @override
+  TextPosition getLeadingTextBoundaryAt(TextPosition position) {
+    // Handles outside of right bound.


nit: left -> leading/start, right -> trailing/end. The TextBoundary class does not know the directionality of the text.

Also I don't remember why we need to clamp here, doesn't seem to be the case for other text boundary classes?

I did this for character and this because these require manually calculation on the string, the others call into textlayoutmetrics, which handle the boundary check. we could do this for all text boundaries regardless though, WDYT?

Shouldn't most text boundaries assert the incoming position is within the document range?

The PushTextPosition may potentially push out side of the bound, unless I add a API to query text length in TextBoundary, I can't do perform clamping there. I think it is not worth it though.

packages/flutter/lib/src/services/text_boundary.dart

LongCatIsLooong · 2022-09-22T01:41:26Z

packages/flutter/lib/src/services/text_boundary.dart

+    return inner.getTrailingTextBoundaryAt(
+      outer.getTrailingTextBoundaryAt(position),
+    );
+  }


Consider overridding

TextRange getTextBoundaryAt(TextPosition position)

too.

This method exists because it's sometimes slightly cheaper to query both the leading + trailing than to query them separately. (for example, the low-level line boundary call on ui.Paragraph actually gives you both offsets so there's no need to call that twice to get both the leading boundary and trailing boundary). That probably should have been documented...

Ah LineBreak didn't override that. Could you add that?

added line break and wordboundary override, but I can't find a meaningful implementation of ExpandedTextBoundary.getTextBoundaryAt since the return value doesnot contain affinity. I can't use the return value of the outer textboundary to call inner textboundary.

That's sad. Can we assume the TextRange's start position is always downstream and end position upstream?

Well if the TextRange is collapsed we'll lose information. Maybe we should make TextPosition a subclass of TextRange?

That's sad. Can we assume the TextRange's start position is always downstream and end position upstream?

That is not the case for whitespace text boundary.

abc def the position at the whitespace in the middle will return
leading = TextPosition(3, upstream)
trailing = TextPosition(4, downstream)

I think TextRange.start and TextRange.end should be a TextPosition instead of int

packages/flutter/lib/src/services/text_boundary.dart

LongCatIsLooong · 2022-09-23T01:22:41Z

packages/flutter/lib/src/services/text_boundary.dart

+/// by one with its affinity sets to upstream. For example,
+/// `TextPosition(1, upstream)` becomes `TextPosition(1, downstream)`,
+/// `TextPosition(4, downstream)` becomes `TextPosition(5, upstream)`.
+class PushTextPosition extends TextBoundary {


Would it be better to use TextBoundary composition instead of taking a TextBoundary explicitly? With the former say we create 2 constants PreviousCaretLocation and NextCaretLocation,

So today's

_PushTextPosition(mixedBoundary, intent.forward);

will be equivalent to

(intent.forward ? TextBoundary.NextCaretLocation : TextBoundary.PreviousCaretLocation) + mixedBoundary;

or

_PushTextPosition(info.forward) + mixedBoundary;

The order of operations will be clearer (from left to right, with _PushTextPosition it's not immediately clear if mixedBoundary will be applied first or push will be applied first).

I find it hard to understand than the current code

PushTextPosition( WhiteSpaceBoundary() + WordBoundary(), true)
will become
(WordBoundary()+ WordBoundary).nextCaretLocation where the nextCaretLocation is from right to left.

I'm not suggesting making that a getter. Your example will be equivalent to NextCaretLocation + WhiteSpaceBoundary + WordBoundary

LongCatIsLooong · 2022-09-23T01:42:13Z

packages/flutter/lib/src/services/text_boundary.dart

+    return inner.getTrailingTextBoundaryAt(
+      outer.getTrailingTextBoundaryAt(position),
+    );
+  }


That's sad. Can we assume the TextRange's start position is always downstream and end position upstream?

LongCatIsLooong · 2022-09-23T01:43:14Z

packages/flutter/lib/src/services/text_boundary.dart

+    return inner.getTrailingTextBoundaryAt(
+      outer.getTrailingTextBoundaryAt(position),
+    );
+  }


Well if the TextRange is collapsed we'll lose information. Maybe we should make TextPosition a subclass of TextRange?

LongCatIsLooong

LGTM. IMO it's better to make _PushTextPosition a regular TextBoundary so you could do _PushTextPosition + Whitespace + WordBoundary, instead of letting it handle composition itself.

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Sep 21, 2022

chunhtai changed the title ~~Text boundary part ii!~~ Expose text boundary combiner class Sep 21, 2022

chunhtai marked this pull request as ready for review September 21, 2022 16:50

chunhtai requested a review from LongCatIsLooong September 21, 2022 16:50

chunhtai force-pushed the text-boundary-partII! branch from fd3d18f to 2c54b83 Compare September 21, 2022 17:46

LongCatIsLooong reviewed Sep 22, 2022

View reviewed changes

chunhtai force-pushed the text-boundary-partII! branch from cc5ac7e to 637856d Compare September 22, 2022 18:43

chunhtai requested a review from LongCatIsLooong September 22, 2022 18:43

LongCatIsLooong reviewed Sep 23, 2022

View reviewed changes

chunhtai requested a review from LongCatIsLooong September 23, 2022 18:04

LongCatIsLooong approved these changes Sep 26, 2022

View reviewed changes

chunhtai added 9 commits September 26, 2022 10:46

Expose text boundary combiner class

48ada2f

comment

bbf7a98

update

a57b122

fix doc

7b405c2

addressing comment

2947232

fix doc

6ff9ffd

addressing comments

f9a5de0

analyzer

d9c48ed

Makes PushTextPosition a standalone text boundart

0f54bd9

chunhtai force-pushed the text-boundary-partII! branch from f2c5893 to 0f54bd9 Compare September 26, 2022 17:53

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022

auto-submit bot merged commit 50cd737 into flutter:master Sep 26, 2022

chunhtai mentioned this pull request Sep 26, 2022

Attempting to delete whole words throws error #112334

Closed

engine-flutter-autoroll mentioned this pull request Sep 27, 2022

Roll Flutter from 7b6074fbc542 to f57c2fe8ff49 (23 revisions) flutter/plugins#6502

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 27, 2022

50cd737 Expose text boundary combiner class (flutter/flutter#112085)

38ec4ee

engine-flutter-autoroll mentioned this pull request Sep 27, 2022

Roll Flutter from 7b6074fbc542 to f57c2fe8ff49 (23 revisions) flutter/packages#2651

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2022

50cd737 Expose text boundary combiner class (flutter/flutter#112085)

6165867

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose text boundary combiner class #112085

Expose text boundary combiner class #112085

chunhtai commented Sep 21, 2022 •

edited

Loading

LongCatIsLooong Sep 22, 2022

LongCatIsLooong Sep 22, 2022

chunhtai Sep 22, 2022

LongCatIsLooong Sep 23, 2022

chunhtai Sep 23, 2022 •

edited

Loading

LongCatIsLooong Sep 22, 2022

LongCatIsLooong Sep 22, 2022

chunhtai Sep 22, 2022

LongCatIsLooong Sep 23, 2022

chunhtai Sep 23, 2022

LongCatIsLooong Sep 22, 2022

LongCatIsLooong Sep 22, 2022

chunhtai Sep 22, 2022

LongCatIsLooong Sep 23, 2022

LongCatIsLooong Sep 23, 2022

chunhtai Sep 23, 2022

chunhtai Sep 23, 2022

LongCatIsLooong Sep 23, 2022

chunhtai Sep 23, 2022

LongCatIsLooong Sep 23, 2022

LongCatIsLooong Sep 23, 2022

LongCatIsLooong Sep 23, 2022

LongCatIsLooong left a comment

Expose text boundary combiner class #112085

Expose text boundary combiner class #112085

Conversation

chunhtai commented Sep 21, 2022 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong left a comment

Choose a reason for hiding this comment

chunhtai commented Sep 21, 2022 •

edited

Loading

chunhtai Sep 23, 2022 •

edited

Loading