Add surrounds support for vim #9400

weartist · 2024-03-15T15:40:33Z

There are still some minor issues:

When change the surround and delete the surround, we should also decide whether there are spaces inside after deleting/replacing according to whether it is open parentheses, and replace them accordingly, but at present, delete and change, haven't done this adaptation for current pr, I'm not sure if I can fit it in the back or if it needs to be fitted together.
In the selection mode, pressing s plus brackets should also trigger the Add Surrounds function, but this MR has not adapted the selection mode for the time being, I think we need to support different add behaviors for the three selection modes.(Currently in select mode, s is used for Substitute)
For the current change surrounds, if the user does not find the bracket that needs to be matched after entering cs, but it is a valid bracket, and will wait for the second input before failing, the better practice here should be to return to normal mode if the first bracket is not found
I reused BracketPair in language, but two of its properties weren't used in this mr, so I'm not sure if I should create a new struct with only start and end, which would have less code

I'm not sure which ones need to be changed in the first issue, and which ones can be revised in the future, and it seems that they can be solved

Release Notes:

vim: Add support for vim-surround. Note that HTML tags are not yet supported. (Vim mode surrounds #4965)

weartist · 2024-03-15T16:08:52Z

The first question is being revised and shouldn't take much time, I'll fix it tomorrow

mrnugget · 2024-03-15T17:23:55Z

I haven't looked at the code yet, but one thing that popped into my mind yesterday: would it make more sense in Zed to make surround mode work with tree-sitter and multi-cursors? As in: vs" selects the surrounding "?

Or put another way: can we do something better than what Vim has by using tree-sitter and multi-cursors? Because Vim didn't have those tools, vim-surround also has some things that I always found a bit awkward, such as changing the surrounding tag. With multi-cursors it's relatively easy though: vst to select tags, then hit c and you're good to go, right?

weartist · 2024-03-16T13:51:03Z

I haven't looked at the code yet, but one thing that popped into my mind yesterday: would it make more sense in Zed to make surround mode work with tree-sitter and multi-cursors? As in: vs" selects the surrounding "?

Or put another way: can we do something better than what Vim has by using tree-sitter and multi-cursors? Because Vim didn't have those tools, vim-surround also has some things that I always found a bit awkward, such as changing the surrounding tag. With multi-cursors it's relatively easy though: vst to select tags, then hit c and you're good to go, right?

Lol, vstc to change the tag I think it is indeed very convenient, at the same time select two cursors and then make any input, vs "c can also change both sides through two cursors".
Do we also need to click s and " or other parentheses in the selection mode to make the current selected Chinese words add parentheses?

mrnugget · 2024-03-18T08:57:44Z

Do we also need to click s and " or other parentheses in the selection mode to make the current selected Chinese words add parentheses?

Not sure I understand correctly. Do you mean S? I think that make sense, for muscle-memory alone.

Again: I didn't look at the code yet and I also feel bad about possibly derailing you here, but I just wanted to bring my idea about "what if multi-cursors make vim-surround easier to implement?" up so that we might end up with an easier implementation that might also fit better into Zed.

weartist · 2024-03-18T09:01:57Z

Do we also need to click s and " or other parentheses in the selection mode to make the current selected Chinese words add parentheses?

Not sure I understand correctly. Do you mean S? I think that make sense, for muscle-memory alone.

Again: I didn't look at the code yet and I also feel bad about possibly derailing you here, but I just wanted to bring my idea about "what if multi-cursors make vim-surround easier to implement?" up so that we might end up with an easier implementation that might also fit better into Zed.

That's okay, do we need to close this pr, I really didn't use a multi-cursor to support add and change

mrnugget · 2024-03-18T09:32:03Z

That's okay, do we need to close this pr, I really didn't use a multi-cursor to support add and change

Not sure. What I'd propose, if you're up for it: investigate whether what I proposed is even doable and if so how much work it would be. If it turns out that it is doable and a reasonable amount of work, then we can close this PR here and try the other approach. But if there are blockers, then we can continue here. What do you think?

weartist · 2024-03-18T13:58:07Z

That's okay, do we need to close this pr, I really didn't use a multi-cursor to support add and change

Not sure. What I'd propose, if you're up for it: investigate whether what I proposed is even doable and if so how much work it would be. If it turns out that it is doable and a reasonable amount of work, then we can close this PR here and try the other approach. But if there are blockers, then we can continue here. What do you think?

I will willing to do some investigate. With multi-cursor, we only need to select the corresponding pair of symbols for each current cursor, and we can add, change and delete. I think this is indeed more convenient(Maybe deletion will be a little more troublesome than ds), but it may not be consistent with many people's habits.
maybe we need to confirm whether you can conveniently add cursor and change select the current cursor. I think it should be fine.

mrnugget · 2024-03-18T17:29:46Z

Okay, @ConradIrwin pointed something out to me that I should've thought of 3 comments ago: this won't work for pairs that don't have the same start/end, i.e. {} or [], because when you then wnat to change them you end up with {{ etc. So, abort my plan :)

weartist · 2024-03-19T01:21:51Z

Okay, @ConradIrwin pointed something out to me that I should've thought of 3 comments ago: this won't work for pairs that don't have the same start/end, i.e. {} or [], because when you then wnat to change them you end up with {{ etc. So, abort my plan :)

I also didn't think of this, in multi-cursor selection mode if we want to automatically enter matching parentheses e.g. we type { and current two cursors finish with '{' and '}', we may need at least some flags or extra logic to determine this, as there are other occasions when multiple cursors are more likely to trigger

weartist · 2024-03-19T01:41:06Z

@mrnugget So let's move on to this PR? I'll fix the first issue, maybe the third one I can fix as well, and then you can review the code and see the rest of the issues XD

ConradIrwin · 2024-03-19T05:20:03Z

@weartist I think we should fix 1 and 3 as you said; 2 seems fine for now.

For 4. I think it's fine to re-use BracketPair, but if you wanted to make the code a little shorter I think you could just use a rust tuple of (start, end) or a struct with two fields.

weartist · 2024-03-19T06:22:16Z

@weartist I think we should fix 1 and 3 as you said; 2 seems fine for now.

For 4. I think it's fine to re-use BracketPair, but if you wanted to make the code a little shorter I think you could just use a rust tuple of (start, end) or a struct with two fields.

Ok, let me fix 1 and 3 and then you can review code, they may have some tweaks and optimizations. After the fix, I'll take a look at the use about of BracketPair, wait me XD

crates/vim/src/motion.rs

crates/vim/src/surrounds.rs

ConradIrwin · 2024-03-20T02:24:10Z

Thanks for this, some comments in line and bugs I noticed:

hello ysiw" => "hello" cs"' => ' hello' (should be 'hello')
( a ) cs)} => { a } (should be {a}).

