Update default vim substitute command behavior and add support for 'g' flag - #28138
Conversation
| return; | ||
| } | ||
|
|
||
| self.active_match_index = Some(0); |
There was a problem hiding this comment.
Not sure if we should directly update the field here, but when trying to use update_matches and activate_match, just like select_last_match does, with and index of 0, the active match index was not being updated, not sure why 🤔
|
@ConradIrwin Two quick questions from my side ✍️
Thanks! 🙂 |
|
@dinocosta awesome, thanks for working on this!
|
|
@dinocosta Also, testing this out, vim's behavior is a bit more complicated: |
Interesting, I was wondering what would be the best way to actually test this! For what it's worth, after running
That's a good catch, thank you for pointing it out! I'll try to work on a fix for this 🙇 |
|
@ConradIrwin Somewhat unrelated but I also just noticed that, with the current Zed release, after running Screen.Recording.2025-04-05.at.15.21.09.movNot sure if this could be considered a bug, but thought it's worth pointing out, in case you want me to take a look at it or to open an issue 👍 |
|
Hmm, that doesn't seem right. There are two possibilities:
|
- Update BufferSearchBar::replace_next to be a public method, so it can be called from Vim::replace_command . - Update the Replacement::parse to set the should_replace_all field to false by default, and only set it to true if the 'g' flag is present in the query. - Add support for when the Replacement.should_replace_all is set to false in Vim::replace_command, so as to have it only replace the next ocurrence instead of all ocurrences in the line.
Update the default behaviour of the substitute command in vim mode to match what NeoVim does when replacing only one ocurrence of a string, where it replaces only the first ocurrence in the line, instead of the first ocurrence relative to the cursor. - Introduce BufferSarchBar::select_first_match to update the active match index to 0, so as to be able to select the first match in the line, regardless of the cursor's position. - Update the Vim::replace_command so as to call BufferSearchBar::select_first_match if should_replace_all is set to false, before replacing the next match with BufferSearchBar::replace_next .
I'm wondering if we could potentially call @ConradIrwin I also took a quick look into actually fixing the issue where running
I believe option 1. would be easier to implement, seeing as all of the methods to do it seem to already be available, albeit it's probably less efficient, while option 2. would probably involve refactoring Let me know what you think, but I might try option 1. in the meantime, just as a proof of concept 👍 |
|
@dinocosta I think option 2. is the correct approach :D. Happy to pair if you want: https://cal.com/conradirwin/pairing We already have a number of options that affect the search, so adding a new one seems fine. I think we can add a check here: zed/crates/editor/src/items.rs Line 1539 in f03efed |
|
@ConradIrwin Thank you so much for the suggestion, I wasn't aware that we could actually convert the
I'll try to implement the suggested change and test it a bit, but we still have a pairing session scheduled for tomorrow, so we can always review the approach then and get your feedback on it! |
This commit introduces the necessary changes in order to support
replacing only the first match per line when using vim's subsitute
command, for example, :s/{pattern}/{string}/ .
In order to support this, the following changes have been introduced:
- Update `SearchQuery::Regex` with a new field, `one_match_per_line`,
which controls whether all matches will be replaced when using replace
all, or if only one match per line should be replaced.
- Add `ONE_MATCH_PER_LINE` to `SearchOptions` struct.
- Update `Vim::replace_command` to set the
`SearchOptions::ONE_MATCH_PER_LINE` option, if
`replacement.should_replace_all` is false.
- Update `SearchQuery::regex` method in order to expect the
`one_match_per_line` argument to be provided, allowing callers to
specify whether the search query should be used to replace only one
match per line.
Lastly, since they're no longer necessary, the
`BufferSearchBar::select_first_match` method has been removed, and the
`BufferSearchBar::replace_next` method has been moved to private again.
|
@ConradIrwin I've pushed 4d209ef which adds the change you proposed and it seems things are working as expected for the
There's two issues I'm aware of that I'll need to fix, but I first wanted to double-check that the approach in that commit seems good before committing to fixing the issues, so do let me know what you think! If the approach looks good, I'll go ahead and attempt the issues:
Thanks! |
|
Amazing! Yes, we'd either need the indicator (or to reset the state after running the replace the first time). This is great. |
Fix two issues introduced with the recent changes that update
:s/{pattern}/{string} to allow only replacing the first occurrence on
each line, namely:
- Fix `:%s/{pattern}/{string}` to actually work on all lines, instead
of only from line 1. This issue was happening because the default point
we were checking against in `editor::items::replace_all` was having its
`row` value set to `0`, which would always end up being the same as the
first line, so that line would be skipped if the `one_match_per_line`
option was set to true.
- Disabled the search bar's `ONE_MATCH_PER_LINE` search option after
running vim's replace command, as having leaving this enabled would
cause the "Replace All Matches" functionality to only replace the first
occurrence on each line, and I'm not sure if this is something Zed
wants to eventually support and add an icon for in the search bar.
Thank you! 🙇 I've pushed cbb54f7 which fixes the two issues I mentioned ! As for the indicator in the search bar, I believe it's a good idea to also expose this functionality to non-vim users, but I'm wondering if that should be tracked in a different issue, as it seems we'd need to involve someone else from Zed's team to create an icon for that, let me know 🤔 |
|
@ConradIrwin Something else I just noticed is that, after releasing these changes it might be better to update Zed's Vim Mode documentation, specifically the section on Replacement, as it states:
This will no longer be true if these changes end up being shipped, let me know if I should also update |
|
We should! I was holding off hitting merge until today's preview is out – Before this change ships to preview (though after it's merged) I'd also like to ship a setting to re-enable the current behavior for those who set |
Awesome! I believe folks who are used to the |
|
Go for it! If you don't get to it this week I'll take a pass. |
With the introduction of #28138, the current vim docs became stale. This PR makes a small update to the docs to reflect this.
With the introduction of zed-industries/zed#28138, the current vim docs became stale. This PR makes a small update to the docs to reflect this.
…' flag (zed-industries#28138) This Pull Request updates the default behavior of the substitute (`s`) command in vim mode to only replace the next match by default, instead of all, and replace all matches only when the `g` flag is provided, making it more similar to NeoVim's behavior. In order to achieve this, the following changes were introduced: - Update `BufferSearchBar::replace_next` to be a public method, so it can be called from `Vim::replace_command` . - Update the `Replacement::parse` to set the `should_replace_all` field to `false` by default, and only set it to `true` if the `'g'` flag is present in the query. - Add support for when the `Replacement.should_replace_all` is set to `false` in `Vim::replace_command`, so as to have it only replace the next occurrence instead of all occurrences in the line. - Introduce `BufferSearchBar::select_first_match` so as to activate the first match on the line under the cursor. Closes zed-industries#24450 Release Notes: - Improved vim's substitute command so as to only replace the first match by default, and replace all matches if the `'g'` flag is provided --------- Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
This Pull Request updates the default behavior of the substitute (
s) command in vim mode to only replace the next match by default, instead of all, and replace all matches only when thegflag is provided, making it more similar to NeoVim's behavior.In order to achieve this, the following changes were introduced:
BufferSearchBar::replace_nextto be a public method, so it can be called fromVim::replace_command.Replacement::parseto set theshould_replace_allfield tofalseby default, and only set it totrueif the'g'flag is present in the query.Replacement.should_replace_allis set tofalseinVim::replace_command, so as to have it only replace the next occurrence instead of all occurrences in the line.BufferSearchBar::select_first_matchso as to activate the first match on the line under the cursor.Closes #24450
Release Notes:
:s//command now defaults to replacing the first match per line (like vim). Use/gto replace all matches.