Adding state to substitution command

[captaincaius]

What this PR does / why we need it:

So you can :s to repeat your previous substitution, just like vim itself.

Which issue(s) this PR fixes

#3067

Special notes for your reviewer:

[shawnaxsom]

I know the other tests here follow this format, but maybe take a look at test/testSimplifier.js, such as asVimInputText and tokenizeKeySequence. Most of our standard tests now use the helpers there to input text in a concise, readable manner. This doesn't have to use the helper, but it might with this call at least.

It could look something like this:

  const enterKeys = keys => modeHandler.handleMultipleKeyEvents(tokenizeKeySequence(keys))
  await enterKeys(['foo', 'bar', 'foo', 'bar'].join('\n'));
  await enterKeys('/bar');
  await waitForCursorSync();

[captaincaius]

Oh, cool! Yeah, that's way easier on the eyes! Thanks!

Sorry about not seeing that... I only looked at the tests nearby and only saw this pattern.

Will put it on my list of things to change for the next commit. :)

[captaincaius]

• rewrite tests using awesome helpers so they're easier on people's eyes :P

[captaincaius]

@shawnaxsom I used newTest, and you were right - it's way better :)

Feel free to let me know if there's anything else.

[rebornix]

Could this part be simplified to

if (!args.pattern) {
  // no pattern found, use previous state
  const prevSubstiteState = vimState.globalState.substituteState; 
  if (prevSubstiteState === undefined || prevSubstiteState.searchPattern === '') { 
    throw VimError.fromCode(ErrorCode.E35); 
  }
  args.pattern = prevSubstiteState.searchPattern; 

  // pattern undefined, use previous replace state
  if (args.pattern === undefined) {
   args.replace = prevSubstiteState.replaceString; 
  }
}

[captaincaius]

If I'm understanding you correctly, I think this is also related to the response below. I tried to make comments to make it clearer, but clearly I should redo them. What do you think if instead of scattering comments into the cases, I write a big comment at the top here, as well as at the top of the substitute parser?

These are the following cases and how vim handles them:
1. :s/this/that
  - standard search/replace
  - update substitution state
  - update search state too!
2. :s
  - use previous SUBSTITUTION state, and repeat previous substitution pattern and replace.
  - do not touch search state!
  - changing substitution state is dont-care cause we're repeating it ;)
3. :s/ or :s// or :s///
  - use previous SEARCH state (not substitution), and DELETE finding from previous pattern (replace with nothing)
  - update substitution state
  - updating search state is dont-care cause we're repeating it ;)
4. :s/this or :s/this/ or :s/this//
  - input is pattern - replacement is empty (delete)
  - update replacement state
  - update search state too!

Do you think a big verbose comment like this would be helpful? Or do you think the tests would be documentation enough?

By the way, one of the reasons I tried so hard to mimic all the cases of cross-state behavior is that I subconsciously rely on this behavior myself with little habits my mind has (e.g. ":s/this/that" then "n" to find the next one, then decide if I want to ":s" or not, then "n"...)

I remember when I first tried to use another IDE-vim thing (I think it was visvim), my habits weren't getting the results I subconsciously expected, so I dropped it - I didn't quite get why it wasn't right until I worked on this issue :P. What I've been loving about VSCodeVim is that whatever's been implemented works freaking perfectly. It really feels like best-of-both-worlds instead of worst-of-both-worlds ;).

PS - I mean no disrespect to visvim - maybe visvim works great now - it must have been at least ten years ago that I'm talking about.

[captaincaius]

@rebornix I decided to add that big verbose comment after all. Feel free to let me know if it's helpful or not.

And again, since tests for all the different sequences are pretty well covered, if you have any suggestions to make this implementation more readable without the comments, I'm all ears :).

[rebornix]

I'm thinking if we can use the same rule for replace as pattern, use previous state when it's undefined, instead of using pattern's value to control if replace` should be used.

[captaincaius]

Ah. I remember wanting to do this, but totally forgot that there was a reason why I couldn't. The replacement actually works differently than the pattern in vim. Contrary to pattern, "empty replacement" is always interpreted by vim as "a replacement with nothing", no matter what the previous replacement was ;) (EXCEPT for the ":s" case, which works differently).

So, for example, if you do a ":s/this/that/", and then do a ":s/" or a ":s//", ":s///g", etc... it will always delete occurrences of "this".

I'm glad you brought this up, because it seems I missed this case in the tests :-! I'll definitely add it :).

• add tests for empty replacement string deleting, regardless of previous state

[captaincaius]

Hi @rebornix

I added a new test to check for all the variations of empty strings. Please let me know if reading the new test resolves this item.

With that in mind, I do understand that this totally isn't the easiest code to read.

Now that all the different sequences of events are pretty well covered by tests, if you have any other ideas you'd like me to try in line with this, I'll be happy to try them, and we can be sure it'll have the same behavior :).

[captaincaius]

@rebornix thanks so much for taking the time to review the PR and suggest such detailed improvements! I'm on mobile but your suggestions look to me like they would work awesomely and mimic vim's behavior just as well! I look forward to trying them out as soon as I can and I'll check all the goofy corner cases against them and report back 😃.

I'll also rewrite the tests based on Shawn's input so the results of this will be clearer either way.

[Chillee]

Is this ready for merging now?

[captaincaius]

@Chillee I just updated to master again, so just waiting for approvals :)