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

Cursor Sexp Right Select, Cursor Sexp Left Select commands #1062

Closed
max-reify opened this issue Mar 9, 2021 · 16 comments
Closed

Cursor Sexp Right Select, Cursor Sexp Left Select commands #1062

max-reify opened this issue Mar 9, 2021 · 16 comments

Comments

@max-reify
Copy link

When editing a non-clojure file, selection via alt+arrows typically looks like this:

normal selection

Notice that expanding and contracting the selection naturally follows the current position of the cursor in relation to the original position of the cursor when selection began.

By contrast, the same motion with Calva produces the following result:

calva selection

This is jarring for new users (and maybe even experienced ones!)

Implementing the "normal" behavior except jumping by sexp instead of word isn't possible because Calva only has a single "shrink selection" command, irrespective of the position of the cursor in relation to the selection start position. Thus I propose 2 new commands:

  • cursorSexpRightSelect (no default keybinding): mirrors VSCode's default cursorWordRightSelect by moving the cursor one sexp right while leaving the selection start location in place
  • cursorSexpLeftSelect (no default keybinding): mirrors VSCode's default cursorWordLeftSelect by moving the cursor one sexp left while leaving the selection start location in place
@bpringe
Copy link
Member

bpringe commented Mar 10, 2021

Thanks for the proposition. This makes sense to me. Let's see what @PEZ thinks.

@PEZ
Copy link
Collaborator

PEZ commented Mar 10, 2021

I agree it is a bit strange how it behaves. However, at least it works consistently across all paredit select commands. If we fix this, I think it should be fixed at the root. If we figure out how to figure out that the direction has changed, then we can push the new selection on the selection stack and keep the ”undo” functionality of the shrink-selection command.

That said, I have not thought through the suggestion fully. I might find that a special case is in order. Haha.

How would we detect that the command should change direction of the selection rather than expand it in the other direction, btw?

@max-reify
Copy link
Author

I think the key is that the "normal select mode" doesn't work in terms of expand/contract actions. Instead, it anchors the selection at a point in the buffer, and then everything between that point and the cursor is selected as the cursor moves. So you're not expanding/contracting so much as moving the cursor. I think that behavior would be difficult to replicate in terms of expand/contract actions.

@PEZ
Copy link
Collaborator

PEZ commented Mar 10, 2021

The thing is that Calva's expand/contract selection is not really implemented like that. Under the hood there is just a stack of selections, which we push to and pop from.

I am staring a bit at the gifs you provided and it sort of seems to me like we should be able to just push ”the right thing” and it should start working as I think many people expect it to do. I really don't see a reason why someone would want it to behave like it currently does. (Doesn't mean there isn't a reason, but anyway.)

So, it was a long while since I made this selection stack thingy. But if I recall correctly it really just is a matter of:

Expand

  1. Expand selection command (any one of them) fired
  2. Figure out what the new selection should be
  3. Select that, and push the new selection on the stack

Shrink

  1. Shrink selection command fired
  2. Pop the stack
  3. Select whatever now is on top of the stack

I hope this makes sense. I am now thinking about Expand step-2. If I recall correctly we do have both the start and the end of the selection in the model (the end being where the cursor is). Again, looking at the gifs... say that:

  • end > start
    • Expand selection forward should then push end forward
    • Expand selection backward should then what? Set start <= end before considering what the new selection should be, maybe?
  • end < start
    • Expand selection backward should then push end backward
    • Expand selection forward, hmmm.... set end <= start, then select forward

Of course, this relies on that I am recalling correctly about the model we have there. But I think I am. Will have a check later.

@PEZ
Copy link
Collaborator

PEZ commented Mar 10, 2021

Now checked. It looks like we do have the mechanisms we need for making the default behaviour be like the normal selection works. Still not completely sure I am thinking about it correctly, but my gut feeling is that this is within reach.

@max-reify
Copy link
Author

One subtlety worth pointing out: consider this sequence of theoretical commands

(foo (boo bar baz) bing bang)
          ^cursor
  1. Expand selection forward sexp
