Avoid using TextAffinity in TextBoundary

[LongCatIsLooong]

• Changes the method signatures of the TextBoundary class from TextPosition -> TextPosition to Int -> Int?. This is more in line with the BreakIterator interface that ICU uses and easier to implement. Also TextAffinity/TextPosition probably shouldn't be used in lower-level APIs.
• Changes most "move selection" operations to place the handle at downstream affinity. Only "move to line boundary" shortcuts and tap gestures move the caret or the targeted selection handle to the upstream position.
• Removed Whitespace boundary and PushText and the + operator since they're no longer used in the framework.

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.

[LongCatIsLooong]

@chunhtai I think we should file a bug and skip this test on the HTML renderer instead?

[chunhtai]

yeah I think it make more sense to skip it. but do you know why it was passing for html render before?

[chunhtai]

Is the goal to only use character position in the framework? I originally include affinity in the textboundary because some part of the framework use character position and some part uses caret position. There were workaround converting character to/from caret by +1 and -1 when the data flowing through various API, and some of them create weird corner cases. I figured it would be cleaner if everything is in caret position.

[LongCatIsLooong]

At least for text boundary analysis it's much easier to reason about with code unit indices. It has fewer corner cases (the only thing I noticed was hard line breaks for line boundaries) and we're not losing information. RenderEditable still needs to know what the affinity is but I think ideally we shouldn't have affinity in dart:ui and lower-level layers.

[justinmc]

I guess the only thing you're using from this class is range, right? You could almost take that as a parameter and put these somewhere as static methods... I guess you might have bigger plans for organizing these in the long term, though?

[LongCatIsLooong]

yeah, probably don't need affinity here at all. Removed the comment.

[chunhtai]

I don't have strong opinion whether we should handle everything in character position or caret position in framework as long as they are consistent. I left some question but mostly just styling nits

[chunhtai]

Can you think of use case other than skipping whitespace? maybe just hard code a skipWhiteSpace method so that you don't need to duplicate code on both renderparagraph and rendereditable? WDYT

[LongCatIsLooong]

unfortunately this can't be done right now, WordBoundary itself doesn't have access to code units

[chunhtai]

Do you think it worth adding access to the code unit for word boundary?

[LongCatIsLooong]

I can move this class to TextPainter so we have random access to the code units

[chunhtai]

It seems weird that the base class creates a circular dependencies between getTrailingTextBoundaryAt/getLeadingTextBoundaryAt and getTextBoundaryAt.

I think we should leave one empty or throw if called without implementation.

[chunhtai]

we can also consider leaving getTextBoundaryAt private in this file since it is not used elsewhere

[LongCatIsLooong]

It's documented that the minimal complete definition must contain getTextBoundaryAt, or both getLeading/TrailingTextBoundaryAt. Failing to do so will result in a stack overflow exception being thrown.

[LongCatIsLooong]

I think getTextBoundaryAt should be used for double tap, but right now it's using RenderEditable.selectWordsInRange.

[LongCatIsLooong]

We can potentially put in an assert to check if the same method is called reentrantly with the same input and throw a different exception (and throw faster), but at the cost of the const constructor. But this is not a class most people are going to try extending so I feel the const constructor is more useful.

[chunhtai]

I think you can do something like this?

abstract class TextBoundary {
  /// A constant constructor to enable subclass override.
  const TextBoundary();

  int? getLeadingTextBoundaryAt(int position) {
    if (position < 0) {
      return null;
    }
    final int start = getTextBoundaryAt(position).start;
    return start >= 0 ? start : null;
  }

  int? getTrailingTextBoundaryAt(int position) {
    final int end = getTextBoundaryAt(max(0, position)).end;
    return end >= 0 ? end : null;
  }

  TextRange getTextBoundaryAt(int position) {
    throw UnimplementedError('subclass must implement getTextBoundaryAt or ...');
  }
}

Instead of giving stackoverflow if developer doesn't implement either of the method, this will throw unimplemented error instead.

[LongCatIsLooong]

This is an abstract class so if the method is left unimplemented that could be made a compile time error instead of a runtime error. But I want to allow subclass to just override either of these minimal definitions, like in some other languages you can just override either == or != for equality:

x /= y     = not (x == y)
x == y     = not (x /= y)

[chunhtai]

sgtm

[chunhtai]

nit: we should probably do a typedef.
also the variable name should be something like onCreateTextBoundary or getTextBoundary

[chunhtai]

same

[chunhtai]

yeah I think it make more sense to skip it. but do you know why it was passing for html render before?

[chunhtai]

extent means the selection end. maybe rename the parameter back to position or offset?

[chunhtai]

also the _beyondTextBoundary seems like a weird name...

[LongCatIsLooong]

renamed

[chunhtai]

nit: we may need a better name or some doc string to document what this method is actually doing.

It looks like it converts the input from caret position to character position before passing into textboundary?

[chunhtai]

also should the textposition to be store in character position in RenderParagraph instead so that we don't need to do this conversion?

[LongCatIsLooong]

I mostly just copeid these 2 from editable text. Added comments and renamed the 2 methods.

[LongCatIsLooong]

also should the textposition to be store in character position in RenderParagraph instead so that we don't need to do this conversion?

I'll probably do that in a separate PR.

[chunhtai]

can we reuse _beyondTextBoundary here?

[LongCatIsLooong]

It's different, the beyond one has a -1 somewhere.

[chunhtai]

sounds good

[chunhtai]

LGTM, felt a bit weird about initializing boundary class on textpainter. I think I like the original API better even if we have to open up until API. but not really too big of a deal.

[tgucio]

@LongCatIsLooong FWIW, Flutter engine's web_ui has a good Dart word breaker implementation with Unicode rules (except for dicts) that could be recycled to remove the dependency on TextPainter and ui.Paragraph:
https://github.com/flutter/engine/blob/main/lib/web_ui/lib/src/engine/text/word_breaker.dart

[LongCatIsLooong]

I think at some point we'll have ICU4X bindings for dart so I'm banking on that. Besides that, word boundary iterator is usually used for handling user interactions, and that happens between frames so only in very rare cases we'll need to locate word breaks without a valid text layout (e.g. when the text layout is invalidated between frames). So I'd say it's not that urgent to remove the dependency.