vim: Add Multi Replace mode in Vim #8469

weartist · 2024-02-27T08:36:21Z

For #4440, I've only added support for normal, if it's visual mode, would we like this to delete the current selection row and enter insert mode?

schacon · 2024-02-27T09:52:39Z

Heya. It almost works, but there are other hotkeys that take over and switch it back into other modes. For instance, if you go into replace mode and start typing "something", it will work for "som" and then the 'e' switches mode again.

mrnugget · 2024-02-27T09:57:10Z

I was just about to leave a comment, but @schacon beat me to it. I can't get it to work locally: R and then typing some characters doesn't really do anything.

Another thought after looking at the implementation: should we model R as a proper mode? Because R should also support esc to exit it, right? And maaaaybe we then later also want gR for visual replace mode.

weartist · 2024-02-28T01:09:57Z

Heya. It almost works, but there are other hotkeys that take over and switch it back into other modes. For instance, if you go into replace mode and start typing "something", it will work for "som" and then the 'e' switches mode again.

sorry i missed some charges, and the implementation was too simplistic, In order to be able to support more, I'll improve this part of the code, please wait for my latest good news XD

weartist · 2024-02-28T01:13:55Z

I was just about to leave a comment, but @schacon beat me to it. I can't get it to work locally: R and then typing some characters doesn't really do anything.

Another thought after looking at the implementation: should we model R as a proper mode? Because R should also support esc to exit it, right? And maaaaybe we then later also want gR for visual replace mode.

sorry i missed some changes, I was a bit sloppy before, and I thought that the MultiReplace really needs some better design, for example, in MultiReplace mode, the delete function will replace it back to the original content, and the gR mentioned needs to be handled separately

ConradIrwin · 2024-02-28T02:21:37Z

Thanks for this! I'd love to pair with you to get this over the line: https://calendly.com/conradirwin/pairing, otherwise some notes below to get this to a mergeable state.

