Help beta test multi-cursor Calva Paredit, please ❤️ 🙏

[PEZ]

@riotrah is bringing something wonderful to Calva, multi cursor support to Paredit!

The way this beta test will work is that Calva Team members will maintain a checklist of known issues right here and all who participate in the testing can report findings in the comments. See How to test custom VSIX packaged on the Calva wiki if this is unfamiliar to you. (TL;DR: It is super easy.)

• Issue: #610
• PR: #1606
• Latest VSIX: calva-2.0.266-pull-1606-62a922f0.vsix

This is recorded April 3, 2022, with the build: calva-2.0.263-pull-1606-298af21c.vsix

https://user-images.githubusercontent.com/30010/161421143-7058384e-447d-4da9-b7f2-7dcb00f16e43.mp4

As you can see it is largely working! As you also can see, sometimes it doesn't do the right thing. So this is a call for help testing this to help us find bugs and to help us discover what is the right thing for certain cases.

See Paredit, a Visual Guide for how Calva Paredit works for a single cursor (and to appreciate what an effort this is for @riotrah to pull off, and why the rest of us should help with testing.

Test notation

In the Calva tests for structural navigation and editing we use a special document notation, which is easy to read for both the machine and for us humans:

/**
 * Text Notation for expressing states of a document, including
 * current text and selection.
 * See this video for an intro: https://www.youtube.com/watch?v=Sy3arG-Degw
 * * Since JavasScript makes it clumsy with multiline strings,
 *   newlines are denoted with the paragraph character: `§`
 * * Selections are denoted like so
 *   * Cursors, (which are actually selections with identical start and end) are denoted with a single `|`, with <= 10 multiple cursors defined by `|1`, `|2`, ... `|9`, etc, or in regex: /\|\d/. 0-indexed, so `|` is 0, `|1` is 1, etc.
 *   * This is however actually an alternative for the following `>\d?` notation, it has the same left->right semantics, but looks more like a cursor/caret lol, and more importantly, drops the 0 for the first cursor.
 *   * Selections with direction left->right are denoted with `>0`, `>1`, `>2`, ... `>9` etc at the range boundaries
 *   * Selections with direction right->left are denoted with `<0`, `<1`, `<2`, ... `<9` etc at the range boundaries
 */

See this CalvaTV video for tutorial: https://www.youtube.com/watch?v=Sy3arG-Degw

A typical report has

1. a description of a before-document
2. a command
3. a description of a an expected after-document
4. a description of a the actual after-document

We try to keep the document descriptions as minimal as possible. See the first report in the comments for an example.

There's a command for it

The test VSIX has a command for printing the text notation from the state of the current document.

Which makes creating a report involve these steps:

1. Create your minimal document (an Untitled doc in Clojure mode will do)
2. Place your cursors/selections
3. Issue the Print TextNotaion. command
4. Issue the Paredit command
5. Issue the Print TextNotaion. command again

Now you have the Before and Actual texts in the Output channel Calva says. You only need to add the Expected texts yoursefl.

Reported issues

• [x] Delete Forward to End of List only deletes for the first cursor, and loses the other cursors
• [x] Delete Backward to End of List only deletes for the first cursor, and loses the other cursors
• [x] Raise sexp wrong placement of cursor after the edit
• [x] Delete Sexp Forwards only deletes at primary cursor, loses the others
• [x] Delete Sexp Backwards only deletes at primary cursor, loses the others
• [x] Expand selection when all text in a list is selected, doesn't select the list. It selects the opening paren
• [ ] Paredit Delete Backward does not move cursor inside list when used from adjacent after the list
• [ ] Paredit Delete Forward does not move cursor inside list when used from adjacent before the list
• [ ] Forward Sexp moves cursor out of line comment instead of moving by word
• [ ] Multi-cursor Paredit Delete Backward destroys structure when a cursor is at the beginning of a list
• [ ] Multi-cursor Paredit Delete Forward destroys structure when a cursor is at the end of a list
• [ ] Multi-cursor edit commands loose all but the primary cursor (because formatting, which is not multi-cursor aware)
• [x] Multi-cursor Splice sexp misplaces cursors
• [x] Wrap around commands only wraps primary cursor and loses the other cursors
• [ ] Delete backward does not move into list from behind closing bracket

[PEZ]

Hi! Thanks @riotrah for working on this awesome feature! ❤️ Of course I love to help with testing. Here's an issue I found:

Delete Forward to End of List only deletes for the first cursor, and loses the other cursors

Before

(|2a)(|1a)(|a)

Delete Forward to End of List

Expected

(|2)(|1)(|)

Actual

(a)(a)(|)

As this is also an example of the report format, let me elaborate some:

1. I've started with the text (a)(a)(a)
2. Placed the cursor before the last a -> (a)(a)(|a)
3. Placed a second cursor before the middle a -> (a)(|1a)(|a)
4. (You know this step)
5. Issued the command
6. Got the Actual document

The command is written like <kbd>**Command**</kbd> to make it look command-ish.

[PEZ]

I'm having a great time testing this! Thanks again! 🙏

Delete Backward to Start of List only deletes for the first cursor, and loses the other cursors

Before

(a|)(a|1)(a|2)

Delete Backward to Start of List

Expected

(|)(|1)(|2)

Actual

(|)(a)(a)

[PEZ]

Here's an interesting one. The edits are correct, then all but the leftmost cursor get offset. I've tried it on some more cases then below, and I think the offset is related to how much text is deleted by leftwards cursors.

Raise sexp wrong placement of cursor after the edit

Before 1

;; 1. Start two cursors left->right
(a (b|)) (a (b|1)) (a (b))

Raise sexp

Expect 1

(a b|) (a b|1) (a (b))

Actual 1

(a b|) (a b) |1(a (b))

Before 2

;; 1. Start two cursors left->right
(a (b|1)) (a (b|)) (a (b))

Raise sexp

Expect 2

(a b|1) (a b|) (a (b))

Actual 2 (not sure if it keeps the cursor order)

(a b|1) (a b) |(a (b))

[PEZ]

Delete Sexp Forwards only deletes at primary cursor, loses the others

Before

(|2a) (|1a) (|a) (a)

Delete Sexp Forwards

Expect

(|2) (|1) (|) (a)

Actual

(a) (a) (|) (a)

[PEZ]

Delete Sexp Backwards only deletes at primary cursor, loses the others

Before

(a|2) (a|1) (a|) (a)

Delete Sexp Backwards

Expect

(|2) (|1) (|) (a)

Actual

(a) (a) (|) (a)

[riotrah]

Almost done with the ones posted so far. The text notation format in which you've posted these has made it very easy to test driven develop.

Along the way I learned:

• Actions that reduce the number of characters in the document - eg deletions - are not so trivial to make multi cursor compatible, as the document shortens each deletion! This then means some of the cursors
• The above is made easier if I just iterate in descending order of cursor location
  • Lol somehow I managed to miss this until recently, and just kept doing all these calculations when going in ascending order to calculate the sum of deletions to adjust each subsequent cursor offset etc

[riotrah]

Update:

Made the above pass. Added a docToTextNotation unit test debugging helper and added test for it.

Now just making sure the changes I made are actually legible lol.

It's been interesting having to calculate these offsets and stuff, haven't done much like this before.

A question for @PEZ et al.:

Multicursor copy or "kill while adding to clipboard" is tricky. I'm not exactly sure how vscode does it natively, but if you have n cursors and copy text per cursor, and then paste at a later point with exactly n cursors, the cursors paste according to the original corresponding cursor's location. However, if you do not have exactly n cursors, the total sum of copied text is pasted at each cursor. For now I'm copying only the first (primary) cursor's content. Will have to see how that goes after

[PEZ]

Stellar work, @riotrah! I can't thank you enough. 🙏 ❤️

Made the above pass. Added a docToTextNotation unit test debugging helper and added test for it.

So awesome! I made a user command from it. See updated issue description.

Actions that reduce the number of characters in the document - eg deletions - are not so trivial to make multi cursor compatible, as the document shortens each deletion!

😄 Happy you have discovered that it is easier if you work from the end of the document. (Not saying that it gets easy, but anyway.)

The next level of this is when edits overlap, like:

(|(a :a) (|1b :b))

Raise sexp

Now what?

In the latest build nothing happens, which might be the right call, and mean that you have thought of this.

Multicursor copy or "kill while adding to clipboard" is tricky.

Indeed! I think what you do makes sense for now. Add the behaviour to the paredit.md doc, if you haven't.

[PEZ]

Used this build all day at work today. Totally useful, even though some quirks remain of course. Here's one.

Expand selection when all text in a list is selected, doesn't select the list. It selects the opening paren:

Before

(|a|)

Expand Selection

Expected

|(a)|

Actual

|(|a)

This bug is now famous from this video: https://www.youtube.com/watch?v=Sy3arG-Degw 😄

[riotrah]

Can TokenCursors (or any other API) tell us how "deep" a cursor or the form it's in is? That way, for Raise, we should go in order of cursor depth. It's not as efficient as short circuiting by doing shallower overlapping form raises first, but it's easier to implement and read, and is logically accurate.

For example, in the given example, the deepest symbol/form to raise is the b at |1b. We'd raise that first to get (|(a :a) b). Then we'd run it for the (a :a) at |(a :a), which would give us the final output of (a :a).

[PEZ]

Calva Highlight is doing this kind of work continuously. But only on visible forms, so maybe it's not the place to look...

Maybe just do cursorUp until at the top, with each cursor?

[PEZ]

Maybe we can factor out the thing from Calva Highlight and run it on demand...

[PEZ]

Paredit Delete Backward does not move cursor inside list when used from adjacent after the list:

Before

()|

Paredit Delete Backward

Expected

(|)

Actual

()|

We have the same error with Delete Forward.

[PEZ]

@riotrah Did you remove the cursor contexts without updating the Paredit commands to handle it instead? 😄

Forward Sexp moves cursor out of line comment instead of moving by word:

Before

a§|; a b c§d e

Forward Sexp

Expected

a§;| a b c§d e

Actual

a§; a b c§d| e

[PEZ]

Multi-cursor Paredit Delete Backward destroys structure when a cursor is at the beginning of a list:

Before

(a|)§(|1)

Paredit Delete Backward

Expected

(|)§|1

Actual

(|)§|1)

