Visualstar #1616

mikew · 2017-04-30T20:33:43Z

The default behaviour of * and # isn't really intuitive, and Vim itself has many plugins to handle this.

This PR adds a new option to treat * and # as a new search when pressed in visual mode.

This reverts commit 8ad56a4.

Chillee · 2017-05-01T02:29:58Z

package.json

+        "vim.visualstar": {
+          "type": "boolean",
+          "description": "When pressing * or # in visual mode treat it as a new search",
+          "default": true


Should this be "default": false?

Good catch, that may be why the tests were inexplicably passing ...

I'm fine with toggling bonus features like this on, actually.

I think the difference is that this one affects default behavior.

johnfn

Thank you so much for the PR!! It's really appreciated.

This looks great to me. Just a few stylistic comments and we should be good to go here. :)

johnfn · 2017-05-01T03:10:40Z

README.md

@@ -180,6 +180,10 @@ These settings are specific to VSCodeVim.
    }
 ```

+#### `"vim.visualstar"`
+* When pressing `*` or `#` in visual mode treat it as a new search


Could we rephrase this slightly:

"In visual mode, start a search with * or # using the current selection."

Gladly, I was never sure what the wording should be.

johnfn · 2017-05-01T03:12:15Z

src/actions/actions.ts

@@ -1739,6 +1781,10 @@ class CommandSearchCurrentWordExactForward extends BaseCommand {
  runsOnceForEachCountPrefix = true;

  public async exec(position: Position, vimState: VimState): Promise<VimState> {
+    if (Configuration.visualstar && vimState.currentMode !== ModeName.Normal) {


Instead of a custom dispatch on current mode, can we have two separate commands, one for ModeName.Normal, and one for ModeName.Visual/Line?

Ha, that's how I had things originally.

johnfn · 2017-05-01T03:13:09Z

src/actions/actions.ts

+    });
+}
+
+interface IPerformSearchMovementArgs {


I'm fine with inlining this in performSearchMovement arguments.

(When you extract it to an interface, I subconsciously think that we're using it elsewhere in the code.)

Yeah I asked about this on slack, because the inlined arguments made the linter complain about the line length. So the question was numerous arguments over a multiple lines versus an interface.

Oh I see. If you indent every line of the object you should be good right?

johnfn · 2017-05-01T03:13:21Z

src/actions/actions.ts

+}
+
+interface IPerformSearchMovementArgs {
+    needle: string | undefined;


Better written as needle?: string

johnfn · 2017-05-01T03:14:04Z

src/actions/actions.ts

+    searchStartCursorPosition: Position;
+}
+
+function performSearchMovement(args: IPerformSearchMovementArgs) {


Suggestion to destructure args here e.g.

const { needle, isExact, vimState } = args

Removes a lot of args. throughout this function and should improve readability ever so slightly.

I'll just go back to inlining them

johnfn · 2017-05-01T03:14:36Z

src/actions/actions.ts

+}
+
+function performSearchMovement(args: IPerformSearchMovementArgs) {
+    if (args.needle === undefined || args.needle === null || args.needle.length === 0) {


No need to check against null because you said args is string | undefined

johnfn · 2017-05-01T03:16:03Z

src/actions/actions.ts

+    searchStartCursorPosition: Position;
+}
+
+function performSearchMovement(args: IPerformSearchMovementArgs) {


Can you give this function a better name? Possibly moveCursorToNextSearchMatch.

I'm fine with making the name more clear if you think it's ambiguous, but it's really doing two things: performing the search and making the movement.

It's just that "performing the search" feels a bit general to me. You mean we create the current search state right?

Maybe createSearchStateAndMoveToMatch?

…ateSearchStateAndMoveToMatch

…wn commands

mikew · 2017-05-01T03:52:17Z

All those changes are in, let me know what you think

johnfn · 2017-05-01T03:53:40Z

src/actions/actions.ts

+    return createSearchStateAndMoveToMatch(currentSelection, vimState, direction, false, searchStartCursorPosition);
+}
+
+function createSearchStateAndMoveToMatch(


Ohh wait a sec. I mean like this:

function createSearchStateAndMoveToMatch(args: { needle: string | undefined, vimState: VimState, direction: SearchDirection, isExact: boolean, searchStartCursorPosition: Position, }

IMO it's important to use an object here so we associate names with the arguments. Otherwise when we pass in the 5 different args to the function it's hard to remember what each one is.

Ohhhhh, I didn't know that was possible in typescript.

Yeah! :) Fun fact, if you can specify a good default for each, you can even auto-destructure the arguments:

function doStuff({ a = 1, b = "foo" }) { /* go on to use a and b */ }

(That's not possible here - you can't really get a default VimState - so don't worry about it)

I knew object destructuring defaults from ES6, wasn't sure how to use them in typescript with type information as well.

johnfn · 2017-05-01T03:54:19Z

One last thing - sorry about that, I could have been more clear! Then we should be good to go. :)

mikew · 2017-05-01T04:04:33Z

Heck of a lot of work for a long line.

And in the end, a line was added that's just on the cusp of the limit.

johnfn · 2017-05-01T04:08:26Z

To be fair, it's less about the length of the line, and more about trying to move this codebase in the direction of explicitly naming all arguments when argument length gets past a certain point. I find foo(bar, baz, undefined, true, 5) to be a lot less readable than (say) foo({ name: bar, type: baz, deleteLine: true }).

Thank you for making those changes, and thank you again for the great PR. I (and all VSCodeVim users!) will really appreciate it. :)

mikew · 2017-05-01T04:13:11Z

I'm with you on the number of arguments, any more than 2 or 3 really.

mikew added 10 commits April 30, 2017 15:32

WIP: Add visualstar support

9b3a71f

Fix start positions in visualstar

6ee1828

Fix modeVisual.test.ts

ab872a9

Make note of vim.visualstar in README.md

95af26d

Fix comments in searchCurrentSelection

0f0b278

Explain what's being tested in the visualstar tests

50059d4

Refactor performSearchMovement args into IPerformSearchMovementArgs

b612b05

Fix lint issues

8ad56a4

Revert "Fix lint issues"

966fa95

This reverts commit 8ad56a4.

Fix lint issues

e0c51b3

Chillee reviewed May 1, 2017

View reviewed changes

Fix default value for vim.visualstar

35c3c60

johnfn requested changes May 1, 2017

View reviewed changes

mikew added 3 commits May 1, 2017 00:28

Change wording of visualstar description

212ebc0

Ditch IPerformSearchMovementArgs, rename performSearchMovement to cre…

412433b

…ateSearchStateAndMoveToMatch

Make CommandSearchVisualForward / CommandSearchVisualBackward their o…

c7cf426

…wn commands

johnfn reviewed May 1, 2017

View reviewed changes

Make createSearchStateAndMoveToMatch take a single object as arguments

94e6f0b

johnfn merged commit a670b28 into VSCodeVim:master May 1, 2017

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visualstar #1616

Visualstar #1616

mikew commented Apr 30, 2017

Chillee May 1, 2017 •

edited

Loading

mikew May 1, 2017

johnfn May 1, 2017

Chillee May 1, 2017

johnfn left a comment

johnfn May 1, 2017

mikew May 1, 2017

johnfn May 1, 2017

mikew May 1, 2017

johnfn May 1, 2017

mikew May 1, 2017 •

edited

Loading

johnfn May 1, 2017

johnfn May 1, 2017

johnfn May 1, 2017

mikew May 1, 2017

johnfn May 1, 2017

johnfn May 1, 2017

mikew May 1, 2017

johnfn May 1, 2017

mikew commented May 1, 2017

johnfn May 1, 2017

mikew May 1, 2017

johnfn May 1, 2017 •

edited

Loading

johnfn May 1, 2017

mikew May 1, 2017

johnfn commented May 1, 2017

mikew commented May 1, 2017

johnfn commented May 1, 2017

mikew commented May 1, 2017

Visualstar #1616

Visualstar #1616

Conversation

mikew commented Apr 30, 2017

Chillee May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikew May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikew commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfn May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfn commented May 1, 2017

mikew commented May 1, 2017

johnfn commented May 1, 2017

mikew commented May 1, 2017

Chillee May 1, 2017 •

edited

Loading

mikew May 1, 2017 •

edited

Loading

johnfn May 1, 2017 •

edited

Loading