I also notice there is no support for HTML tags at all. I think people will need this to feel like we have support for surround.vim, but I am happy to do that as a second PR as it adds quite a bit of complexity and the non-tag version is still useful.

As always happy to talk this through if you need more help.

weartist · 2024-03-20T02:50:53Z

@ConradIrwin A little bad message, I might need to make some changes to the code, currently my deal is 'expand_selection', but I find that this can only be checked backwards, e.g. in the current case:
test teˇst "test" type: ds"
It can be done well, but if it is
test "tes"t teˇst type: ds"
expand_selection doesn't look forward
I'm going to switch to a tree-sitter-based scheme for matching, how do you feel?
another small question, such as ds, cs, whether we should only judge whether the current line has matching content, I think it may be

weartist · 2024-03-22T13:44:05Z

@ConradIrwin I looked at move_point related code and need some help wanted to confirm with you the logic for using the move_point for surround, and if we change it to move_point, the delete method looks like this:
We have a way to generate a find motion, for example:

fn get_find_motion(target: char, is_forward: bool) -> Motion {
    if is_forward {
        Motion::FindForward {
            before: false,
            char: target,
            mode: FindRange::MultiLine,
            smartcase: false,
        }
    } else {
        Motion::FindBackward {
            after: false,
            char: target,
            mode: FindRange::MultiLine,
            smartcase: false,
        }
    }
}

