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

Support exact and inexact current word search #1277

Merged
merged 3 commits into from
Feb 7, 2017

Conversation

rhys-vdw
Copy link
Contributor

@rhys-vdw rhys-vdw commented Feb 6, 2017

Rename CommandStar and CommandHash to CommandSearchCurrentWordExactForward and CommandSearchCurrentWorldExactBackward, which describe their effect. Also include regex word bounds in the search so that they act the same as in Vim.

Add CommandSearchCurrentWordForward and …Backward (commands g* and g# respectively), that match without word bounds.

Closes #1266


So this is mainly a copy+paste, I wasn't sure if there was an idiomatic approach to sharing behaviour between these two commands. Also I renamed the classes because it felt more sensible to name them after their effect than their keybindings (also it seemed strange to call the new one something like CommandGStar.

Let me know if there's anything stylistically or functionally wrong here, I'm happy to iterate on it.

Copy link
Member

@johnfn johnfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the bit of duplication, this looks good to me. Thanks so much for the PR! :)

@@ -1583,6 +1583,34 @@ class CommandStar extends BaseCommand {
return vimState;
}

vimState.globalState.searchState = new SearchState(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we share methods with static methods on one of the relevant classes.

@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Feb 6, 2017

Seems I need to update some tests... I started looking into this and found this test:

  newTest({
    title: "Can handle tricky *",
    start: ['|blah blahblah duh blah'],
    keysPressed: '*',
    end: ['blah blahblah duh |blah']
  });

Which superficially implies that the change I've made was already implemented? However, on further investigation this is actually behaving incorrectly. Either * or # will immediately jump to the final blah, but then pressing n will cycle through all blahs (including the two in blahblah). So the initial jump is incorrect.

Vim
vim

VSCodeVim
Note how it jumps to the last occurrence on either * or #. This is master (not my branch). I haven't fixed this specific problem.
vcodevim

I think I should review these few test cases and make sure they can't trigger false positives (and add more coverage for these commands). I need to fix other broken tests as well so I can look into this at the same time (or open a separate issue).

@johnfn
Copy link
Member

johnfn commented Feb 6, 2017

Yeah - this behavior actually works correctly on my machine. So I feel like we're dealing with some config oddities rather than actual bugs. (I do remember covering this case many many months ago...)

I'm okay with your solution, which feels nice, but I would like to know what exactly went wrong...

@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Feb 6, 2017

Hm. Yeah, that is weird. You did mention that and I had assumed at the time that you had misunderstood my report (since the code itself didn't seem to handle the correct behaviour). Even if I completely clear my config this still behaves as reported in #1266, so perhaps you have some configuration that is causing it to do the right thing?

Could you confirm whether you get the incorrect initial move with * and # that I show in the second gif above?

@johnfn
Copy link
Member

johnfn commented Feb 6, 2017

Oh yeah good point. My Vim related settings are:

    "vim.useCtrlKeys": true,
    "vim.useSolidBlockCursor": false,
    "vim.timeout": 1000,
    "vim.easymotion": true,
    "vim.showcmd": true,
    "vim.otherModesKeyBindings": [ ],

@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Feb 6, 2017

Using your configuration made no difference.

Rename `CommandStar` and `CommandHash` to `CommandSearchCurrentWordExactForward` and `CommandSearchCurrentWorldExactBackward`, which describe their effect. Also include regex word bounds in the search so that they act the same as in Vim.

Add `CommandSearchCurrentWordForward` and `…Backward` (commands `g*` and `g#` respectively), that match without word bounds.

Closes VSCodeVim#1266
Add tests for `g*` and `g#` by themselves and in combination with `n` (to step to next command).

Previously the `#` and `*` commands (and, by virtue of having been copied, the `g#` and `g*` commands introduced in a previous commit) would immediately search and filter any matches that were not full word matches.

ie. they would skip matches "hello" when searching for "he".

This would work fine for `*` or `#` directly, but subsequent uses of `n` or `N` would match incorrectly since the word bounds were not included in the search state.
Combine implementation of all four search commands (`*`, `#`, `g*`, `g#`) into a single function taking a `direction` and `isExact` argument.
@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Feb 7, 2017

@johnfn Okay, I've added tests, fixed all the problems I identified, and removed the duplication between commands. There's more information in the commit messages.

Locally all tests pass. The failures here seem to also be present in master.

@johnfn
Copy link
Member

johnfn commented Feb 7, 2017

Great! I have been noticing weirdness with the tests as well - sorry if that was confusing.

Thanks again for the PR.

@johnfn johnfn merged commit 3eada0f into VSCodeVim:master Feb 7, 2017
@rhys-vdw rhys-vdw deleted the exact-search branch June 4, 2023 13:01
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants