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

Adopt latest list navigation support #3432

Merged
merged 5 commits into from Feb 1, 2019

Conversation

Projects
None yet
9 participants
@joaomoreno
Copy link
Contributor

joaomoreno commented Jan 31, 2019

What this PR does / why we need it:

This PR introduces a new keybinding when in the list context (/) which invokes the list.toggleKeyboardNavigation in order to use the new upcoming keyboard navigation feature in VS Code 1.31.

The introduction is backwards compatible since the keybinding is only enabled if listSupportsKeyboardNavigation is true which is the case only from 1.31 onwards.

Special notes for your reviewer:

Related VS Code issue: Microsoft/vscode#66995

This should be merged before VS Code 1.31 releases.

@@ -360,6 +360,8 @@ export async function activate(context: vscode.ExtensionContext) {
}

await Promise.all([
// This is in order to disable automatic keyboard navigation in lists
vscode.commands.executeCommand('setContext', 'listAutomaticKeyboardNavigation', false),

This comment has been minimized.

@jpoon

jpoon Jan 31, 2019

Member

We ended up wrapping this as we've found that setContext sometimes takes awhile to complete. It doesn't make much difference in this context, but to follow with the pattern, can you call:

  await VsCodeContext.Set('listAutomaticKeyboardNavigation', false);
@@ -718,5 +723,10 @@
"tslint": "5.12.1",
"typescript": "3.2.4",
"vscode": "1.1.28"
},
"__metadata": {

This comment has been minimized.

@jpoon

jpoon Jan 31, 2019

Member

Ohh, secret fields -- what is this?

This comment has been minimized.

@joaomoreno

joaomoreno Feb 1, 2019

Author Contributor

D'oh! Can't believe I let this slip. 🤦‍♂️ Just some gallery metadata.

@jpoon
Copy link
Member

jpoon left a comment

Yikes, you'll likely have conflicts -- I just moved some stuff around earlier today.

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 1, 2019

No worries, I'll push new commits and resolve asap.

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 1, 2019

@jpoon Alright, merged and cleaned up! 👍

@TravisBuddy

This comment has been minimized.

Copy link

TravisBuddy commented Feb 1, 2019

Travis tests have failed

Hey @joaomoreno,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

Node.js: 8

View build log

if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
diff --git a/extension.ts b/extension.ts
index f237e13..ef7857b 100644
--- a/extension.ts
+++ b/extension.ts
@@ -116,14 +116,14 @@ export async function activate(context: vscode.ExtensionContext) {
       changeEvent.contentChanges.length === 1 &&
       changeEvent.contentChanges[0].text === '' &&
       changeEvent.contentChanges[0].range.start.line !==
-      changeEvent.contentChanges[0].range.end.line;
+        changeEvent.contentChanges[0].range.end.line;
 
     const textWasAdded = changeEvent =>
       changeEvent.contentChanges.length === 1 &&
       (changeEvent.contentChanges[0].text === '\n' ||
         changeEvent.contentChanges[0].text === '\r\n') &&
       changeEvent.contentChanges[0].range.start.line ===
-      changeEvent.contentChanges[0].range.end.line;
+        changeEvent.contentChanges[0].range.end.line;
 
     if (textWasDeleted(event)) {
       globalState.jumpTracker.handleTextDeleted(event.document, event.contentChanges[0].range);
diff --git a/test/configuration/configuration.test.ts b/test/configuration/configuration.test.ts
index 0bdf8e3..f91670c 100644
--- a/test/configuration/configuration.test.ts
+++ b/test/configuration/configuration.test.ts
@@ -78,7 +78,7 @@ suite('Configuration', () => {
   });
 
   test('neovim disabled on missing path', async () => {
-    assert.equal(false, srcConfiguration.configuration.enableNeovim)
+    assert.equal(false, srcConfiguration.configuration.enableNeovim);
   });
 
   newTest({
Prettier Failed. Run [08:36:50] Using gulpfile ~/build/VSCodeVim/Vim/gulpfile.js
[08:36:50] Starting 'forceprettier'...
[08:37:02] Finished 'forceprettier' after 13 s and commit changes to resolve.
TravisBuddy Request Identifier: 8cb8ac10-25fc-11e9-a660-67a7bedcbb29
@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 1, 2019

Ahah I even created a commit which left those bad whitespaces in simply because I didn't want more changes in the PR. But sure, bot, I can leave them in!

@@ -78,7 +78,7 @@ suite('Configuration', () => {
});

test('neovim disabled on missing path', async () => {
assert.equal(false, srcConfiguration.configuration.enableNeovim)
assert.equal(false, srcConfiguration.configuration.enableNeovim);

This comment has been minimized.

@jpoon

jpoon Feb 1, 2019

Member

Yikes, thanks for fixing this.

@jpoon

jpoon approved these changes Feb 1, 2019

@jpoon jpoon merged commit 22b826c into VSCodeVim:master Feb 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joaomoreno joaomoreno deleted the joaomoreno:adopt-list-navigation branch Feb 1, 2019

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 1, 2019

Thanks @jpoon! 🙏

@bcpugh

This comment has been minimized.

Copy link

bcpugh commented Feb 6, 2019

thanks guys! slightly off-topic:
any advice on setting listSupportsKeyboardNavigation = false without implementing VSCodeContext.set() inside of an extension (i.e. a settings.json entry?)

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 7, 2019

@bcpugh Do you have single-letter keybindings for lists regardless of using VIM?

@FelikZ

This comment has been minimized.

Copy link

FelikZ commented Feb 7, 2019

So when I am focused on (for example) file explorer, vim mode enabled and hit / then it should enter a search mode?

I did not get it how it works.

Because I have updated VSCode and vim plugin and whenever I hit / I got the old behaviour (which is good).

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 7, 2019

What was the old behaviour?

@FelikZ

This comment has been minimized.

Copy link

FelikZ commented Feb 7, 2019

@joaomoreno
Old:

  • Focus on editor, hit / - toggles vim search
  • Focus on file explorer, hit / - nothing expected to happen
    New:
  • Focus on editor, same behaviour
  • Focus on file explorer, hit / - filter/highlight / char (new feature)

So my question is, what this PR actually did, since in file editor it is the same behaviour as before and in file explorer (other lists) vim is not active there (was it?).

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 7, 2019

VIM sets hjkl as keybindings to navigate in file explorer.

@FelikZ

This comment has been minimized.

Copy link

FelikZ commented Feb 7, 2019

Thank you, it is clear now! Awesome job!

@bcpugh

This comment has been minimized.

Copy link

bcpugh commented Feb 7, 2019

@joaomoreno

We lost functionality for key binding of “a” to create new file when the file tree is focused (and other similar bindings). Only some of our team uses VIM, but I wanted to ask here since the latest VScode release notes pointed us here. Can we somehow revert back to the old behavior without installing an extension like VIM?

@joaomoreno

This comment has been minimized.

Copy link
Contributor Author

joaomoreno commented Feb 7, 2019

@bcpugh My recommendation would be to simply switch to a keybinding with modifier key, eg Ctrl N.

@asilvadesigns

This comment has been minimized.

Copy link

asilvadesigns commented Feb 7, 2019

I'm very excited about this feature, awesome work, but it does not work for me.
With VIM enabled, and workbench.list.keyboardNavigation = "simple", focusing on the tree and typing "/" does not enter highlight or filter mode. What am I missing, I'm not sure to enable this?

@nomad-software

This comment has been minimized.

Copy link

nomad-software commented Feb 7, 2019

How can I disable this feature added by this pull request?

@macksta

This comment has been minimized.

Copy link

macksta commented Feb 11, 2019

This is an epic feature!! Good work!!

@Molunerfinn

This comment has been minimized.

Copy link

Molunerfinn commented Feb 14, 2019

@asilvadesigns Hi~ I have found how to fix your problem!
You just need to set the
"workbench.list.keyboardNavigation": "filter" or
"workbench.list.keyboardNavigation": "highlight"
Since list.toggleKeyboardNavigation is to toggle navigation mode to filter or highlight. While you just set to simple, so the navigation mode is just simple after toggling(whether it is on or off).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment