Make FilteringTextInputFormatter's filtering Selection/Composing Region agnostic
#89327
Conversation
772dc54 to
435f45f
Compare
435f45f to
9b0b2ef
Compare
…gion agnostic - Fixes flutter#80842 - Fixes flutter#87764
9b0b2ef to
b845ae7
Compare
justinmc
left a comment
There was a problem hiding this comment.
LGTM, assuming that no one is relying on the existing behavior for any good reason. I can't think of why they would be. And sorry it took me so long to get to this!
| /// The filter may adjust the selection and the composing region of the text | ||
| /// after applying the text replacement, such that they still cover the same | ||
| /// text. For instance, if the pattern was `o+` and the last character "s" was | ||
| /// selected: "Into The Wood|s|", then the result will be "Into The W*|s|", |
There was a problem hiding this comment.
Should the result actually be this, or am I misunderstanding? "Int* The W*d|s|"
| /// | ||
| /// Additionally, each segment of the string before, during, and | ||
| /// after the current selection in the [TextEditingValue] is handled | ||
| /// separately. This means that, in the case of the "Into the Woods" |
There was a problem hiding this comment.
Do you know why this was done in the first place? Was it just because it was easier? Talking about separating before/during/after selection. I agree that it seems much more useful the new way.
There was a problem hiding this comment.
No idea. It's still a hard choice to make when an index is strictly within the replace range. The old implementation works in most cases I think.
| /// If the current [selection] has a negative `baseOffset` or `extentOffset`, | ||
| /// then the text currently does not have a selection or a caret location, and | ||
| /// most text editing operations that rely on the current selection (for | ||
| /// instance, insert a character at the caret location) will do nothing. |
There was a problem hiding this comment.
Thank you for documenting these quirks 🙏
| /// user via interacting with the IME. | ||
| /// | ||
| /// If the range represented by this property is [TextRange.empty], then the | ||
| /// text is not currently being composed. |
There was a problem hiding this comment.
Thanks for fixing my issue about a lack of docs here. Some users had no idea what this was for.
| /// The start index of the range, inclusive. | ||
| /// | ||
| /// The value of [base] should always be greater than or equal to 0, and can | ||
| /// be larger than, smaller than, or equal to [extent]. |
There was a problem hiding this comment.
Do we need to worry about inverted selection?
There was a problem hiding this comment.
Oh right I think I just copied the documentation over. No I think the indices are processed separately and it doesn't care if base > extent or not. I'll add a test for this.
There was a problem hiding this comment.
Oh nvm the documentation was right, it works fine with inverted selections. Many test cases have the selection range inverted.
| expect( | ||
| FilteringTextInputFormatter(RegExp('o+'), allow: false, replacementString: '*').formatEditUpdate(testOldValue, selectedIntoTheWoods), | ||
| const TextEditingValue(text: 'Int* the W**ds', selection: TextSelection(baseOffset: 11, extentOffset: 14)), | ||
| const TextEditingValue(text: 'Int* the W*ds', selection: TextSelection(baseOffset: 11, extentOffset: 13)), |
There was a problem hiding this comment.
Is this behavior change part of the fix?
There was a problem hiding this comment.
Yeah the double "o" was separated by the caret so the filtering was applied separately. Hopefully nobody is relying on this behavior.
replace by string of the same length.
6e8ae99 to
41c0249
Compare
41c0249 to
a3546e4
Compare
…ing Region agnostic" flutter#89327 (flutter#90211)
FilteringTextInputFormatter's output text is selection dependent #80842Removed the additional interfaces added from the original (closed) PR.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.