(foo (boo bar baz) bing bang)
          ^-^
        selection
  1. Select forward up
(foo (boo bar baz) bing bang)
          ^----------------^
              selection
  1. Shring backwards sexp
(foo (boo bar baz) bing bang)
          ^-----------^
            selection

If for 3 you just popped off the selection stack, you'd end up back at 2 rather than 3. It sounds like your're thinking about that with your approach of switching the cursors.

One more thing to think about is the behavior when you move the cursor while selecting. In normal select mode, the cursor moves from its current position and the "selection start location" is lost.

@PEZ
Copy link
Collaborator

PEZ commented Mar 10, 2021

On the now deleted comment about this shrink implementation sounding more like undo than shrink: Indeed. It is called contract/shrink in old Paredit tradition. I just choose to implement it using this stack and it turned out to be a good choice since I could hook other selection commands on it. But if you use the grow command, then shrink, it will shrink. =)

Also turns out I was a bit stupid when implementing the select forward/backward commands. Having the help of this issue to think about it, it was pretty easy to make this happen:

B56CCE0C-E082-4A03-82C2-6317C0835384-96651-0000403B1BEF899E

@max-reify , I wasn't quite thinking to pop to implement the behaviour. It is all grow (push) actions. If you look at this gif, you'll see that after having selected forward and backward a few times, I then start to pop the stack and it all plays backwards.

This fix coming to a Marketplace near you soon. I just need to figure out the case when selecting backward up, then forward something because then the cursor will have left the current form...

PEZ added a commit that referenced this issue Mar 10, 2021
@PEZ
Copy link
Collaborator

PEZ commented Mar 10, 2021

So, still not sure what to do about the forward-up, then backward-something, case. Might decide that we can live with it, or might figure out how to cleanly do the right thing there.

Here's a VSIX you can try the fix with, @max-reify : https://12044-125431277-gh.circle-artifacts.com/0/tmp/artifacts/calva-2.0.180-1062-select-back-fwd-df06c31b.vsix

@bpringe
Copy link
Member

bpringe commented Mar 11, 2021

select

I tested the PR vsix and not sure if it's behaving as you expect. I select forward sexp twice then select backward. After selecting forward twice, I expect when I select backward to essentially shrink the selection, so it should go back to what was selected when I had selected forward once.

@bpringe
Copy link
Member

bpringe commented Mar 11, 2021

(|+ 1 2)

Select forward sexp:

(+ 1 2)
 ^-^
 selection

Select forward sexp:

(+ 1 2)
 ^---^
 selection

Select backward sexp (expected):

(+ 1 2)
 ^-^
 selection

Select backward sexp (actual):

(|+ 1 2) ;; no selection

@PEZ
Copy link
Collaborator

PEZ commented Mar 11, 2021

Oh, yes. Back to the drawing board. Thanks!

@PEZ
Copy link
Collaborator

PEZ commented Mar 11, 2021

Things where of course not as simple as I had envisioned them to be, but it's not super complicated either. At least not the the point where it is now, where it is still glitching a bit depending on where you start the selection and if you go backwards or forwards across lines.

Also not sure if I've gotten the up-down deselections right yet (and even might have broken the selections). But thinking that if some more people test we can get a better picture.

Here's the latest VSIX: https://12054-125431277-gh.circle-artifacts.com/0/tmp/artifacts/calva-2.0.180-1062-select-back-fwd-bfb9c1f7.vsix

@bpringe
Copy link
Member

bpringe commented Mar 11, 2021

Seems to be working well in my testing.

@PEZ
Copy link
Collaborator

PEZ commented Mar 15, 2021

I think this is addressed now. Please reopen if I am wrong.

@PEZ PEZ closed this as completed Mar 15, 2021
@max-reify
Copy link
Author

Works perfectly in my testing, thank you! This has been one of the most positive open source experiences I've ever had.

@PEZ
Copy link
Collaborator

PEZ commented Mar 15, 2021

This has been one of the most positive open source experiences I've ever had.

Oh, wow. You just made my day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants