Implemented EasyMotion plugin functionality #993

AlexanderArvidsson · 2016-10-26T19:42:51Z

Yay! We love PRs! 🎊

Please include a description of your change and ensure:

Commit message has a short title & issue references
Each commit does a logical chunk of work.
It builds and tests pass (e.g gulp)

More info can be found on our contribution guide.

This implements the common motions found in the EasyMotion vim plugin, which is s, w, e, b. More to come later.
Now also supports the motions t, T, f, F.

johnfn · 2016-10-27T19:34:20Z

src/actions/actions.ts

+
+
+// Only register the EasyMotion actions if the configuration is set
+if (Configuration.getInstance().easymotion) {


Instead of this can you check for the configuration inside the exec() function that switches to the EasyMotion mode?

johnfn · 2016-10-27T19:58:07Z

src/actions/actions.ts

+      return match.position;
+    }
+
+    public processMarkers(matches: EasyMotion.Match[], position: Position, vimState: VimState) {


Can you put the call to clearMarkers() inside this function?

Also, can you rename this function to recreateAllMarkers? Or something that indicates a little better what's going on?

I actually think this entire function might want to be located on the easyMotion object on VimState instead. It seems more associated with that object than BaseEasyMotionCommand.

You're right about it belonging in EasyMotion object, but then I would have to pass in the getMatchPosition as a callback in order to map the matches differently on each action (e, T, t have different match mappings). Do you think this should be preferred?

johnfn · 2016-10-27T19:59:35Z

src/actions/actions.ts

+      this.processMarkers(matches, position, vimState);
+
+      // Let EasyMotion update all decorations
+      vimState.easyMotion.updateDecorations(position);


This should be called from our updateView function in ModeHandler.

(That's where we do all our view changing stuff.)

johnfn · 2016-10-27T20:02:06Z

src/actions/actions.ts

+
+// Only register the EasyMotion actions if the configuration is set
+if (Configuration.getInstance().easymotion) {
+  class BaseEasyMotionCommand extends BaseCommand {


Could this class be made abstract?

johnfn · 2016-10-27T20:03:51Z

src/easymotion/easymotion.ts

+  /**
+   * Search and sort using the index of a match compared to the index of position (usually the cursor)
+   */
+  public sortedSearch(position: Position, searchString = "", options: EasyMotion.SearchOptions = {}): EasyMotion.Match[] {


Instead of passing in isRegex: true, simply make the type of searchString string | RegExp. Then you can do if (typeof searchString === "string") and TypeScript is intelligent enough to infer that within the curly braces of that if, searchString is now a string (similarly with the RegExp).

Good idea, I was following the StateSearch method. This suggestion applies to StateSearch as well!

johnfn · 2016-10-27T20:04:57Z

src/actions/actions.ts

+  }
+
+  @RegisterAction
+  class CommandEscEasyMotionMode extends BaseCommand {


Can you consolidate this with our normal CommandEsc stuff? Just be like if (vimState.currentMode === ModeName.EasyMotionMode) and then do the custom handling you have here.

I actually disagree

Originally I was going to make the same comment, however I then thought this was more modular if/when he moves it to its own file

Why do we have a Replace vs VisualReplace sort of thing, behavior is different so its a different action

johnfn · 2016-10-27T20:05:35Z

src/actions/actions.ts

+        return position;
+      }
+
+      // "nail" refers to the accumulated depth keys


Why is it called nail? :P

I honestly have no idea, I couldn't find a decent word to describe what it does, which is why I am referring to it as "accumulated depth keys". Got a better name? :)

johnfn · 2016-10-27T20:06:57Z

src/easymotion/easymotion.ts

+  public exitMode() {
+    this._vimState.currentMode = ModeName.Normal;
+
+    this.accumulation = "";


Instead of doing this (which requires you to remember to add new properties here as well), you can just do something like vimState.easyMotionObject = new EasyMotion()

Vimstate already has vimstate.easymotion = new EasyMotion(vimstate)

Recreating the whole object instead of individual variables just puts unnecessary load on the garbage collector. Any particular reason you want it this way instead?

Yes. Until we have actual profiling data that show that memory consumption or gc cycles are issues, we shouldn't worry at all about performance. Code quality is much more important.

My concern with the way we're doing here is that every time you add another property to the EasyMotion object, you also have to remember to clear it out in exitMode(). This is an easy way to end up adding bugs when someone comes back to modify this class months down the line.

I follow this pattern in ModeHandler with RecordedState - look for the multiple instances of new RecordedState() - because I got bitten by similar bugs. :)

johnfn · 2016-10-27T20:12:11Z

Pretty amazing stuff. It's awesome that you did this!

BEST VIM PLUGIN EVER!!!

…ymotion

kidproquo · 2016-10-28T14:57:25Z

Awesome! This is a really cool plugin. Thanks for doing this!

johnfn · 2016-10-28T21:56:04Z

Also, I don't have any problem with defaulting easymotion to on. :)

johnfn · 2016-11-01T22:18:58Z

Thanks for the amazing work, @Metamist!

hastebrot · 2016-11-20T02:46:57Z

src/easymotion/easymotion.ts

+      var uri = vscode.Uri.parse(
+        `data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 ${width} ` +
+        `13" height="14" width="${width}"><rect width="${width}" height="14" rx="2" ry="2" ` +
+        `style="fill: ${backgroundColor};"></rect><text font-family="Consolas" font-size="14px" ` +


This shows a serif font in the editor in EasyMotion mode on Ubuntu.

Consolas font isn't installed on my macbook either. Why not just lookup the font-family being used in the user-settings?

@dsifford, Good point, this was the first time I had ever touched anything regarding extensions for VSCode, so I didn't know I could look it up. If someone who has more experience regarding this can fix it that would be great!

@Metamist Not even totally sure my suggestion is possible. I'm a "converted-atom-user-in-progress" so I have very little experience on the ins and outs of VSCode for now.

That said, an even easier solution for now would be to just give a bunch of fallback font options ending with monospace.

So, for example, font-family: Consolas, Monaco, 'Courier New', monospace; might work in a wider range of machines.

casprwang · 2016-11-21T06:47:35Z

Do we have a shortcut for this motion? I've tried 'ctrl+alt+\', but It doesn't work.

hastebrot · 2016-11-21T07:23:45Z

@wangsongiam \ \ s (backslash then backslash then s-key) and then the character to search, worked for me.

settings.json:

{
    "vim.easymotion": true
}

AlexanderArvidsson · 2016-11-21T07:30:50Z

@wangsongiam, you can use most basic motions with EasyMotion, such as s, e, w, b, ge, f, F, t, F, with s being the most useful one IMO. All these motions are prefixed with \\.

casprwang · 2016-11-21T08:10:21Z

@Metamist worked, thanks! but can I change the prefix \\ to something else? Also, It would be helpful if we make other link hint styles(as black and red are not very recognizable for some light themes), or we can configure the hint style in css like vimium.

Metamist added 18 commits October 26, 2016 21:14

Initial EasyMotion functionality

0ef4049

Fixed EasyMotion mode not added to list of modes

2d3bf33

Made TSLint happy

20d1cc4

Fixed some code indentation mistakes

16a1c74

Added check if no matches are found

ffa8c75

Added comments

f40e3eb

Made TSLint happy

474d971

Implemented f, F, t, T motions

be172e7

Changed up the comments

d3c4440

Increased max matches to 1000

4c22a4a

Fixed searching for space should only match first

67e1a28

Made TSLint happy

2d995f1

Abstracted out EasyMotion actions

0380881

Removed debug log

ac0d55a

Small optimizations when finding markers

a806533

EasyMotion.Match now contains the matched text

9739bdc

Implemented ge motion

3a032dd

Merge branch 'master' into easymotion

365ddf4

johnfn requested changes Oct 27, 2016

View reviewed changes

Metamist added 5 commits October 27, 2016 22:50

Moved escape functionality to CommandEsc

120e70e

Added status bar text to show current depth

ca34a30

Moved EasyMotion updateDecorations to updateView

d42001f

Merge branch 'easymotion' of https://github.com/Metamist/Vim into eas…

231358b

…ymotion

Remove isRegex and use string | RegExp instead

d911ec0

EasyMotion object is now recreated when reset

d261014

johnfn approved these changes Nov 1, 2016

View reviewed changes

johnfn merged commit 19deb1e into VSCodeVim:master Nov 1, 2016

johnfn mentioned this pull request Nov 1, 2016

Can Visual Studio Code use Vim Plugins? #1025

Closed

hastebrot reviewed Nov 20, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented EasyMotion plugin functionality #993

Implemented EasyMotion plugin functionality #993

AlexanderArvidsson commented Oct 26, 2016 •

edited

Loading

johnfn Oct 27, 2016

johnfn Oct 27, 2016

AlexanderArvidsson Oct 27, 2016 •

edited

Loading

johnfn Oct 27, 2016

johnfn Oct 27, 2016

johnfn Oct 27, 2016

johnfn Oct 27, 2016

AlexanderArvidsson Oct 27, 2016

johnfn Oct 27, 2016

xconverge Oct 27, 2016

johnfn Oct 27, 2016

AlexanderArvidsson Oct 27, 2016

johnfn Oct 27, 2016

AlexanderArvidsson Oct 27, 2016

AlexanderArvidsson Oct 28, 2016

johnfn Oct 28, 2016

johnfn commented Oct 27, 2016

kidproquo commented Oct 28, 2016

johnfn commented Oct 28, 2016

johnfn commented Nov 1, 2016

hastebrot Nov 20, 2016

dsifford Jan 22, 2017

AlexanderArvidsson Jan 23, 2017

dsifford Jan 23, 2017 •

edited

Loading

casprwang commented Nov 21, 2016 •

edited

Loading

hastebrot commented Nov 21, 2016 •

edited

Loading

AlexanderArvidsson commented Nov 21, 2016 •

edited

Loading

casprwang commented Nov 21, 2016 •

edited

Loading



		// Only register the EasyMotion actions if the configuration is set
		if (Configuration.getInstance().easymotion) {

Implemented EasyMotion plugin functionality #993

Implemented EasyMotion plugin functionality #993

Conversation

AlexanderArvidsson commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderArvidsson Oct 27, 2016 • 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

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

johnfn commented Oct 27, 2016

kidproquo commented Oct 28, 2016

johnfn commented Oct 28, 2016

johnfn commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsifford Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

casprwang commented Nov 21, 2016 • edited Loading

hastebrot commented Nov 21, 2016 • edited Loading

AlexanderArvidsson commented Nov 21, 2016 • edited Loading

casprwang commented Nov 21, 2016 • edited Loading

AlexanderArvidsson commented Oct 26, 2016 •

edited

Loading

AlexanderArvidsson Oct 27, 2016 •

edited

Loading

dsifford Jan 23, 2017 •

edited

Loading

casprwang commented Nov 21, 2016 •

edited

Loading

hastebrot commented Nov 21, 2016 •

edited

Loading

AlexanderArvidsson commented Nov 21, 2016 •

edited

Loading

casprwang commented Nov 21, 2016 •

edited

Loading