Text Editing Model Refactor #86736
Text Editing Model Refactor #86736
Conversation
…instead of in DefaultTextEditingActions
flutter-dashboard
bot
added
the
framework
| @@ -653,110 +643,6 @@ class RawFloatingCursorPoint { | |||
| final FloatingCursorDragState state; | |||
| } | |||
|
|
|||
| /// The current text, selection, and composing state for editing a run of text. | |||
| @immutable | |||
| class TextEditingValue { | |||
There was a problem hiding this comment.
TextEditingValue was moved to its own file in order to not pollute this one, now that it's much larger.
| @@ -77,47 +77,6 @@ class TextSelectionPoint { | |||
| } | |||
| } | |||
|
|
|||
| // Check if the given code unit is a white space or separator | |||
There was a problem hiding this comment.
This file's diff is really hard to read, but basically what happened was:
- Lots of logic moved to TextEditingAction, TextEditingValue, and TextSelection.
- Some alphabetizing/reordering may have happened...
- TextMetrics was implemented.
- debugAssertLayoutUpToDate was turned into a function and deduplicated.
justinmc
force-pushed
the
text-editing-model
branch
from
fc1e306
to
d91066d
Compare
justinmc
force-pushed
the
text-editing-model
branch
from
afb5775
to
25202a7
Compare
|
@LongCatIsLooong Ready for re-review. |
justinmc
force-pushed
the
text-editing-model
branch
from
be60d5a
to
8ada996
Compare
| void selectAll(SelectionChangedCause cause) { | ||
| textSelectionDelegate.selectAll(cause); | ||
| /// Returns the TextPosition above or below the given offset. | ||
| TextPosition getTextPositionVertical(TextPosition position, double verticalOffset) { |
There was a problem hiding this comment.
This should be private I think?
| } | ||
|
|
||
| /// Return the selection collapsed and moved to the given [TextPosition]. | ||
| TextSelection moveTo(TextPosition position) { |
There was a problem hiding this comment.
Is this just TextSelection.fromPosition, except this preserves the isDirectional property?
There was a problem hiding this comment.
Good point, which as we've discussed doesn't seem to be relevant to this. I'll swap in fromPosition and get rid of this method.
| /// | ||
| /// Setting includeWhitespace to false will only return the index of non-space | ||
| /// characters. | ||
| static int nextCharacter(int index, String string, [bool includeWhitespace = true]) { |
There was a problem hiding this comment.
The CharacterRange.at constructor does handle that case according to its documentation https://master-api.flutter.dev/flutter/characters/CharacterRange/CharacterRange.at.html, and its implementation seems to be more efficient than iterating through the whole document:
https://github.com/dart-lang/characters/blob/e7406a17e1318e702b824d0f47db131349ff9ee0/lib/src/characters_impl.dart#L447-L452
convert to using Characters everywhere
I think one of the current road blocker is that both our text layout libs and most of our platform input plugins only take UTF16 code points so converting the indices (efficiently) is a bit of a problem.
| /// | ||
| /// Includes newline characters from ASCII and separators from the | ||
| /// [unicode separator category](https://www.compart.com/en/unicode/category/Zs) | ||
| static bool isWhitespace(int codeUnit) { |
There was a problem hiding this comment.
This seems to be a more reasonable place to place the utility method. But I wonder if it's possible to not expose this (at least not until we expose ICU methods) for now?
There was a problem hiding this comment.
It looks like it's used in 3 different files now unfortunately (RenderEditable, TextEditingValue, TextEditingActionTarget). Probably not worth it to duplicate the method three times.
I see @gspencergoog's comment about this:
// TODO(gspencergoog): replace when we expose this ICU information.
Any idea if we still have plans to expose that, or if there's a cleaner way for me to expose this one?
There was a problem hiding this comment.
I don't know if we plan to expose it, but I think the problem was that it adds to executable size too much to add the tables in that ICU uses.
| /// {@template flutter.services.TextMetrics.getLineAtOffset} | ||
| /// Return a [TextSelection] containing the line of the given [TextPosition]. | ||
| /// {@endtemplate} | ||
| TextSelection getLineAtOffset(String text, TextPosition position); |
There was a problem hiding this comment.
IMO this is going to be error prone. TextMetrics's layout information is highly dependent on its text, so being able to provide a potentially different String is a bit confusing.
enforce what text it's using
Yeah but we can't layout a plain String. It has to be styled and you need other inputs for instance the incoming box constraints. Also other methods in the interface don't seem to take a String? All TextMetrics implementations are likely going to have states.
And for RenderEditable.getLineAtOffset, getting the length of the obscured text should be an implementation detail instead of being exposed as a parameter I think.
| } | ||
| /// The current text, selection, and composing state for editing a run of text. | ||
| @immutable | ||
| class TextEditingValue { |
There was a problem hiding this comment.
Yeah the android Editable implementation implements most common text manipulation methods in terms of replace, since it's replace implementation takes care of the selection and composition.
…er similar methods
… replace method will follow in another PR
This PR aims to improve the separation of concerns of our text editing classes.
Design doc: https://docs.google.com/document/d/1lkpg4dfVZif9NJ_dDQ01QkwQC3OqzhUFahhjgv_rcbM/edit#
Related: #80226
Summary
Reviewing
Sorry this is such a huge diff! It's all moving code around and refactoring it; very little totally new. You can skim most of it because of this. I tried to leave helpful comments where big chunks of code were moved. Let me know if I can explain anything.
My commits are also pretty messy as I explored ideas and changed approaches 😁