As well as:

Before

(|)§(|1)

Paredit Delete Backward

Expected

|§|1

Actual

|)§|1)

We have the same error with Delete Forward.

[PEZ]

Now we are entering into tricky territory, indeed. Post-edit formatting! Paredit editing commands "spawns of” a formatting of the text after the editing has happened. But the formatter is not multi-cursor aware, so we loose all but the primary cursor after an edit. Illustrating here with Barf forward:

With formatting

https://user-images.githubusercontent.com/30010/162610054-27a7221a-5c44-449c-b5e1-85b9a67c814c.mp4

Copying the before, expected, actual text-notation in here. Note that they can't be straightforwardly translated to unit tests because currently our Paredit unit tests do not capture the post-formatting step.

Before

([|a§  b])§([|1a§  b])

Barf forward

Expected

([|a]§ b)§([|1a]§ b)

Actual

([|a]§ b)§([a]§  b)

Without formatting

Calva checks if the formatter has changed the document and won't apply the changes if there is no change, so this retains the cursors:

https://user-images.githubusercontent.com/30010/162610582-206882d5-24c0-4f32-8a55-763508dd4a68.mp4

Before

([|a b])§([|1a b])

Barf forward

Expected

([|a] b)§([|1a] b)

✅

[PEZ]

Some options for the formatting problem:

