Auto pair-removal #2940
Auto pair-removal #2940
Conversation
|
\cc @dead10ck |
|
This implementation is pretty much the same exact mechanism as #1379 (i.e. look ahead and behind one char and return a delete, see here) and so has exactly the same problem (which I've verified by pulling this branch locally and testing manually):
The implementation of this should live in the I would also want to see test coverage to verify that the intersection of all text edit cases works, as I described in this comment. I'm afraid the solution won't be this simple. It's why I've delayed trying until after we have an integration testing suite, which we have now. I plan on working on this after I finish up the write path fixes in #2267, if no one gets to it before me. |
|
Yeah it doesn't fully fix it but the plan was to at least implement this for 1-width selections which should solve this for majority editing of cases. |
With respect, I don't think this is as uncommon as you may think. The test case above is going to happen any time the user enters append mode, or does a select all -> search for text, and then deletes chars and runs into a pair, e.g. empty strings, function calls with no arguments, slices, etc, etc. And it may not even be visible to them if it happens off screen. |
|
On the other hand, I saw it suggested in the chat room that we could make this behavior only happen specifically when the range is single width, otherwise fall back to the regular single char delete. If that constraint were added, this would indeed be a good workaround until it can be fixed in a more general way for selections too. |
done in 005f4c6 |
| // delete both autopaired characters | ||
| { | ||
| ( | ||
| graphemes::nth_prev_grapheme_boundary(text, pos, count), |
There was a problem hiding this comment.
This might get a bit tricky with the count. If we just delete n chars in both directions, this will be wrong in some cases. e.g. presently with this case:
hello, "["], goodbye("")
If you hit backspace 3 times, you get
hello[,] goodbye("")
like you'd expect. But if you use the command with a count, e.g. if you bound backspace to this in normal mode and then hit 3<backspace>, you end up with:
hello[g]oodbye("")
Though, I don't know how many people bind this in normal mode and use the count with it, so I don't know if I'd consider this a blocker. It might be tricky to get the logic right.
helix-term/src/commands.rs
Outdated
| auto_pairs, | ||
| ) { | ||
| (Some(_x), Some(_y), Some(ap)) | ||
| if range.len() == 1 |
There was a problem hiding this comment.
Might be better to use Range::is_single_grapheme so that it also works when the cursor is over a non-ASCII grapheme.
|
One minor issue I just noticed: if a pair is at the very beginning of the file, and the cursor is on the first char, it will delete the first char instead of doing nothing. This might be fixable by checking if the cursor is the first char in the document. |
* auto pair-removal Fixes helix-editor#1673 * autopairs removal: use doc autopairs * autopairs-removal: limit to one-char selections * use single_grapheme() to check if range is one char * fix errouneous deletes of " and other symmetric autopairs when at buffer start Co-authored-by: Houkime <>
Fixes #1673
Supersedes #1379
No problems with multiple selections observed.
Putting match inside tuple would hurt readabilty - these tuples already take a while to comprehend.
Simple logic inspired by Godot 3.x stable branch:
https://github.com/godotengine/godot/blob/14f69acaa4ef80b235ef78f9caac1072daf8008b/scene/gui/text_edit.cpp#L2101
https://github.com/godotengine/godot/blob/14f69acaa4ef80b235ef78f9caac1072daf8008b/scene/gui/text_edit.cpp#L2054