If the user enters ds{:
First we look forward to {, if we find it, move the cursor to the location of {, and generate an edit, set the content of the corresponding position to "",
Since the user is typing {, if you can we want to remove the space next to the parentheses, we look backwards for " ", and determine if it is the cursor next to it, if yes, we are generating an edit (or merging with the previous edit),

and so on to deal }

The logic of change is similar to delete. and add,We just need to change the FindForward and FindBackward motion to PreviousWordStart NextWordEnd, If the process is fine, I'll change the code as soon as possible, thanks again~

ConradIrwin · 2024-03-25T12:05:38Z

I think we should still use the objects and not FindForward/FindBackward because they (should) handle things like "abc\"def".

we may need to change the code to operate on ranges and not selections.

weartist · 2024-03-25T13:14:00Z

I think we should still use the objects and not FindForward/FindBackward because they (should) handle things like "abc\"def".

we may need to change the code to operate on ranges and not selections.

I also think that using lookup may be a bit cumbersome, and the scope of use is relatively simple, but there may be some cases where the scope of use cannot be handled well, for example, when selecting "", the subsequent spaces may be selected together, of course, we can also handle this part manually, I will try to add an extraction range instead of the selected method :)

weartist · 2024-03-27T11:10:14Z

@ConradIrwin Submitted a new modified version, you can check if the current logic is suitable, please just look at the delete_surrounds and change_surrounds, because add_surrounds use motion, I haven't changed the way motion gets range, if the current is ok I'll continue to complete the add part XD

ConradIrwin · 2024-04-03T02:46:54Z

@weartist sorry for the slow reply, I was out last week!

Yes, the new approach is much better, thank you!
BUT display_map.chars_at and display_map.reverse_chars_at include soft-wrapping and inline hints which means you cannot use those methods, you must use map.buffer_snapshot.chars_at instead.

weartist · 2024-04-03T04:06:58Z

@weartist sorry for the slow reply, I was out last week!

Yes, the new approach is much better, thank you!

BUT display_map.chars_at and display_map.reverse_chars_at include soft-wrapping and inline hints which means you cannot use those methods, you must use map.buffer_snapshot.chars_at instead.

Don't mind, I'm glad I'm on the right track, I'll continue the rest of the work XD

ConradIrwin · 2024-04-03T04:13:20Z

Once #10103 is merged, you should be able to use buffer_chars_at instead, which may make it a bit easier to iterate and keep the position tracked at the same time.

weartist · 2024-04-03T04:16:56Z

Once #10103 is merged, you should be able(能) to use buffer_chars_at instead, which may make it a bit(位) easier to iterate and keep the position(位置) tracked at the same time.

Thanks!

…t_for_vim

weartist · 2024-04-04T14:37:17Z

@ConradIrwin I submitted the latest code, this pr does not contain the surround in visual mode, the s shortcut conflicts with the existing Substitute, so it only includes the surround support in normal mode at the moment.
At present, there are some little problems:

The vim surround plugin will also look for a matching surround in front of the cursor of the current line, such as the following complete content:
’‘’
var foo = "foo"; ˇvar bar =
‘’‘
Do a delete or change operation on this text to find it", vim will locate the pair in front of it, but our current range method can't return the pair in front of us, if we need to support this, we can not pass in the offset corresponding to the current cursor when querying the range, but pass in the offset at the beginning of the current line of the cursor to support it, but need special processing, content of this part can be referred to:
https://github.com/weartist/zed/blob/627f41bde95f9f49e3fd2e59cb0796918705e614/crates/vim/src/surrounds.rs#L622C1-L624C22
In the case of multiple cursors, it is possible that multiple cursors hit the same pair, and currently in this case, we will merge the cursors, and I'm not sure if this situation requires an extra judgment, if there are multiple cursors matching and it is the same parentheses, only let one cursor move to the corresponding position, and the other cursors remain the same.The content of this part can be referred to:https://github.com/weartist/zed/blob/627f41bde95f9f49e3fd2e59cb0796918705e614/crates/vim/src/surrounds.rs#L707C1-L710C22
In vim, execute the add and change commands, it will first select the target range, wait for the last input, and currently zed will clear the operation stack when switching mode, so it is not consistent with vim for the time being

ConradIrwin · 2024-04-05T04:45:40Z

Thanks! I will test this more thoroughly tomorrow, but those limitations seem probably ok to me.

weartist · 2024-04-05T07:50:39Z

A new problem has been discovered, if the matching parentheses are not on the current screen (e.g. after 200 lines), may need to position the cursor to the position of the symbol when manipulating

ConradIrwin · 2024-04-08T17:34:43Z

Luckily that's a straightforward fix, pushed to your branch.

Thanks for all your hard work on this!

baldwindavid · 2024-04-08T19:28:18Z

Incredible work. This is a big one to knock off the list!...

mrnugget · 2024-04-09T07:32:26Z

Woooooo! 🥳

screenshot-2024-04-09-09.31.08.mp4

d1y · 2024-04-11T06:28:47Z

Hope S" to work :)

Screen.Recording.2024-04-11.at.14.26.31.mov

ConradIrwin · 2024-04-11T21:22:44Z

@d1y unfortunately S in visual mode is already taken by vim (for substitute likewise); and I'm reluctant to introduce non-standard keybindings that conflict with standard ones by default.

Even more unfortunately, we don't (as far as I can figure out) have enough mechanisms in place to allow you to rebind S yourself yet, we'd need a way of specifying the "range currently highlighted in visual mode", which we don't have...

Please do open an issue for this, as it seems nice to have an option to make it work even if it's not the default.

quarkw · 2024-04-12T09:42:59Z

@d1y @ConradIrwin

the S in visual mode can be kind-of simulated using keybinds

  {
    //vim visual mode
    "context": "Editor && vim_mode == visual && !VimWaiting && !VimObject",
    "bindings": {
      "shift-s '": ["workspace::SendKeystrokes", "d i ' ' left left ctrl-g p"],
      "shift-s `": ["workspace::SendKeystrokes", "d i ` ` left left ctrl-g p"],
      "shift-s \"": [
        "workspace::SendKeystrokes",
        "d i \" \" left left ctrl-g p"
      ],
      "shift-s [": [
        "workspace::SendKeystrokes",
        "d i [ space space ] left left left ctrl-g p"
      ],
      "shift-s ]": ["workspace::SendKeystrokes", "d i [ ] left left ctrl-g p"],
      "shift-s {": [
        "workspace::SendKeystrokes",
        "d i { space space } left left left ctrl-g p"
      ],
      "shift-s }": ["workspace::SendKeystrokes", "d i { } left left ctrl-g p"],
      "shift-s (": [
        "workspace::SendKeystrokes",
        "d i ( space space ) left left left ctrl-g p"
      ],
      "shift-s )": ["workspace::SendKeystrokes", "d i ( ) left left ctrl-g p"]
    }
  },
  {
    //vim insert mode
    "context": "Editor && vim_mode == insert",
    "bindings": {
      "ctrl-g p": "vim::Paste" //Uses the vim clipboard
    }
  }

It needs to be defined for every surround type, as you can see above
The default S functionality still works with this setup in visual mode, but there is a bit of a delay since the editor waits to see if you chord anything afterwards. In other modes it still works without any delay

I have an additional keybind to use vim paste in insert mode, since I have my vim settings set to only update the system clipboard on yank. I think if you have the default vim settings, you can omit the insert mode keybinding, and replace al instances of ctrl-g p with cmd-v. You may have to play around with the number of lefts as I believe the non-vim and the vim pastes in insert mode paste to slightly different positions. The non-vim paste may also change the mode, so you'll have to account for that if that's the case too

Using ctrl-g in a shortcut also depends on this keybind to disable the default ctrl-g functionality. Or you could change the keybind for vim::Paste to something else

  {
    "context": "Editor",
    "bindings": {
      "ctrl-g": ["workspace::SendKeystrokes", "escape"]
    }
  },

add surrounds support for vim

f4762cc

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Mar 15, 2024

weartist added 2 commits March 15, 2024 23:43

💄

2d03042

💄

9221a18

maxdeviant changed the title ~~add surrounds support for vim~~ Add surrounds support for vim Mar 15, 2024

ConradIrwin self-assigned this Mar 19, 2024

adjust delete surround logic

189f8e0

weartist marked this pull request as draft March 19, 2024 07:57

weartist added 5 commits March 19, 2024 15:58

adjust format

8f447c3

💄

e5b1e17

adjust delete surround

62adc43

adjust change surround

67dc943

💄

b820452

ConradIrwin reviewed Mar 20, 2024

View reviewed changes

crates/vim/src/motion.rs Show resolved Hide resolved

ConradIrwin reviewed Mar 20, 2024

View reviewed changes

crates/vim/src/surrounds.rs Show resolved Hide resolved

ConradIrwin reviewed Mar 20, 2024

View reviewed changes

crates/vim/src/surrounds.rs Outdated Show resolved Hide resolved

weartist added 3 commits March 27, 2024 18:57

adjust change_surrounds and delete_surrounds

dd42bf7

Merge branch 'main' into add_surround_support_for_vim

fffb877

💄

7116cc8

weartist added 2 commits April 3, 2024 15:25

Merge remote-tracking branch 'upstream/main' into add_surround_suppor…

9970e95

…t_for_vim

Replace the chars_at with buffer_chars_at

5c17190

weartist mentioned this pull request Apr 3, 2024

Refactor selection expansion logic into a separate method #10117

Merged

weartist added 4 commits April 4, 2024 21:54

Merge remote-tracking branch 'upstream/main' into add_surround_suppor…

c2e62c2

…t_for_vim

Adjust the logic code related to surround

5e19184

💄

9311502

💄

627f41b

auto-scroll when changing selections

9b71e84

ConradIrwin merged commit 44aed4a into zed-industries:main Apr 8, 2024
8 checks passed

Benjamin-Davies mentioned this pull request Jun 17, 2024

Add action for vim surround in visual mode #13122

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add surrounds support for vim #9400

Add surrounds support for vim #9400

weartist commented Mar 15, 2024 •

edited by ConradIrwin

Loading

weartist commented Mar 15, 2024 •

edited

Loading

mrnugget commented Mar 15, 2024

weartist commented Mar 16, 2024

mrnugget commented Mar 18, 2024

weartist commented Mar 18, 2024

mrnugget commented Mar 18, 2024

weartist commented Mar 18, 2024

mrnugget commented Mar 18, 2024

weartist commented Mar 19, 2024

weartist commented Mar 19, 2024

ConradIrwin commented Mar 19, 2024

weartist commented Mar 19, 2024

ConradIrwin commented Mar 20, 2024

weartist commented Mar 20, 2024

weartist commented Mar 22, 2024 •

edited

Loading

ConradIrwin commented Mar 25, 2024

weartist commented Mar 25, 2024

weartist commented Mar 27, 2024

ConradIrwin commented Apr 3, 2024

weartist commented Apr 3, 2024 •

edited

Loading

ConradIrwin commented Apr 3, 2024

weartist commented Apr 3, 2024

weartist commented Apr 4, 2024 •

edited

Loading

ConradIrwin commented Apr 5, 2024

weartist commented Apr 5, 2024

ConradIrwin commented Apr 8, 2024

baldwindavid commented Apr 8, 2024

mrnugget commented Apr 9, 2024

d1y commented Apr 11, 2024

ConradIrwin commented Apr 11, 2024

quarkw commented Apr 12, 2024 •

edited

Loading

Add surrounds support for vim #9400

Add surrounds support for vim #9400

Conversation

weartist commented Mar 15, 2024 • edited by ConradIrwin Loading

weartist commented Mar 15, 2024 • edited Loading

mrnugget commented Mar 15, 2024

weartist commented Mar 16, 2024

mrnugget commented Mar 18, 2024

weartist commented Mar 18, 2024

mrnugget commented Mar 18, 2024

weartist commented Mar 18, 2024

mrnugget commented Mar 18, 2024

weartist commented Mar 19, 2024

weartist commented Mar 19, 2024

ConradIrwin commented Mar 19, 2024

weartist commented Mar 19, 2024

ConradIrwin commented Mar 20, 2024

weartist commented Mar 20, 2024

weartist commented Mar 22, 2024 • edited Loading

ConradIrwin commented Mar 25, 2024

weartist commented Mar 25, 2024

weartist commented Mar 27, 2024

ConradIrwin commented Apr 3, 2024

weartist commented Apr 3, 2024 • edited Loading

ConradIrwin commented Apr 3, 2024

weartist commented Apr 3, 2024

weartist commented Apr 4, 2024 • edited Loading

ConradIrwin commented Apr 5, 2024

weartist commented Apr 5, 2024

ConradIrwin commented Apr 8, 2024

baldwindavid commented Apr 8, 2024

mrnugget commented Apr 9, 2024

d1y commented Apr 11, 2024

ConradIrwin commented Apr 11, 2024

quarkw commented Apr 12, 2024 • edited Loading

weartist commented Mar 15, 2024 •

edited by ConradIrwin

Loading

weartist commented Mar 15, 2024 •

edited

Loading

weartist commented Mar 22, 2024 •

edited

Loading

weartist commented Apr 3, 2024 •

edited

Loading

weartist commented Apr 4, 2024 •

edited

Loading

quarkw commented Apr 12, 2024 •

edited

Loading