Let's use a new mode for this (so the mode indicator can say -- REPLACE -- and we can control key bindings).
I think the minimum set of bindings we need is:
- escape ctrl-c and ctrl-[ for switching back to normal mode.
- enter needs to be handled as inserting a line-break before the cursor and continue indentation from the previous line (I hope the existing binding will do the right thing)
- backspace should restore the last character overwritten.
We need at a few tests to cover the above, using the NeovimBackedTestContext so we can easily verify compatibility.

I think we can get away without supporting gR or thinking too hard about tab for a v1 (same with ctrl-w and ctrl-u which are called out in :help Replace-mode). I also think we can probably descope perfecting the interaction with ., though ideally we'd replace the same number of characters again (handling this more similarly to insert mode).

weartist · 2024-02-28T02:24:56Z

Thanks for this! I'd love to pair with you to get this over the line: https://calendly.com/conradirwin/pairing, otherwise some notes below to get this to a mergeable state.

Let's use a new mode for this (so the mode indicator can say -- REPLACE -- and we can control key bindings).

I think the minimum set of bindings we need is:

escape ctrl-c and ctrl-[ for switching back to normal mode.

enter needs to be handled as inserting a line-break before the cursor and continue indentation from the previous line (I hope the existing binding will do the right thing)

backspace should restore the last character overwritten.

We need at a few tests to cover the above, using the NeovimBackedTestContext so we can easily verify compatibility.

I think we can get away without supporting gR or thinking too hard about tab for a v1 (same with ctrl-w and ctrl-u which are called out in :help Replace-mode). I also think we can probably descope perfecting the interaction with ., though ideally we'd replace the same number of characters again (handling this more similarly to insert mode).

Thank you for your help! I will start this journey

weartist · 2024-03-01T09:04:22Z

@ConradIrwin Apologize, but I might need some of your help. I'm familiarizing myself with the vim mode code for this PR, and I've found the following method:
https://github.com/weartist/zed/blob/7c93f6244d4013dd1967189d1ca7c3bd16b76538/crates/vim/src/state.rs#L147C1-L153C6

pub fn vim_controlled(&self) -> bool {
        !matches!(self.mode, Mode::Insert)
            || matches!(
                self.operator_stack.last(),
                Some(Operator::FindForward { .. }) | Some(Operator::FindBackward { .. })
            )
    }

In this method, I feel that the Replace Mode should return false. Some actions should not be accepted in Replace mode like Insert Mode. and in the editor, the input_enabled of the editor is the opposite of this value. If vim_controlled returns false in Replace Mode, input_enabled will be true. Currently, events are only passed to the vim module when input_enabled is false:

https://github.com/weartist/zed/blob/7c93f6244d4013dd1967189d1ca7c3bd16b76538/crates/editor/src/editor.rs#L9333C1-L9343C1

fn replace_text_in_range(
        &mut self,
        range_utf16: Option<Range<usize>>,
        text: &str,
        cx: &mut ViewContext<Self>,
    ) {
        if !self.input_enabled {
            cx.emit(EditorEvent::InputIgnored { text: text.into() });
            return;
        }

So, I'm not sure whether to rely on adding new properties or judgments to pass replace-related methods to vim when input_enabled is true. I'm worried that my thinking might be wrong, so I might need you to help me answer the thinking here.

In addition, there are two small questions. The first one is about restoring the content of backspace. I think we just need to add a vec. Every time we delete, if vec has value, it will be restored to the content of vec.pop(), and clear this vec every time the cursor changes. Because we don't need to handle the situation where the cursor moves to other places after replacing a few characters, and then moves back to backspace. I'm not sure if this is a good solution.

The second question is, when I was understanding the vim_controlled method mentioned above, I found that if vim_controlled returns true, that is, we are still in the controlled situation in Replace mode, all keys will not work after entering replace mode. I found the problem is:

https://github.com/weartist/zed/blob/7bad1eb3f207f6c5362351ae2d4f1fe038b28a85/crates/gpui/src/platform/mac/window.rs#L1215C1-L1229C14

if let Some(ime) = last_ime {
            if let ImeInput::InsertText(text, _) = &ime {
                if !is_composing {
                    window_state.lock().previous_keydown_inserted_text = Some(text.clone());
                    if let Some(callback) = callback.as_mut() {
                        event.keystroke.ime_key = Some(text.clone());
                        handled = callback(PlatformInput::KeyDown(event));
                    }
                }
            }

            if !handled {
                handled = true;
                send_to_input_handler(this, ime);
            }
        }

handled = callback(PlatformInput::KeyDown(event));

This line of code intercepts and handles the input event, and cannot be passed into the editor. If it's convenient, I'd like you to help me explain the logic here. How should I avoid input being intercepted here in some modes?

ConradIrwin · 2024-03-01T20:50:52Z

@weartist I'm also happy to work on this together which may be more effective: https://calendly.com/conradirwin/pairing.

Notes below:

vim_controlled needs some refactoring :D. I think it's equivalent to this:

fn vim_controlled() {
    let is_insert_mode = matches!(self.mode, Mode::Insert)
    if !is_insert_mode {
      true
    }
    matches!(
        self.operator_stack.last(),
        Some(Operator::FindForward { .. }) | Some(Operator::FindBackward { .. })
    )
}

I think we need to have vim_controlled() returning true in Replace mode so that vim gets the keystrokes instead of the editor (this is how r works today).

Given that we'll need to implement the replacement ourselves in vim mode, again you can copy this from the existing replace action. (This will also be helpful when we add ctrl-r " to paste in replace mode so we can be sure to overwrite the correct number of characters).

For backspace a Vec might work, but we'll want to maintain one per selection to work with multi-cursor. I'm not sure if there are edge-cases with <enter> and extending off the end of the line that you won't be able to represent (but I can't immediately think of any).

An alternative idea would be to take a buffer snapshot when we enter replace mode, and then you can just read that to get the previous character at the current position.

weartist · 2024-03-02T12:26:02Z

@weartist I'm also happy to work on this together which may be more effective: https://calendly.com/conradirwin/pairing.

Notes below:

vim_controlled needs some refactoring :D. I think it's equivalent to this:
fn vim_controlled() {
    let is_insert_mode = matches!(self.mode, Mode::Insert)
    if !is_insert_mode {
      true
    }
    matches!(
        self.operator_stack.last(),
        Some(Operator::FindForward { .. }) | Some(Operator::FindBackward { .. })
    )
}
I think we need to have vim_controlled() returning true in Replace mode so that vim gets the keystrokes instead of the editor (this is how r works today).

Given that we'll need to implement the replacement ourselves in vim mode, again you can copy this from the existing replace action. (This will also be helpful when we add ctrl-r " to paste in replace mode so we can be sure to overwrite the correct number of characters).

For backspace a Vec might work, but we'll want to maintain one per selection to work with multi-cursor. I'm not sure if there are edge-cases with <enter> and extending off the end of the line that you won't be able to represent (but I can't immediately think of any).

An alternative idea would be to take a buffer snapshot when we enter replace mode, and then you can just read that to get the previous character at the current position.

Okay, I'll submit a working version, hopefully smooth,

weartist · 2024-03-04T16:01:57Z

@ConradIrwin Sorry im late, I've submitted an implementation based on snapshots, but during my recent testing, I discovered a drawback to this approach:
In replace mode, pressing enter actually starts a new line from the current cursor, which is different from a regular replace
behaviour. It's equivalent to insert a "\n", which causes the current editor's structure to be inconsistent with the structure of the copy when we entered replace mode. As a result, when obtaining the content under the copy corresponding to the current cursor, the obtained content is also incorrect.

e.g.:
init:

abcdˇefg

typing R to into Replace mode
typing enter:

abcd
ˇefg

typing z to replace e to z

abcd
zˇfg

At this point, if we type backspace, our current cursor is on the second line, so we want to find the content of the second line copy, but the copy only has one line, and there are other complications that will cause us to not get the correct content

Because only when entering a "\n" or deleting a "\n" will the current structure change (adding a line and deleting a line), I just had an idea to edit the recorded snapshot copy at the same time every time a newline character is added and deleted. However, snapshots do not support editing. If we want to use this scheme, we can only save a copy of the editor, which doesn't seem like a good idea.

My other proposal is, for each operation in replace mode (replace, insert "\n", delete "\n"), can we use a map to save the reverse operation corresponding to each coordinate, such as undo, redo? The overhead of this idea would not be too large, and for multiple operations on each coordinate, we can save all changes in the form of an array (if someone repeatedly modifies the same position). I'm not sure if you think this is a good solution.

assets/keymaps/vim.json

crates/vim/src/replace.rs

ConradIrwin · 2024-03-04T23:51:23Z

crates/vim/src/replace.rs

+                        let mut range = selection.range();
+                        // "\n" need to be handled separately, because when a "\n" is typing,
+                        // we don't do a replace, we need insert a "\n"
+                        if text.as_ref() != "\n" {


Does mapping "enter": "editor::Newline" in the keymap do the right thing? If not quite we may still want to call editor.newline as that also handles preserving leading indentation/comments etc.

ConradIrwin · 2024-03-04T23:59:29Z

@weartist don't fret – there's no late!

I'm happy to go with the HashMap/Vec idea (most important is that it actually works). As always, I'd love to work with you on this, it's always easier to resolve this kind of question when we're both in the code.

weartist · 2024-03-09T08:30:26Z

@ConradIrwin Hi, I submitted a version based on hashmap to record the original content in different locations in the replacement mode, and it supports multi-cursor and code block folding.
I tested some cases and it worked well, but because of the insert operations, e.g. enter, we may only be able to change the structure of our records at any time when these operations occur, which is a bit cumbersome.

There is also a situation that needs to be considered whether to support auto-indentation by pressing enter, which is not currently supported because auto-indentation may insert many characters, and I don't know how to listen to these insertions in replacement mode

If the current solution is acceptable, I'll submit some unit tests of the boundary case later and further optimize the code :)

…_R_support # Conflicts: # crates/vim/src/motion.rs

ConradIrwin · 2024-03-11T17:19:39Z

@weartist Makes sense. Looking at the current approach, I think it's not a good idea to store previous edits by offset (or even by point) as concurrent edits invalidate the map. We can instead store these by Anchor, which gives us a stable identifier for a position in the doc (at some loss of efficiency, but I suspect the typical number of replaces in replace mode is not more than a few tens and thousands or more will be very rare).

I also think we should not be using an UndoReplace motion, but instead bind the behavior of undo directly on backspace. Most motions in replace mode (up/down/left/right/home/end/pgup/pgdown) should act as they do in normal mode. As we want to support the replace behavior of ctrl-w and ctrl-u we can add direct mappings for those (and I'm hoping there's only a handful of vim mechanisms to delete ranges, so that list should be small-ish).

I'll push these changes to your branch, and then I think the last piece is a test or two. If you are still keen to do those, that'd be great; otherwise I'll pick this up again later in the week.

* Not use motion for now (most motions like up/down/left/right) in replace mode should be handled as in normal mode. * Key replacements off anchors not offsets (this makes them robust to concurrent edits, at the cost of efficiency)

schacon · 2024-03-12T15:14:42Z

🎉 This works great!

schacon · 2024-03-12T15:58:33Z

Been using this for an hour now. Game changer!

weartist · 2024-03-13T02:45:43Z

@weartist Makes sense. Looking at the current approach, I think it's not a good idea to store previous edits by offset (or even by point) as concurrent edits invalidate the map. We can instead store these by Anchor, which gives us a stable identifier for a position in the doc (at some loss of efficiency, but I suspect the typical number of replaces in replace mode is not more than a few tens and thousands or more will be very rare).

I also think we should not be using an UndoReplace motion, but instead bind the behavior of undo directly on backspace. Most motions in replace mode (up/down/left/right/home/end/pgup/pgdown) should act as they do in normal mode. As we want to support the replace behavior of ctrl-w and ctrl-u we can add direct mappings for those (and I'm hoping there's only a handful of vim mechanisms to delete ranges, so that list should be small-ish).

I'll push these changes to your branch, and then I think the last piece is a test or two. If you are still keen to do those, that'd be great; otherwise I'll pick this up again later in the week.

Can we solve the problem of deleting replies if we configure automatic indentation when entering enter? I don't have a clue about this. I can provide test cases, and I've tested a lot of extreme cases when writing these codes. Other modifications may need to wait and so on, I'm writing the vim mode surround, hopefully it will be done today so I can continue over the weekend

weartist · 2024-03-13T02:47:15Z

@ConradIrwin You're moving so quickly! and I'm just realizing that you've already submitted a change

ConradIrwin · 2024-03-13T02:57:20Z

@weartist it seems to work for me and auto-indent correctly (the range in the replacements vector covers the new newline and indentation and marks the undo as "").

Anything else you think we need to fix?

weartist · 2024-03-13T03:19:20Z

@weartist it seems to work for me and auto-indent correctly (the range in the replacements vector covers the new newline and indentation and marks the undo as "").

Anything else you think we need to fix?

Looks cool, I'll test the issue in its entirety and remove some unnecessary comments

weartist · 2024-03-14T05:41:50Z

@ConradIrwin I ran a full test and found a few minor issues:

When executing right at the end of the line or left at the beginning of the line, neovim will not cross the line, but zed will
The cursor is in the first column of the first row and the backspace will perform the insertion, as you said
The following example will recover the error:
"ˇabc"
type: shift-r, 123
current is "123ˇ"
type left
current is "12ˇ3"
type 4
current is "124ˇ"
notice, type backspace
current "12ˇ3"
type backspace, backspace
current "ˇab3"

I'm not sure if this situation needs to be fixed in this commit, or as a follow-up fix to a known issue, and the cursor movement is inconsistent with neovim behavior, I didn't add the corresponding test either

…pport

* If the cursor is at the start more backspaces continually re-insert the first character * If you have two multi-cursors that overlap and then overlap on undo, this now works.

ConradIrwin · 2024-03-15T02:13:00Z

Thanks for this!

This is fine (we should probably fix it for both replace mode and insert mode by binding Left/Right to the vim motions instead of the editor motions in that case).
Hah, that's a funny bug! Looks like a relatively easy fix though, I've pushed to this branch.
I had noticed this in testing, and thought the zed behavior made more sense. When I was working on undo, I came across this interesting snippet in the docs, which I think explains why vim acts as it does. Let's keep this the zed way until someone complains :D.

3. Special special keys                         ins-special-special

The following keys are special.  They stop the current insert, do something,
and then restart insertion.  This means you can do something without getting
out of Insert mode.  This is very handy if you prefer to use the Insert mode
all the time, just like editors that don't have a separate Normal mode. You
can use CTRL-O if you want to map a function key to a command.

The changes (inserted or deleted characters) before and after these keys can
be undone separately.  Only the last change can be redone and always behaves
like an "i" command.

weartist · 2024-03-15T02:21:59Z

Thanks for this!

This is fine (we should probably fix it for both replace mode and insert mode by binding Left/Right to the vim motions instead of the editor motions in that case).

Hah, that's a funny bug! Looks like a relatively easy fix though, I've pushed to this branch.

I had noticed this in testing, and thought the zed behavior made more sense. When I was working on undo, I came across this interesting snippet in the docs, which I think explains why vim acts as it does. Let's keep this the zed way until someone complains :D.
3. Special special keys                         ins-special-special

The following keys are special.  They stop the current insert, do something,
and then restart insertion.  This means you can do something without getting
out of Insert mode.  This is very handy if you prefer to use the Insert mode
all the time, just like editors that don't have a separate Normal mode. You
can use CTRL-O if you want to map a function key to a command.

The changes (inserted or deleted characters) before and after these keys can
be undone separately.  Only the last change can be redone and always behaves
like an "i" command.

Learning the repair idea of the second problem is worth being happy XD, I didn't find any other issues, and I guess the third case has very few times to trigger when using it normally, and it's cool at the moment

ConradIrwin · 2024-03-15T02:32:36Z

Thanks so much for your work on this! Quite the journey :D.

This will likely ship to preview on Wednesday in 0.128.0-pre

schacon · 2024-03-19T08:25:53Z

@weartist thanks so much for picking this up! Loving it.

weartist · 2024-03-19T12:33:27Z

@weartist thanks so much for picking this up! Loving it.

Thanks for your gift! Have a nice day!

add MultiReplace for Normal Vim Mode

b43aee6

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 27, 2024

💄

baeebfe

ConradIrwin self-assigned this Feb 27, 2024

add missing submissions

57b43f3

weartist added 2 commits March 1, 2024 15:39

reset simple implementation

2ac6a88

add simple vim replace mode implement

4de7dcf

weartist marked this pull request as draft March 1, 2024 08:51

Merge remote-tracking branch 'upstream/main' into vim_R_support

7bad1eb

add a simple multi-replace implement

d6791d9

ConradIrwin reviewed Mar 4, 2024

View reviewed changes

assets/keymaps/vim.json Outdated Show resolved Hide resolved

ConradIrwin reviewed Mar 4, 2024

View reviewed changes

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

ConradIrwin reviewed Mar 4, 2024

View reviewed changes

add the vim replace mode support code

056f0fd

weartist marked this pull request as ready for review March 9, 2024 08:30

weartist added 3 commits March 9, 2024 16:33

Merge branch 'main' of https://github.com/zed-industries/zed into vim…

67b95c6

…_R_support # Conflicts: # crates/vim/src/motion.rs

💄

2ead48e

💄

adf5d16

Tidy up replace a little

1978432

* Not use motion for now (most motions like up/down/left/right) in replace mode should be handled as in normal mode. * Key replacements off anchors not offsets (this makes them robust to concurrent edits, at the cost of efficiency)

ConradIrwin force-pushed the vim_R_support branch from b6fd6fa to 1978432 Compare March 11, 2024 17:24

add test for replace mode

7611981

weartist and others added 4 commits March 14, 2024 13:43

Merge branch 'main' into vim_R_support

f5172cf

add replace mode test

f7a9dcd

Merge branch 'vim_R_support' of github.com:weartist/zed into vim_R_su…

20c2f5e

…pport

Fix edge cases in replace

1a759c3

* If the cursor is at the start more backspaces continually re-insert the first character * If you have two multi-cursors that overlap and then overlap on undo, this now works.

💄

70642ca

delete resolved Issue Comments

65cab75

ConradIrwin merged commit e836a97 into zed-industries:main Mar 15, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vim: Add Multi Replace mode in Vim #8469

vim: Add Multi Replace mode in Vim #8469

weartist commented Feb 27, 2024

schacon commented Feb 27, 2024

mrnugget commented Feb 27, 2024

weartist commented Feb 28, 2024 •

edited

Loading

weartist commented Feb 28, 2024 •

edited

Loading

ConradIrwin commented Feb 28, 2024

weartist commented Feb 28, 2024

weartist commented Mar 1, 2024 •

edited

Loading

ConradIrwin commented Mar 1, 2024 •

edited

Loading

weartist commented Mar 2, 2024

weartist commented Mar 4, 2024 •

edited

Loading

ConradIrwin Mar 4, 2024

ConradIrwin commented Mar 4, 2024

weartist commented Mar 9, 2024

ConradIrwin commented Mar 11, 2024

schacon commented Mar 12, 2024

schacon commented Mar 12, 2024

weartist commented Mar 13, 2024

weartist commented Mar 13, 2024

ConradIrwin commented Mar 13, 2024

weartist commented Mar 13, 2024

weartist commented Mar 14, 2024

ConradIrwin commented Mar 15, 2024

weartist commented Mar 15, 2024

ConradIrwin commented Mar 15, 2024

schacon commented Mar 19, 2024

weartist commented Mar 19, 2024

vim: Add Multi Replace mode in Vim #8469

vim: Add Multi Replace mode in Vim #8469

Conversation

weartist commented Feb 27, 2024

schacon commented Feb 27, 2024

mrnugget commented Feb 27, 2024

weartist commented Feb 28, 2024 • edited Loading

weartist commented Feb 28, 2024 • edited Loading

ConradIrwin commented Feb 28, 2024

weartist commented Feb 28, 2024

weartist commented Mar 1, 2024 • edited Loading

ConradIrwin commented Mar 1, 2024 • edited Loading

weartist commented Mar 2, 2024

weartist commented Mar 4, 2024 • edited Loading

ConradIrwin Mar 4, 2024

Choose a reason for hiding this comment

ConradIrwin commented Mar 4, 2024

weartist commented Mar 9, 2024

ConradIrwin commented Mar 11, 2024

schacon commented Mar 12, 2024

schacon commented Mar 12, 2024

weartist commented Mar 13, 2024

weartist commented Mar 13, 2024

ConradIrwin commented Mar 13, 2024

weartist commented Mar 13, 2024

weartist commented Mar 14, 2024

ConradIrwin commented Mar 15, 2024

weartist commented Mar 15, 2024

ConradIrwin commented Mar 15, 2024

schacon commented Mar 19, 2024

weartist commented Mar 19, 2024

weartist commented Feb 28, 2024 •

edited

Loading

weartist commented Feb 28, 2024 •

edited

Loading

weartist commented Mar 1, 2024 •

edited

Loading

ConradIrwin commented Mar 1, 2024 •

edited

Loading

weartist commented Mar 4, 2024 •

edited

Loading