#610 Multi Cursor for paredit

[rayat-amperity]

Fixes #610

What has Changed?

• Multi cursor support for most Paredit commands
• Significant changes to ModelEditSelection API, EditableDocument API
• Added ModelEditResult, ModelEditFunction, ModelEditFunctionArgs types
• Almost all paredit functions, esp those exposed directly to extension as a command handler, now take an array of offsets, ranges, ModelEditSelections etc, where they previously took individual instances of each, and similarly return an array of whatever they previously returned.
  • Where single cursor calculations are required, just pass and expect a single-item array

Fixes #610

My Calva PR Checklist

I have:

• [x] Read How to Contribute.
• [x] Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• [x] Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• [x] Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• [x] Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• [x] Added to or updated docs in this branch, if appropriate
• [x] Tests
  • [x] Tested the particular change
  • [x] Figured if the change might have some side effects and tested those as well.
  • [x] Smoke tested the extension as such.
  • [x] Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test.
• [x] Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • [x] If I am fixing the issue, I have used GitHub's fixes/closes syntax
  • [x] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• [x] Created the issue I am fixing/addressing, if it was not present.
• [x] Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
• [x] Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @pez, @bpringe

[PEZ]

Looks like you have had a nice start! Please let me know when you think I should test something. Consider basing this PR onto the one where you removed left/right selection confusion.

[rayat-amperity]

@PEZ I already started rebasing this PR on #1608. I'll switch back to prepping that PR for merging.

I do have a(nother) question:

This might be better implemented in the cleanup PR

• The paredit command handlers (eg. selectRangeForward (src/cursor-doc/paredit.ts#49)) each take args usually like (doc: EditableDocument, ranges: [number, number]) {}.
  • Why not take a ModelEditSelection directly? Eg (doc: EditableDocument, selection: ModelEditSelection) {}
  • Then, if we extend ModelEditSelection with start, end, isReversed properties as I proposed in the Cleanup API Issue, instead of logic like const rangeRight = Math.max(range[0], range[1]); as some currently have, we could simply call const rangeRight = selection.end

[riotrah]

Just putting a note here that the tests have to be rewritten at least somewhat for this.

Your shorthand/dsl for describing documents/text assumes two | symbols are a single directionless selection, instead of 2 cursors.

This will need to change but I want to do so in the least disruptive way possible. Any thoughts?

[PEZ]

I've been thinking a bit about this, actually. I'd like to keep the current semantics as long as we only have one cursor to consider. How about that for multi cursor cases, we number the cursors? |1, etcetera?

[rayat-amperity]

I've been thinking a bit about this, actually. I'd like to keep the current semantics as long as we only have one cursor to consider. How about that for multi cursor cases, we number the cursors? |1, etcetera?

Sure thing, that's a reasonable idea - it might be tricky if, for any reason, any of the existing tests had a numeral after the | like in your example, but if they exist, we'll prob find them.

Similarly then, we're assuming that if there's something like "(:a {:b '(:c |1 2 [3 |1 2])})" the 2 |1 would defined an enclosed direction-less selection, and similarly if they had the >|1 syntax it would be the same, but directionless, right?

[rayat-amperity]

I do need help with another thing @PEZ :

src/cursor-doc/cursor-context.ts#determineContexts() is used for setting vsc contexts for some of the more 'intelligent' (or at least contextually aware) commands, that pertain to or would benefit from knowledge of the forms or document contents around the "current" cursor.

That function checks some stuff around the current primary cursor, and then sets vsc contexts like:

• 'calva:cursorAtStartOfLine'
• 'calva:cursorAtEndOfLine'
• 'calva:cursorInString'
• 'calva:cursorInComment'
• 'calva:cursorAfterComment'
• 'calva:cursorBeforeComment'

Which are then checked by those aforementioned cursor contextual commands. What I should prob do at some point is list all of them, and see how likely it is users would expect or desire multi cursor for those in particular.

In any case, this clearly doesn't work as-is for multi-cursor.

A few options:

1. [ ] Figure out some other approach for contextually aware commands that would benefit from multi-cursor
2. [ ] Concede the "loss" or compromise here and simply make it clear to users that those particular commands only operate on the context of the current primary cursor.
3. [ ] Figure this out later, as long as most of the multi cursor works now 😆

[PEZ]

Tricky with the contexts! I think that for all commands affected this cursors contexts should be removed and the commands should handle the check on a per cursor basis.

If it should be done in this PR or not... Depends. Mostly on how much work you think is involved. I guess there is some MVP we can cut out. That said, one place I miss multi cursors a lot is when moving by sexpression. 😄

[riotrah]

Tricky with the contexts! I think that for all commands affected this cursors contexts should be removed and the commands should handle the check on a per cursor basis.

If it should be done in this PR or not... Depends. Mostly on how much work you think is involved. I guess there is some MVP we can cut out. That said, one place I miss multi cursors a lot is when moving by sexpression. 😄

Well, the contexts are really helpful for users making contextual keybindings no? For the "when expressions"?

[PEZ]

Well, the contexts are really helpful for users making contextual keybindings no? For the "when expressions"?

Yes, but they also get to be an ”API” that now proves tricky to maintain. If should keep the context we need to figure out what the semantics of them is in a multi-cursor world. Probably they would refer to ”all cursors”? But that will still lead to weird effects in situations where cursors are in different contexts, like one is in a comment and some others are not, what would !calva.cursorInComment mean then?

Hmmm... I think we probably should remove the contexts.

[riotrah]

Regarding when contexts:

I made an exhaustive search of all the when contexts available by default in the latest version of VSC, and none of the general purpose ones involve the specific circumstances of one or any cursors.

Ie. there are no when contexts that say something 'cursorAtIndentationorcursorInCommentorselectionSpansMultipleLinesetc. The only ones areeditorHasSelectionandeditorHasMultipleSelections`.

This would suggest that it's not a pattern to have the when context evaluated per cursor, and suggests the when context is a boolean check vscode does for the user/extension etc.

My hope was that the end-user could still use calva's cursorInComment etc when expressions in keybindings, and then instead of calva setting that context true or false based on primary cursor and then vscode deciding at keypress-time whether or not to execute the command, that instead, vscode would "inform" calva that the user executed whatever command, but had also specified the when context for it, and then calva would evaluate the expression per cursor, and execute per cursor when passing the condition.

Don't think that's possible from my research, tho vsc is massive and full of features.

Options:

1. As you suggested, simply remove this feature for users
2. Leave em, but they only apply to primary cursor (perhaps rename them to "primaryCursorInComment" ?)
3. Move them from when contexts to standard settings configuration. Not as convenient, but at least possible.

[PEZ]

If I remember correctly, vscode does not have contexts for cursor location, so the lack of prior art isn't really telling us much.

My hope was that the end-user could still use calva's cursorInComment etc when expressions in keybindings, and then instead of calva setting that context true or false based on primary cursor and then vscode deciding at keypress-time whether or not to execute the command, that instead, vscode would "inform" calva that the user executed whatever command, but had also specified the when context for it

Pretty sure this is not possible.

What's important is that we can keep Calva's default behavior for moving the cursor. I don't think the when contexts should be kept if we can achieve the correct movement without them. And that should be possible by making forward/backward sexp check the cursor position with the code we already have and relay to the builtin move by word commands in those cases where the when contexts would have solved that.

Adding more settings makes Calva more complicated for the users and for us. I'd rather avoid that, if we can.

[dosbol]

any news guys?

[PEZ]

@dosbol, unfortunately not. I tried to resolve the conflicts some months ago, but failed miserably. It doesn't mean this work can't be salvaged and continued, just that it needs a focused effort over a longer period.