Vim Settings #508

rebornix · 2016-07-25T07:29:10Z

Commit message has a short title & issue references
Commits are squashed
It builds and tests pass (e.g gulp tslint)

This feature is related to #433, #463, #487 and #481. The basic idea is maintaining a singleton of Vim Settings. This Vim Seting object reads configuration from User Settings, .vimrc, Code configuration then our own (VSCodeVim) default value.

Singleton Vim Settings
Load vim.{xxx} section
Map Vim configure to Code if there is any overlap
Fix w should/could respect wordSeparators settings #487
Fix :set hlsearch not working #481
.vimrc loading
Auto update (monitoring both Code's configuration and .vimrc)

johnfn · 2016-07-25T18:05:03Z

src/configuration/configuration.ts

+ * 4. VSCodeVim flavored Vim option default values
+ */
+export class Configuration {
+  private static _instance: Configuration;


Use nullable types!

private static _instance: Configuration | null;

rebornix · 2016-07-26T06:32:05Z

Currently you will see some problems as we have bugs in Code microsoft/vscode#9731.

johnfn · 2016-07-26T08:09:12Z

.vimrc loading

I have no qualms at all about you leaving this to another PR.

Auto update (monitoring both Code's configuration and .vimrc)

This would be very awesome, but low priority. Most people expect to have to restart their editor anyways.

johnfn · 2016-07-26T08:09:56Z

package.json

@@ -145,6 +145,30 @@
                "vim.scroll": {
                    "type": "number",
                    "description": "Number of lines to scroll with CTRL-U and CTRL-D commands."
+                },
+                "vim.tabstop": {


You don't know how happy it makes me to see we support all this stuff. My god, it's like we're actually making a real Vim plugin!!!

rebornix · 2016-07-27T06:04:52Z

Did a huge refactor to VimSettings. I can't imagine before that we can make this real simple with a little bit hack of Accessor Decorator.

With the new design, we only have one single Class/Object maintaining all Vim options' definition, intialization and updating. It's called Configuration.

Access `Configuration`

Use Configuration.getInstance(), it's the only way of getting an object of Configuration and yeah we make it a Singleton.

getInstance() is the only public method in Configuration.

Access a single Vim option

Get: Configuration.getInstance().{OptionName}, eg, Configuration.getInstance().tabstop.
Set: Configuration.getInstance().{OptionName} = {NewValue}, eg, Configuration.getInstance().tabstop = 4.
Bracket notation: eg, Configuration.getInstance()["tabstop"], Configuration.getInstance()["tabstop"] = 4.

Lastly, we will support .vimrc later on and the update of .vimrc should trigger Configuration to update automatically.

rebornix · 2016-07-27T15:15:24Z

Fix #279 as it's real easy to implement based on current Setting design, just 20 lines of code change.

johnfn · 2016-08-09T04:25:40Z

Hey @rebornix is this good to go? Or are you still working on it?

rebornix · 2016-08-09T06:48:00Z

I updated the code after our last sync up on Slack. Feel free to review and let me know what else you think can be improved.

johnfn · 2016-08-09T06:54:34Z

src/configuration/configuration.ts

+     */
+    let vimOptions = vscode.workspace.getConfiguration("vim");
+    /* tslint:disable:forin */
+    // Disable forin rule here as we make accessors enumerable.`


Can we use for of here as well?

I'd love to for of if possible but for of is specific to collections, which requires the object has a [Symbol.iterator] property. So I just kept using for in to avoid making it complex.

johnfn · 2016-08-09T07:00:02Z

Instead of the object being a singleton, is it possible to export an instance of the class (rather than the class itself) and then use that directly? (keeping the class private)

I'm not actually sure if this is possible, but if it is it would improve the code a little. :-)

jpoon · 2016-08-09T07:03:22Z

src/cmd_line/commands/setoptions.ts

+  async execute(): Promise<void> {
+    switch (this._arguments.operator) {
+      case SetOptionOperator.Set:
+        Configuration.getInstance()[this._arguments.name!] = true;


What does the ! at the end mean?

It's a new feature related to nullable types in Typescript 2.0.

this._arguments.name is an optional type - it's either a string or undefined. The ! converts the type to a string.

jpoon · 2016-08-09T07:04:36Z

@rebornix Sweeeet. I added a couple of minor comments but LGTM!

johnfn · 2016-08-09T07:04:50Z

src/mode/modeHandler.ts

-          this._matchRanges.push(new vscode.Range(
-            new Position(lineIdx, i),
-            new Position(lineIdx, i + search.length)
+          this.matchRanges.push(new vscode.Range(


Holy smokes, this PR includes smartcase as well? Heck yeah!

johnfn · 2016-08-09T07:08:55Z

This is great! I think we together (ok, mostly you!) have arrived at a really good design for settings that should be really pleasant to add to in the future. Fix my few minor complaints and we should be good to go here.

jpoon · 2016-08-09T07:09:03Z

src/configuration/configuration.ts

+  }
+
+  public static getInstance(): Configuration {
+    if (Configuration._instance == null) {


If somebody updates their workspace config, being a singleton, won't the instance retain the old value?

I'm ok with a restart for settings change if you are.

(We could do hot reloading as a feature later on...)

I'm ok with it so long as we document it (aka put it on the readme).

Although, I don't see a reason for this to be a singleton, is it a huge perf hit?

Hm, what's the alternative?

Nevermind, we use this A LOT in our code. The alternative I had in mind initially was to read the configs from the file every time.

johnfn · 2016-08-09T07:10:54Z

And please ping me if I don't respond to a finished PR for a day or two. It's a little hard for me to sort through the PRs to know which ones are done, and I definitely don't want to leave anything that is done hanging around. :)

rebornix · 2016-08-11T16:30:32Z

Update per comments and resolve conflicts.

jpoon · 2016-08-11T20:12:51Z

ship it. 🚢

_Edit: As per johnfn's comments. Allow me to change that to ...Ship It*

builds should pass._

johnfn · 2016-08-11T20:20:13Z

src/mode/modeHandler.ts

@@ -145,6 +146,7 @@ export class VimState {
   * by us or by a mouse action.
   */
  public whatILastSetTheSelectionTo: vscode.Selection;
+<<<<<<< b9a0b15742532ecc54e66ee04c417d82e70d29dc


Wait, there are merge conflicts here.

I start to doubt myself as it happens several times. I remember correctly yesterday that I did remove these lines by dd and save them then use git add . & git rebase --continue. After that, I even search ==== globally as I made this kind of mistakes before, but damn, it still happens.

I'll try to track this down next time I do rebasing. The only potential catch came to my mind now is I always reload the window as jsdiff is hanging, I'm not sure if this leads to the unsuccessful saving.

Be careful, you want to use git add . && git rebase --continue. Using a single & will cause a process to be backgrounded.

But yeah, it's annoying and it eventually happens to everyone. No worries :)

… sequence

moopie · 2016-11-03T14:40:59Z

Any update on the .vimrc loading feature?

johnfn reviewed Jul 25, 2016
View reviewed changes

rebornix force-pushed the VimRc branch from 5a36734 to 2039694 Compare July 26, 2016 06:15

johnfn added WIP and removed WIP labels Jul 26, 2016

johnfn reviewed Jul 26, 2016
View reviewed changes

rebornix force-pushed the VimRc branch from 288ef80 to 08783b4 Compare July 27, 2016 05:52

This was referenced Jul 27, 2016

vim g_ and vg_ does not function microsoft/vscode#9804

Closed

Fix #279. #433

Closed

johnfn added the WIP label Jul 29, 2016

rebornix mentioned this pull request Aug 2, 2016

Add support for S #546

Merged

rebornix force-pushed the VimRc branch from 6cfee09 to 55a3679 Compare August 2, 2016 05:47

johnfn reviewed Aug 9, 2016
View reviewed changes

jpoon reviewed Aug 9, 2016
View reviewed changes

johnfn reviewed Aug 9, 2016
View reviewed changes

jpoon removed the WIP label Aug 9, 2016

jpoon reviewed Aug 9, 2016
View reviewed changes

rebornix force-pushed the VimRc branch from 93ae263 to 7cc81bb Compare August 11, 2016 16:30

johnfn reviewed Aug 11, 2016
View reviewed changes

rebornix force-pushed the VimRc branch from 7cc81bb to d3d8e86 Compare August 12, 2016 05:35

rebornix added 12 commits August 12, 2016 21:28

Vim Settings

1693902

support hlsearch

f5ced4a

add supported options to contribution list and read config in correct…

d15271b

… sequence

Refactor code with enumerator

5723dcb

Remove global setter

cd8f206

Fix VSCodeVim#279

3bf9027

Update roadmap

900b5f6

Remove setters for options overlap with Code

ba0eb0a

fix typo and comments

df7574f

fix resolve conflicts

12200ce

fix tabsize, expand spaces and iskeyword overridden issues

c227152

fix test regression for indentation adjustment

8e75efe

rebornix force-pushed the VimRc branch from 94ac3e7 to 8e75efe Compare August 12, 2016 13:29

rebornix merged commit ba639d5 into VSCodeVim:master Aug 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vim Settings #508

Vim Settings #508

rebornix commented Jul 25, 2016 •

edited

Loading

johnfn Jul 25, 2016 •

edited

Loading

rebornix commented Jul 26, 2016

johnfn commented Jul 26, 2016

johnfn Jul 26, 2016

rebornix commented Jul 27, 2016

rebornix commented Jul 27, 2016

johnfn commented Aug 9, 2016

rebornix commented Aug 9, 2016

johnfn Aug 9, 2016

rebornix Aug 11, 2016

johnfn commented Aug 9, 2016 •

edited

Loading

jpoon Aug 9, 2016

johnfn Aug 9, 2016 •

edited

Loading

jpoon Aug 10, 2016

jpoon commented Aug 9, 2016

johnfn Aug 9, 2016

johnfn commented Aug 9, 2016

jpoon Aug 9, 2016 •

edited

Loading

johnfn Aug 9, 2016

johnfn Aug 9, 2016

jpoon Aug 9, 2016 •

edited

Loading

johnfn Aug 9, 2016

jpoon Aug 9, 2016 •

edited

Loading

johnfn commented Aug 9, 2016

rebornix commented Aug 11, 2016

jpoon commented Aug 11, 2016 •

edited

Loading

johnfn Aug 11, 2016

rebornix Aug 11, 2016 •

edited

Loading

johnfn Aug 12, 2016

moopie commented Nov 3, 2016

Vim Settings #508

Vim Settings #508

Conversation

rebornix commented Jul 25, 2016 • edited Loading

johnfn Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

rebornix commented Jul 26, 2016

johnfn commented Jul 26, 2016

Choose a reason for hiding this comment

rebornix commented Jul 27, 2016

Access Configuration

Access a single Vim option

rebornix commented Jul 27, 2016

johnfn commented Aug 9, 2016

rebornix commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfn commented Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

johnfn Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpoon commented Aug 9, 2016

Choose a reason for hiding this comment

johnfn commented Aug 9, 2016

jpoon Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpoon Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpoon Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

johnfn commented Aug 9, 2016

rebornix commented Aug 11, 2016

jpoon commented Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

rebornix Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moopie commented Nov 3, 2016

rebornix commented Jul 25, 2016 •

edited

Loading

johnfn Jul 25, 2016 •

edited

Loading

Access `Configuration`

johnfn commented Aug 9, 2016 •

edited

Loading

johnfn Aug 9, 2016 •

edited

Loading

jpoon Aug 9, 2016 •

edited

Loading

jpoon Aug 9, 2016 •

edited

Loading

jpoon Aug 9, 2016 •

edited

Loading

jpoon commented Aug 11, 2016 •

edited

Loading

rebornix Aug 11, 2016 •

edited

Loading