Fixes #2113: Start in Disabled mode configuration. #2115

westim · 2017-11-03T23:38:55Z

This PR is to add the configuration to package.json to start Vim in the Disabled mode by calling the toggleVim command.

This configuration is false by default, and therefore has no effect on default configuration behavior of VSCodeVim.

jpoon

Thanks for the PR. I have a couple of suggestions.

We should extend the functionality to have a disableExtension configuration rather than a start-up flag.
In doing so, we can get rid of the Globals.active
toggleVim would then update this configuration

This would greatly simplify the state of whether the extension is disabled/enabled as right now that information lives across 2 or 3 different variables.

westim · 2017-11-06T00:04:52Z

Thanks for the great feedback, @jpoon !

jpoon · 2017-11-06T04:20:26Z

extension.ts


-  vscode.commands.executeCommand('setContext', 'vim.active', Globals.active);
+  registerCommand(context, 'toggleVim', async () => {
+    Configuration.disableExtension = !Configuration.disableExtension;


I think this change is only in-memory and doesn't actually change the settings to the configuration file. I don't think we ever call the update fx in the api docs.

It seems like we should convert Configuration class to use getters/setters such that if you set any of the configurations, it'll call the aforementioned update api.

For now, for the sake of limiting the scope of these changes, I've only implemented a getter/setter for the Configuration.disableExtension property.

jpoon · 2017-11-06T04:20:55Z

README.md

+#### `"vim.disableExtension"`
+* VSCodeVim will be in "Disabled" mode
+* This can be changed at any time using the `toggleVim` command in the Command Palette
+* Note that this is not the same as disabling the VSCodeVim extension


Nit: probably best to clarify

...disabling the VSCodeVim extension through VS Code.

jpoon · 2017-11-06T04:22:08Z

extension.ts

          default:
            break;
        }
        editor.options = options;
      });
-      await vscode.commands.executeCommand('setContext', 'vim.active', Globals.active);
+      await vscode.commands.executeCommand('setContext', 'vim.active', false);


Do we need to call this in two different spots (line 335, line 339)? You can probably consolidate it to outside the if to use Configuration.disableExtension

await vscode.commands.executeCommand('setContext', 'vim.active', Configuration.disableExtension)

jpoon · 2017-11-06T04:24:42Z

extension.ts

-      let mh = await getAndUpdateModeHandler();
-      mh.updateView(mh.vimState, { drawSelection: false, revealRange: false });
-    } else {
+  async function toggleDisabledMode() {


Hmm, I'm trying to think of a better name as it's not actually toggling the mode (ie. if you call this twice, it doesn't actually toggle and go from disable->enable).

Maybe this signature:

toggleExtension(bool isDisabled)

jpoon · 2017-11-09T15:48:51Z

@westim let me know when this is ready for review.

westim · 2017-11-09T16:07:56Z

Hi @jpoon ,
I believe all 4 comments from the second review were addressed in f68b16f. This currently ready for review.

jpoon

Can you write a test for this? Otherwise, I presume you've tested it and ensured that the save persists across VS Code instances?

jpoon · 2017-11-09T18:06:46Z

src/configuration/configuration.ts

+  /**
+   * Determines whether VSCodeVim is in Disabled mode or not.
+   */
+  private _disableExtension: boolean | undefined;


Does it make sense to set this to false to start? Is there a scenario where it'll be undefined?

I have some weird behavior I need to work out, and this will be involved in some further changes I need to make. I'll let you know when it's ready. Sorry about all the confusion / edits!

In order to properly access and overwrite the extension configuration values, the name of the private field needed to match the value found in the configuration file, while the getter & setter were given a different name. Also, to properly use the vscode.WorkspaceConfiguration.update() API, ConfigurationTarget.Global had to be specified. The value true could've also been used, but the enum is more explicit.

westim · 2017-11-12T17:34:20Z

Hello @jpoon,

Another update on this. I was able to get the configuration to change correctly after fixing two issues:

The private backing field needed to match the name in config, not the getter/setter.
The vscode.WorkspaceConfiguration.update() API needed a ConfigurationTarget argument. When that argument was undefined, the update() method actually attempts to remove the section from config.

I'm not sure exactly how to test this. I confirmed the configuration value was changing using primitive techniques:

console.log(vscode.workspace.getConfiguration('vim').get('disableExtension'));

I confirmed that the configuration change carries forward to new Editor Groups in the same Window. I'm not sure I can properly test whether the configuration affects new Windows while using the Extension Development Host. If there is, please let me know.

jpoon · 2017-11-12T19:04:39Z

Thanks @westim

Fixes VSCodeVim#2113: Start in Disabled mode configuration.

331c8ae

jpoon self-requested a review November 4, 2017 19:32

jpoon requested changes Nov 4, 2017

View reviewed changes

Updated disableExtension configuration, removed Globals.active.

5b71c5a

Updated README per configuration changes.

4726172

jpoon requested changes Nov 6, 2017

View reviewed changes

westim and others added 2 commits November 6, 2017 20:14

Disabled mode tweaks, including modification of config value.

f68b16f

Merge branch 'master' into start-disabled-config

41203c6

Merge branch 'master' into start-disabled-config

70a57c4

jpoon approved these changes Nov 9, 2017

View reviewed changes

jpoon and others added 2 commits November 9, 2017 23:15

Merge branch 'master' into start-disabled-config

cbf6915

Merge branch 'master' into start-disabled-config

eca226c

jpoon merged commit 9936d8d into VSCodeVim:master Nov 12, 2017

westim deleted the start-disabled-config branch November 14, 2017 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2113: Start in Disabled mode configuration. #2115

Fixes #2113: Start in Disabled mode configuration. #2115

westim commented Nov 3, 2017

jpoon left a comment •

edited

Loading

westim commented Nov 6, 2017

jpoon Nov 6, 2017

westim Nov 7, 2017

jpoon Nov 6, 2017

jpoon Nov 6, 2017

jpoon Nov 6, 2017

jpoon commented Nov 9, 2017

westim commented Nov 9, 2017

jpoon left a comment

jpoon Nov 9, 2017

westim Nov 10, 2017

westim commented Nov 12, 2017

jpoon commented Nov 12, 2017

Fixes #2113: Start in Disabled mode configuration. #2115

Fixes #2113: Start in Disabled mode configuration. #2115

Conversation

westim commented Nov 3, 2017

jpoon left a comment • edited Loading

Choose a reason for hiding this comment

westim commented Nov 6, 2017

jpoon Nov 6, 2017

Choose a reason for hiding this comment

westim Nov 7, 2017

Choose a reason for hiding this comment

jpoon Nov 6, 2017

Choose a reason for hiding this comment

jpoon Nov 6, 2017

Choose a reason for hiding this comment

jpoon Nov 6, 2017

Choose a reason for hiding this comment

jpoon commented Nov 9, 2017

westim commented Nov 9, 2017

jpoon left a comment

Choose a reason for hiding this comment

jpoon Nov 9, 2017

Choose a reason for hiding this comment

westim Nov 10, 2017

Choose a reason for hiding this comment

westim commented Nov 12, 2017

jpoon commented Nov 12, 2017

jpoon left a comment •

edited

Loading