Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paredit commands don't propagate to multiple cursors #610

Open
pitalig opened this issue Apr 13, 2020 · 12 comments · May be fixed by #1606
Open

Paredit commands don't propagate to multiple cursors #610

pitalig opened this issue Apr 13, 2020 · 12 comments · May be fixed by #1606
Assignees
Labels
paredit Paredit and structural editing pr-welcome

Comments

@pitalig
Copy link

pitalig commented Apr 13, 2020

When I have multiple cursors, paredit commands just affects the first one.
This gif shows an example when I try to slurp 3 forms, but it just affects the first one.
ezgif-7-f84d4d5d9ca1
The expected result was:

[[:a 1]
 [:b 2]
 [:c 3]]
@PEZ PEZ added enhancement paredit Paredit and structural editing pr-welcome labels Apr 13, 2020
@PEZ
Copy link
Collaborator

PEZ commented Apr 13, 2020

Yes, it would be great if this worked! PR welcome.

@StarScape
Copy link

+1

@kbosompem
Copy link

+1

@PEZ PEZ removed the enhancement label Feb 10, 2021
@rayat-amperity
Copy link

I would be honored to take this on.

Any pointers or tips before I get started?

@PEZ
Copy link
Collaborator

PEZ commented Mar 16, 2022

Awesome news, @rayat-amperity ! Please consider touching base in the #calva channel on the Clojurians slack. I'd love to give you a guided tour in the relevant parts of the code base.

@rayat-amperity
Copy link

(full disclosure, I am not an honorable man, just one who loves multi-cursor).

  1. Forgive my ignorance, how do I join a slack without an invitation? I see options to "sign in" but of course I do not have an existing account.
  2. In the interest of posterity, collab and perhaps hand-off in case I fall off the face of the earth, would you interested in sharing the guided tour somewhere accessible like in this issue, or something? Totally okay if not, whatever works for you!

(btw big fan of your work and talks @PEZ, and my coworkers have spoken very highly of you personally!!!!!)

@PEZ
Copy link
Collaborator

PEZ commented Mar 17, 2022

I'm blushing here! Please tell your colleagues that I thoroughly enjoyed meeting them and have that chat about Calva. Huge thanks for relaying this. You just made my day. ❤️

I think you should be able to join Clojurians via this link: http://clojurians.net/

Good idea with recording such a guided tour. I touch a bit on some of the relevant parts of this, in my Seajure presentation, which you might have heard your colleagues mention. Here's a deep link: https://youtu.be/CcPxHGm0JWo?t=6155

@rayat-amperity
Copy link

  1. <3

  2. Thanks!

  3. Awesome, just saw that portion. Funny to see my coworkers like that, never had that happen before, man the clj community is intimate!

  4. So I've been toying around in the codebase and have a decent sense of what needs to be done to start with. Preliminary observations, notes or questions:

  5. The various models that wrap the vscode document apis make certain assumptions, the most notable of which for this issue's sake are that editors/documents have a single selection or cursor location at a time.
    2. This assumption is in line with the usage of TextEditor.selection instead of TextEditor.selections (or TextEditor.selections[0]) in the implementers of EditableDocument (namely MirrorDocument).
    3. This has potentially wide-reaching implications that I don't personally know the extent of.
    4. I'll need your take on some of the API changes I'll need to make.

    1. For example, EditableDocument has .selectionLeft/Right properties whose semantics change or whose utility diminish if we try to keep multiple cursors in mind.
      2. Operations that operate on multiple cursors would then primarily access them in manners like return doc.selections.map(s => ({ anchor: s.anchor, active: s.active })) vs return ({ anchor: doc.selectionLeft, active: doc.selectionRight }), so the differences could be subtle, or more than that. I'll have a better sense myself as I dive into this more.
    2. Where should I track this work and those questions? I'm thinking a PR linked to this issue. Whatever works for you.

@PEZ
Copy link
Collaborator

PEZ commented Mar 19, 2022

I think that generally it would be good to start with cleaning away all the places where selectionLeft is used, where selection.active should have been used instead. I don't know, without investigating, wether there are any cases where we actually need Left/Right semantics. It's probably rare, if at all. In those cases we can probably use functions or dynamic properties that derive from anchor/active.

A PR that only does this. Or only does this at the places we deem perfectly safe, would be a good start, and a huge improvement of the codebase, regardless of multi-cursors. Please also touch base on the slack.

@rayat-amperity
Copy link

rayat-amperity commented Mar 19, 2022

Before I just read your comment, I had gotten Expand Selection / Shrink Selection to work! At least it looks like it from messing around in the test folder. Other untouched commands work as they did previously. I'll open a draft PR just so you can maybe provide feedback, or if you're simply curious, in the meantime.

Once that's open I'll switch over to work on your suggestion above first, and I'll rebase my multi cursor work upon that.

EDIT:

#1606 is open as a draft. Have not done any of the steps, as it's not merge-worthy in the slightest. But keeping it until I've done the cleanup work suggested above.

@PEZ
Copy link
Collaborator

PEZ commented Mar 20, 2022

I saw code for expand selection that looked like it might work. 😄 This is so exciting!

@riotrah
Copy link
Member

riotrah commented Apr 7, 2022

For those interested in this, it's almost ready. It needs testing though before we would feel comfortable publishing!

Please check out the HELP announcement here with testing instructions.

You can download the testing version of the extension in the PR and let us know what it's like. It'll come out faster the more you help out!

@riotrah riotrah linked a pull request Apr 7, 2022 that will close this issue
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paredit Paredit and structural editing pr-welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants