Repository navigation
Support block delete with word and line modifiers - #77172
Conversation
flutter-dashboard
Bot
added
the
framework
|
This is a great PR, but unfortunately you and I are going to have conflicts because of the PR you noticed, #75032. I'm in the middle of porting this RenderEditable keyboard code to use Shortcuts instead. In subsequent PRs, everything in the Would you be willing to update this PR to use Shortcuts once #75032 is merged? Most of your logic in RenderEditable could stay, but the key events would come from Shortcuts. Also your tests may need to be moved to EditableText because RenderEditable would not receive the key events any more. Sorry for the conflict and thanks for the PR! |
Sure! I'll keep an eye on that PR 👍 |
4e7bca7 to
fb177bf
Compare
|
@justinmc rebased and updated the approach for the new shortcuts approach. This whole new approach to shortcuts is looking really good! I'll be working on a custom input to compose spreadsheet-like formulas, and this will be super handy! |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Thanks for updating this so fast!
I should have realized this sooner, but there is another PR out that overlaps with this one: #77549
Let's move forward with this PR since it now uses Shortcuts, and I'll close the other one. The problem was probably that we have duplicate issues, which is my fault: #77547. I've updated your PR description to mention that issue too.
One thing I do want to take from that PR is this table of cases below. Can you make sure that all of these work properly and are tested? You might already be good.
| Before | Word Modifier + | After, according to TextEdit on Mac |
|---|---|---|
| text with| multiple words | Delete | text with| words |
| text with |multiple words | Backspace | text |multiple words |
| text with| multiple words | Delete | text with| words |
| text with |multiple words | Backspace | text |multiple words |
| text with multiple words| | Backspace | text with multiple | |
| text with multiple| words | Backspace | text with | words |
| text with multiple\n|words | Backspace | text with |words |
Overall this looks great. This is helping us to quickly get fully on board with Shortcuts, thank you!
|
I just confirmed that all the cases on your table are now working. In my last commit I had to change my approach to the following case:
I was basing my approach on some editors that just delete the immediate previous line break. |
f2d7c46 to
6128d6f
Compare
|
The behavior on Windows is generally swapped. That is,, Ctrl+Backspace does what Alt+Backspace should do, and vice versa. I went to verify what Windows does, as the Alt+Backspace combination isn't one I have ever used; however of the 6 programs I tested, there were variations between all of them. Notepad.exe:Alt+Backspace: Delete all text on the line to the left of the cursor Microsoft Word:Alt+Backspace: Undo (WTF?!) Sublime Text 3:Alt+Backspace: (Unclear, it made the alert sound) VS Code:Alt+Backspace: Delete any characters left of the cursor until a single word has been consumed, do not delete word boundaries Discord:Alt+Backspace: Delete all text on the line to the left of the cursor, except leave the first character of the line (I assume this is a bug, though) Visual Studio 2019:Alt+Backspace: Undo (kill me) Interestingly, 4 of these programs are Microsoft programs. So I basically have no idea what to recommend here in order to be the most "Windows"-like in this case. But if nothing else, swap the Alt and Ctrl behaviors. :) |
|
I'm not sure how much this is part of this PR, either, but Ctr+Delete crashes on Windows: |
|
@AberrantWolf thank you so much for your help! I really need to get a windows machine...
I need to make the same exercise on a Linux machine. |
|
Verified that the crash is fixed. We still generally need to swap the Ctrl/Alt keys on Windows still. Honestly, I'd go with just the swap, and then call it good. Notepad may not be Microsoft's current go-to, but it's the most basic equivalent to TextEdit on Mac, and it IS from Microsoft. And (interestingly to me) the gesture your hand makes ends up matching Mac OS due to Windows keyboards putting the Alt key where the Command key is on Mac. I'm happy with that as a Windows user. I'm new to the key shortcuts -- can then be overridden in userland if defined in the core API? |
|
Yes, you can override in userland. Check the design doc theres a section about overrides. |
canastro
changed the title
There was a problem hiding this comment.
nit: IIRC it's guaranteed that start <= end (but not baseOffset <= extentOffset).
There was a problem hiding this comment.
I think for RTL text "backwards" means to the right?
There was a problem hiding this comment.
From what I'm seeing on iOS when I switch to Hebrew the backspace key on the software keyboard horizontally flips ( <- => ->) so I assume on macOS it would start deleting towards the opposite direction (visually) as well?
There was a problem hiding this comment.
Yes, with an arabic keyboard the delete / backspace (and even arrows) are switched.
Check the following example:
- wrote "qwerty"
- pressed left arrow 4 times
- pressed backspace twice (delete backwards, which with the US keyboard is to the left, but in this case is to the right)
- pressed delete twice (delete forward, which with the US keyboard is to the right, but in this case is to the left)
Screen.Recording.2021-03-15.at.19.04.42.mov
There was a problem hiding this comment.
Ah Ok _getLeftByWord does search upstream. So it's just the _getLeftByWord method needs a better name.
|
Added support for obscure texts:
|
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
This is looking great. Thanks for following up and especially for all of the tests. Most of my comments are about smaller things like docs and naming.
I also tried running this locally and tried all of the examples I posted in the table and it seemed to work in every case 👍
Thanks @AberrantWolf for all of the research on Windows. I like the idea of following what Notepad does on Windows and TextEdit on Mac, in general. I'm ok with alt-backspace deleting by line and ctrl-backspace deleting by word on Windows. If any die hard Windows users complain we can consider changing it.
There was a problem hiding this comment.
Ah Ok _getLeftByWord does search upstream. So it's just the _getLeftByWord method needs a better name.
There was a problem hiding this comment.
why not querying the line range using selection.baseOffset? Then you wouldn't have to do the hard break check right?
There was a problem hiding this comment.
Sorry not sure I fully understood what you meant here
There was a problem hiding this comment.
Ah sorry. I meant is it possible to get do this instead:
final TextSelection line = _getLineAtOffset(selection.baseOffset), to avoid the final bool isPreviousCharacterBreakLine = textBefore.codeUnitAt(textBefore.length - 1) == 0x0A; check?
There was a problem hiding this comment.
Ah I think you should check the affinity of selection.baseOffset first, and call _getLineAtOffset(selection.baseOffset - 1); or _getLineAtOffset(selection.baseOffset + 1); accordingly
There was a problem hiding this comment.
If I understand correctly affinity is only relevant for non-collapsed selections, at this point in the code we're already sure the selection is collapsed. If its not collapsed we call a "deleteSelection" and just delete whatever text is within the selection.
The check for line break is just to be sure we don't delete the line break neither trigger a _setTextEditingValue without actually changing any text or selection.
Eg:
When I have the cursor at the beginning of a line:
My
|Multiline <-- collapsed selection at the start of this line
Text
If I press cmd+delete it shouldnt change anything.
There was a problem hiding this comment.
But anyway, when you get the TextPosition of a newline character I would expect the engine to always return downstream so checking affinity should still work.
There was a problem hiding this comment.
While investigating the affinity stuff I compared the way line selection and line delete are working and I found some inconsistencies between them and inconsistencies with TextEdit (which is one of the sources we've been using to check the expected behaviour).
- Place cursor at the start of a line without line breaks:
- Selection: It selects the previous line
- Delete: it deletes the previous line
- TextEdit: it doesn't delete or select the previous line
- Place cursor at the start of a line with line breaks:
- Selection: It selects the previous line
- Delete: it doesn't delete anything
- TextEdit: it doesn't delete or select the previous line
Here is a confusing recording where I try to show what I mean:
https://user-images.githubusercontent.com/1267623/112716882-e625fd80-8ee0-11eb-8083-0ee768a7e414.mov
What should we do in the context of this PR. Make delete consistent with Selection or with TextEdit?
cc @justinmc
There was a problem hiding this comment.
Good catch! I've opened an issue for the selection problem. It should match TextEdit. #79291
I think you should ideally make delete do the same behavior as TextEdit.
However, if that requires you to change some internal methods in RenderEditable that would also affect selection, then we can put it off until another PR and solve both delete and selection at the same time later, after this PR.
So it's up to you, but the goal is definitely to match TextEdit in all cases.
There was a problem hiding this comment.
Yeah I think it would be better to have a separated PR dedicated to tackle this particular issue in both selections and delete actions. To make sure we reuse as most code as possible and make the PR easy to review.
Is it ok?
There was a problem hiding this comment.
Yes, I'm on board with that!
870f259 to
c44b9ad
Compare
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I think there is an inconsistency with whether or not _setTextEditingValue will be called when the text doesn't change. See my comment below.
Otherwise I think this PR is almost ready to go. Thanks for following up with all of our feedback!
| /// | ||
| /// {@macro flutter.rendering.RenderEditable.cause} | ||
| /// | ||
| /// {@template flutter.rendering.RenderEditable.whiteSpace} |
There was a problem hiding this comment.
I think there should be one empty line after this template and before the next paragraph.
///
There was a problem hiding this comment.
Theres an empty line before and after the template block. Do you mean it should have one within the block as well?
There was a problem hiding this comment.
There isn't an empty line after the template block, or am I reading the diff wrong? I mean it should look like this:
///
/// {@template flutter.rendering.RenderEditable.whiteSpace}
///There was a problem hiding this comment.
I have it like this:
/// {@macro flutter.rendering.RenderEditable.cause}
///
/// {@template flutter.rendering.RenderEditable.whiteSpace}
/// By default, includeWhitespace is set to true, meaning that whitespace can
/// be considered a word in itself. If set to false, the selection will be
/// extended past any whitespace and the first word following the whitespace.
/// {@endtemplate}
///
/// See also:
should I change it to something like this?
/// {@macro flutter.rendering.RenderEditable.cause}
///
/// {@template flutter.rendering.RenderEditable.whiteSpace}
///
/// By default, includeWhitespace is set to true, meaning that whitespace can
/// be considered a word in itself. If set to false, the selection will be
/// extended past any whitespace and the first word following the whitespace.
/// {@endtemplate}
///
/// See also:
I did a code search and haven't found any case where it had a empty line there, hence the confusion.
There was a problem hiding this comment.
Ah sorry you're right, I don't know what I was thinking. I think I misread this as a macro instead of a template. Thanks for checking and not just adding the line 😊
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM, thanks for all of your work on this!
| /// | ||
| /// {@macro flutter.rendering.RenderEditable.cause} | ||
| /// | ||
| /// {@template flutter.rendering.RenderEditable.whiteSpace} |
There was a problem hiding this comment.
Ah sorry you're right, I don't know what I was thinking. I think I misread this as a macro instead of a template. Thanks for checking and not just adding the line 😊
There was a problem hiding this comment.
Yes, I'm on board with that!
justinmc
added
waiting for tree to go green
a: text input
)" This reverts commit 75453e6 because it breaks flutter/test/rendering/editable_test.dart
|
@canastro the PR is reverted to reopen the tree. Could you try relanding the PR after fixing the test failures? It is strange that the test didn't fail during presubmit tests: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8851763205480546384/+/steps/run_test.dart_for_framework_tests_shard_and_subshard_libraries/0/stdout |
This PR focuses on supporting modifiers to delete blocks of text in a editable widget.
Shortcuts per OS
Some cases for reference:
case: 'text with| multiple words'
word modifier + backspace: 'text | multiple words'
line modifier + backspace: '| multiple words'
word modifier + delete: 'text with| words'
line modifier + delete: 'text with|'
case: 'text with multiple\n|words'
word modifier + backspace: 'text with |words'
line modifier + backspace: 'text with multiple\n|words'
word modifier + delete: 'text with multiple\n|'
line modifier + delete: 'text with multiple\n|'
case: 'text with multiple|\nwords'
word modifier + backspace: 'text with |\nwords'
line modifier + backspace: '|\nwords'
word modifier + delete: 'text with multiple|'
line modifier + delete: 'text with multiple|\nwords'
case: 'text with\n|\nmultiple words' in obscure text
word modifier + backspace: '|\nmultiple words'
line modifier + backspace: '|\nmultiple words'
word modifier + delete: 'text with\n|'
line modifier + delete: 'text with\n|'
case: 用多個塊測試 (cursor at offset 4)
backspace: '用多個測試' (cursor at offset 3)
word modifier + backspace: '用多個測試' (cursor at offset 3)
line modifier + backspace: '測試' (cursor at offset 0)
word modifier + delete: '用多個塊試' (cursor at offset 4)
line modifier + delete: '用多個塊' (cursor at offset 4)
case: برنامج أهلا بالعالم (cursor at offset 4 - from the right)
backspace: برنمج أهلا بالعالم (cursor at offset 3 - from the right)
word modifier + backspace: مج أهلا بالعالم (cursor at offset 0 - from the right)
word modifier + delete: برنا أهلا بالعالم (cursor at offset 4 - from the right)
Related issue:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.