1. Update the formatter to be multi-cursor aware. It does some trickery today to figure out the new cursor position after formatting, where trickery == a hack. I anticipate making it work for multi-cursors to be quite hard.
2. Run the formatter on each cursor doing the necessary calculations to adjust cursor in front of a cursor after each formatting. This is probably also quite hard to do, but I'd guess it's easier than 1.
3. In multi cursor situations, instead of formatting each ”current form” we format the whole document. Then our formatter does not touch the cursors, and VS Code is pretty good at figuring out cursor positions when doing this. Drawback here is that as a user you might not want to have parts of the document unrelated to what you are editing to be auto-formatted. And VS Code sometimes misses, while – doing it ourselves – we potentially can make it never miss.
4. ...

[PEZ]

Splice sexp misplaces cursors when there more than one

Before

(|a)§(|1b)

Splice sexp

Expected

|a§|1b

Actual

|a§b|1

That is what I see in VS Code when I do this. In the unit tests I get even weirder results... And it get's weirder (with some pattern to it) with increasing amount of cursors. I've added failing tests and pointed out the weirdness I see in comments.

[PEZ]

Wrap around commands only wraps primary cursor and loses the other cursors

Before

|a§|1b

Wrap around ()

Expected

(|a)§(|1b)

Actual

(|a)§b

Again there is a weird difference between what happens in VS Code and what I see in the unit tests...

[riotrah]

Some interesting stuff in here, we are indeed entering tricky territory haha!

Will look into

[PEZ]

Delete backward does not move into list from behind closing bracket

Before

()|

Delete backward

Expected

(|)

Actual

()|

The unit tests for this pass, though...

[PEZ]

@riotrah , I notice that the failing tests I have been adding, now pass. Does that mean that some checkboxes should potentially be ticked in the issue description here. It takes me a lot of time to test things blindly, so some update on what I should be re-testing would be nice!

[riotrah]

@PEZ I think I've updated the desc to reflect current local branch. Will push them shortly. Some of them are very strange as you've noted 😅

[riotrah]

Multi-cursor Paredit Delete Backward destroys structure when a cursor is at the beginning of a list:

Before

(a|)§(|1)

Paredit Delete Backward

Expected

(|1)§|

Actual

(|)§|1)

Hi @PEZ, I wonder if the cursors in the first Expected are swapped?

[riotrah]

Multi-cursor Paredit Delete Backward destroys structure when a cursor is at the beginning of a list:

Before

(a|)§(|1)

Paredit Delete Backward

Expected

(|1)§|

Actual

(|)§|1)

As well as:

Before

(|)§(|1)

Paredit Delete Backward

Expected

|§|1

Actual

|)§|1)

We have the same error with Delete Forward.

What does We have the same error with Delete Forward** mean here? Can you give me a unit test case?

[riotrah]

Paredit Delete Backward does not move cursor inside list when used from adjacent after the list:

Before

()|

Paredit Delete Backward

Expected

(|)

Actual

()|

We have the same error with Delete Forward.

What is the "delete forward" equivalent of this test/feature?

[PEZ]

Hi @PEZ, I wonder if the cursors in the first Expected are swapped?

Yes, I've updated the comment now.

What does We have the same error with Delete Forward** mean here? Can you give me a unit test case?

It means that the general problem of destroying structure goes for both deleting backwards and forwards. So if I pick your last question about this, it would be:

Before

|()

Paredit Delete Forward

Expected

(|)

Actual

|()