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

surround as selection #368

Closed
wants to merge 3 commits into from
Closed

Conversation

bonega
Copy link
Contributor

@bonega bonega commented Jun 24, 2021

I am proposing an alternative to at least part of match-mode.

Current implementation has one big drawback:
It doesn't visually show what text will be affected(VIM way).

My implementation fixes this, but has a very different design.

Solution:
m followed by match-char will make a selection of nearest enclosing pair by counting open/close.

example with brackets being cursor position: Some (text (here)[ ) ]
Pressing m( will result in Some [ ( ]text (here)[ ) ]

Now we have two normal cursors, so all commands like d or c works as expected.
But we have one important modification to the replace command:

When exactly two selections of length 1 are present and they match according to get_pair, we will do replacement as a pair.
Some [ ( ]text (here)[ ) ]
press r{
Some [ { ]text (here)[ } ]
Note that this replace will happen whenever the selection matches the conditions, it could have been created without m.

What I intend to fix:
replace should enforce that pairs will be balanced after. (this is mostly handled already if selection was made by m)
Condition for matching replace should be extended to selection pairs of count_selections % 2 == 0

I know that there are a few more special cases to handle, but I want feedback on this idea before I move forward with it.
My WIP is working quite nicely I think, please give it a try

@sudormrfbin
Copy link
Member

A selection first implementation is what I tried at first too since it's more aligned with helix's editing model, but there are some problems:

  1. With replace (r), the behavior differs according to the number of cursors and the length of the selections - with two cursors m(r{ will replace () with {}, but with three cursors it's a normal replace. If we extend this to count_selections % 2 == 0, even number of cursors will have special behavior and odd number of cursors will have normal behavior which will be surprising.

  2. With change (c), a new hook would have to be implemented to insert the corresponding pairs at both cursors and this would clash with the autopairs functionality:

    surround-change

    Now we need a new mode to keep track of whether we made the selections using m and then disable the autopairs hook if so (this would also be at odds with being able to make normal selections without m and act on them naturally as you mentioned.)

How are you planning to change the way adding surround pairs work ? Does it use i/a ? Could you elaborate ?

Here's the previous discussion regarding the selection first approach - #320 (comment).

@bonega
Copy link
Contributor Author

bonega commented Jun 24, 2021

Thanks for your reply!

  1. I agree that it is surprising, but I think it is probably a worthwhile trade off.
    I think that normal usage is very unlikely to create three+ cursors on unbalanced pairs.
    The by far most likely way to create cursors with pair characters is using m, which will enforce balance.
    If any cursor contains non-pair characters I fail to see why it would be expected work.

  2. This is about the behavior of pressing ( when in insert mode with multiple cursors I think?
    It is mostly broken behavior by default, so perhaps we can do special logic to fix it when we have balanced cursors.
    Will think about it a little more...

For surrounding the match:
Maybe we could use M( for making a selection(not two cursors) of the pair.

Then we introduce a key pair for placing cursors inside/outside of a selection.
(maybe we also could have it be dual use to contract/extend cursor pairs)

| is cursor.

(hello| world)
press M(
(hello world)
press create_cursors_outside_key
|(hello world)|

(hello| world)
press M(
(hello world)
press create_cursors_inside_key
(|hello world|)

This would also fit with the rest of the movement/selection concept.
|Hello world
press ee
Hello world
press create_cursors_outside_key
Hello |world|

@sudormrfbin
Copy link
Member

  1. I was under the impression that cursors on non pair characters will also do pair replacement ([s]om[e] -> [(]om[)]) - since that doesn't happen I think your solution will work :)

  2. The autopair example I used was buggy; let me use another example - key sequence is i(:

    command
    (Here the behavior of autopairs is correct, it inserts open and close parens on both cursors)

    How will we know what the user intended to insert with the parens ? Was it to type something like (some) on both cursors in which case they want autopairs or was it to surround with ( on the first cursor and ) on the second cursor ? Hence we'd need some kind of mode to switch between them and then some way to enter said mode.

Then we introduce a key pair for placing cursors inside/outside of a selection.

That seems like a good idea, I'll think about it's applications a bit more.

@bonega
Copy link
Contributor Author

bonega commented Jun 25, 2021

I will think about your answers, also have some crazy ideas to test :)

Will be going out for pre-midsummer celebration now here in Sweden.
So will not be much activity from me for the next two days.

Happy midsummer!

@sudormrfbin
Copy link
Member

@bonega we're also on matrix if you want to drop in and say hi ;) https://matrix.to/#/#helix-editor:matrix.org

Happy midsummer to you too :)

@AloeareV
Copy link
Contributor

AloeareV commented Nov 3, 2021

This looks very cool! The codebase has diverged enough that dealing the the merge conflicts looks like it would be non-trivial, as the implementation of Match mode has been significantly refactored, and moved pretty directly to the keybinds section.

With that said, the core of the logic looks intact, and I don't think it would be a lot of work to redo the match-mode changes. @bonega do you have any interest in picking this back up? It looks like a nice quality-of-life change, and possibly more extensible than the current match-mode logic.

@bonega
Copy link
Contributor Author

bonega commented Nov 8, 2021

@AloeareV Thanks for the comments!
I got thrown off the track because my father got sick, so priorities changed.
His estate is closed now, so I will have more time.

I like to revisit this issue given enough time 👍

@pickfire
Copy link
Contributor

pickfire commented May 16, 2022

Is this pull request still valid? I didn't look closely but IIRC @sudormrfbin did some work around this.

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022
@kirawi
Copy link
Member

kirawi commented Jul 14, 2022

Closing as stale. Feel free to re-open the PR if you ever want to continue.

@kirawi kirawi closed this Jul 14, 2022
@kirawi kirawi added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants