Add auto pairs for same-char pairs #1219

dead10ck · 2021-12-02T05:14:30Z

Changes

Add unit tests for all existing functionality
Add auto pairs for same-char pairs (quotes, etc). Account for apostrophe in prose by requiring both sides of the cursor to be non-pair chars or whitespace
Slight factor of moving the cursor transform of the selection to inside the hooks. This will enable doing auto pairing with selections, and fixing the bug where auto pairs destroy the selection.

I will still be working on other things, but I thought it would be a good idea to make an incremental smaller PR for the sake of ease of review, and for providing opportunity for feedback with the approach to testing and implementation details before I build further on them.

Fixes #1014

Things I'd still like to do:

delete implementation where it erases the whole bracket (|) -> |
do not reduce to cursors; use whole selections, and surround with pair
change to multi character pairs to handle cases like placing the cursor in the middle of triple quotes, and more exotic pairs like Jinja's {% %}
add more granular config for particular pairs and behaviors as discussed in Enable config for auto-pairs #992

In particular about the auto pairing with selections, I think when we have a case like (where [] is the selection)

[wor]d

and we enter insert mode and type ", we could either unconditionally surround the selection and make

"[wor]"d

so it behaves exactly like surround, or we could choose only to surround when both sides of the selection are non-pair chars or whitespace, extending the current behavior to selections, so

"[wor]d

I'm on the fence about it, so any guidance there would be helpful.

archseer · 2021-12-02T06:43:11Z

helix-core/src/auto_pairs.rs

+        )
+    }
+
+    // [TODO] broken until it works with selections


This is sort of addressed bybmsa"(match surrounding add ()

Sorry, just to be clear, are you saying we shouldn't worry about adding surround functionality with selections? Or just that this particular case should just be inserting a ( before the word, but if the whole word was selected, do the surround?

Ok, I've fixed this test, although it's still broken until I add the changes for selections.

helix-core/src/auto_pairs.rs

archseer · 2021-12-02T06:48:52Z

Great work! I need to give this a full read later but it's especially great to see test coverage being added :)

archseer · 2021-12-08T03:51:30Z

helix-core/src/auto_pairs.rs

 fn next_char(doc: &Rope, pos: usize) -> Option<char> {
    if pos >= doc.len_chars() {
        return None;
    }
    Some(doc.char(pos))
 }


This can be replaced by doc.get_char(pos) now, ropey added a non-panicking variant.

Right, done.

archseer · 2021-12-08T03:53:11Z

helix-core/src/auto_pairs.rs

 fn handle_open(
    doc: &Rope,
    selection: &Selection,
    open: char,
    close: char,
    close_before: &str,
 ) -> Transaction {
-    let mut ranges = SmallVec::with_capacity(selection.len());
+    let mut end_ranges = SmallVec::with_capacity(selection.len());


Why the rename? It looks like all the variables in _open and _close were renamed but nothing else was changed?

Yeah, sorry, this was mostly for my own benefit to better understand the code. When building the final range, in the anchor position, it's offset inline, but the head is in a variable that was defined above. And to me pos (i.e. position) and head are kind of synonymous, and it was hard to follow that one is what we started with and the other is what we want it to be after the text transformation. Are you okay with the changes?

archseer · 2021-12-08T03:54:47Z

helix-core/src/auto_pairs.rs

+            start_range.anchor + offs
+        };
+
+        // if selection, retain anchor, if cursor, move over


This comment would be better suited above the let end_anchor

Done. Also, it's worth noting that the if expression is currently a tautology because the selection is transformed into cursors before this code is hit. I kept it because something like it will be necessary when I get to fixing selections (which I'm pretty close to now).

archseer · 2021-12-08T03:56:20Z

helix-core/src/auto_pairs.rs

+            //  return transaction that moves past close
+            (start_head, start_head, None) // no-op
+        } else {
+            let mut pair = Tendril::with_capacity(2);


This uses bytes as the capacity, you probably want 2 * token.len_utf8()

archseer

Looks good to me but I'd like to get one more reviewer

/cc @sudormrfbin

* Add unit tests for all existing functionality * Add auto pairs for same-char pairs (quotes, etc). Account for apostrophe in prose by requiring both sides of the cursor to be non-pair chars or whitespace. This also incidentally will work for avoiding a double single quote in lifetime annotations, at least until <> is added * Slight factor of moving the cursor transform of the selection to inside the hooks. This will enable doing auto pairing with selections, and fixing the bug where auto pairs destroy the selection. Fixes helix-editor#1014

sudormrfbin

Some minor nitpicks, other than that LGTM :)

helix-core/src/auto_pairs.rs

sudormrfbin · 2021-12-12T14:28:36Z

helix-core/src/auto_pairs.rs

+            start_range.anchor + offs
+        };
+
+        end_ranges.push(Range::new(end_anchor, end_head));

        match next {
            Some(ch) if !close_before.contains(ch) => {
                offs += 1;
                // TODO: else return (use default handler that inserts open)


Is this TODO still relevant @archseer ? All the handle_ functions would have to return an Option that gets passed through the hook.

helix-core/src/auto_pairs.rs

archseer reviewed Dec 2, 2021

View reviewed changes

helix-core/src/auto_pairs.rs Show resolved Hide resolved

dead10ck force-pushed the auto_pairs branch 7 times, most recently from 0621b85 to bc4c38d Compare December 8, 2021 03:39

archseer requested changes Dec 8, 2021

View reviewed changes

archseer approved these changes Dec 11, 2021

View reviewed changes

dead10ck added 4 commits December 12, 2021 00:03

move comment

ebb19d9

use multiple of token utf8 length for initial Tendril capacity

22202a7

use Rope::get_char instead of doing our own bounds checking

ed4cffc

dead10ck force-pushed the auto_pairs branch from c483d22 to ed4cffc Compare December 12, 2021 05:06

dead10ck mentioned this pull request Dec 12, 2021

Auto pairs selection #1254

Merged

sudormrfbin reviewed Dec 12, 2021

View reviewed changes

dead10ck added 2 commits December 12, 2021 15:42

consolidate logic, get rid of needless mut

2c62d56

use len_utf8 instead of assuming ascii

33f5941

archseer merged commit 94535fa into helix-editor:master Dec 13, 2021

dead10ck deleted the auto_pairs branch December 21, 2021 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto pairs for same-char pairs #1219

Add auto pairs for same-char pairs #1219

dead10ck commented Dec 2, 2021 •

edited

Loading

archseer Dec 2, 2021

dead10ck Dec 2, 2021

dead10ck Dec 4, 2021

archseer commented Dec 2, 2021

archseer Dec 8, 2021

dead10ck Dec 8, 2021

archseer Dec 8, 2021

dead10ck Dec 8, 2021

archseer Dec 8, 2021

dead10ck Dec 8, 2021 •

edited

Loading

archseer Dec 8, 2021

archseer left a comment

sudormrfbin left a comment

sudormrfbin Dec 12, 2021

Add auto pairs for same-char pairs #1219

Add auto pairs for same-char pairs #1219

Conversation

dead10ck commented Dec 2, 2021 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead10ck Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer left a comment

Choose a reason for hiding this comment

sudormrfbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead10ck commented Dec 2, 2021 •

edited

Loading

dead10ck Dec 8, 2021 •

edited

Loading