Skip to content
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

Refactor bindings and README #513

Merged
merged 20 commits into from Jun 3, 2021

Conversation

theol0403
Copy link
Member

@theol0403 theol0403 commented Jan 5, 2021

This is my work in progress for refactoring keybindings and readme. I hope you agree with the changes, and I will explain some of the keybindings at a later time. For now, I just want to show my progress, and see if you have any suggestions or feedback.

Would it be better to split this into two PRs? I put them in one because the readme needs to be changed to reflect the keybindings anyways.

Todo:

@asvetliakov
Copy link
Member

Would it be better to split this into two PRs?

Whatever will be easier for you.

For keybindings, also can suggest these ones:

  {
    "key": "ctrl+p",
    "command": "list.focusUp",
    "when": "inReferenceSearchEditor"
  },
  {
    "key": "ctrl+n",
    "command": "list.focusDown",
    "when": "inReferenceSearchEditor"
  }

These are for selecting prev/next reference in peek view

@Shatur
Copy link
Collaborator

Shatur commented Jan 6, 2021

=z in Vim shows spell suggestions. How about use it to open quick fix suggestions menu? Spell extensions in VSCode also use quick fix menu for spell suggestions.

nnoremap z= <Cmd>call VSCodeNotify('editor.action.quickFix')<CR>

I use the following command for multiply cursors instead of m:

nnoremap <C-Down> i<Cmd>call VSCodeNotify('editor.action.insertCursorBelow')<CR>
nnoremap <C-Up> i<Cmd>call VSCodeNotify('editor.action.insertCursorAbove')<CR>

This requires much less keystrokes. What do you think?

@Shatur
Copy link
Collaborator

Shatur commented Jan 6, 2021

Also how about gx to open current link? This shortcut is provided by built-in netrw plugin (:h netrw-gx)?

nnoremap gx <Cmd>call VSCodeNotify('editor.action.openLink')<CR>

@David-Else
Copy link
Contributor

David-Else commented Jan 6, 2021

@Shatur95 Regarding =z to open quickfix, it means that an integrated spellchecker could not use the correct shortcut #506

gx already opens the current link, I just double-checked in a markdown file. I am using the Linux appimage on Centos 8.3.

@Shatur
Copy link
Collaborator

Shatur commented Jan 6, 2021

@Shatur95 Regarding =z to open quickfix, it means that an integrated spellchecker could not use the correct shortcut #506

Is built-in spellchecking suggestions works?

gx already opens the current link, I just double-checked in a markdown file. I am using the Linux appimage on Centos 8.3.

Did not know that netrw handles it well :) I just have netrw disabled, so I binded this shortcut manually.

@David-Else
Copy link
Contributor

David-Else commented Jan 6, 2021

Is built-in spellchecking suggestions works?

Not really, but check out the thread... I think it could be possible in the future.

@Shatur
Copy link
Collaborator

Shatur commented Jan 6, 2021

Not really, but check out the thread... I think it could be possible in the future.

Yes, checked, thanks. I think it's okay to trigger quickfix via z= since future built-in spellchecking can be displayed in quickfix in future.

@theol0403
Copy link
Member Author

theol0403 commented Jan 16, 2021

Sorry for the silence... been (and will keep being in the near future) busy.

@Shatur
Copy link
Collaborator

Shatur commented Jan 16, 2021

@theol0403, I think it worth to merge it as is. No one like big PR's.

@theol0403
Copy link
Member Author

=z in Vim shows spell suggestions. How about use it to open quick fix suggestions menu? Spell extensions in VSCode also use quick fix menu for spell suggestions.

While I do like the idea (and we can remove it if vim spellcheck gets fixed - though I think vscode spellcheck is fine), it's not very useful because you can't navigate the quickfix items with the keyboard. I think I will instead provide instructions to use the keyboard quickfix plugin, until #469 gets addressed.

Sorry for the long delay everyone, hopefully I can get this into a mergeable state sometime soon.

@Shatur
Copy link
Collaborator

Shatur commented Apr 2, 2021

While I do like the idea (and we can remove it if vim spellcheck gets fixed - though I think vscode spellcheck is fine), it's not very useful because you can't navigate the quickfix items with the keyboard. I think I will instead provide instructions to use the keyboard quickfix plugin, until #469 gets addressed.

Sorry, I didn't mean quickfix like I did when refactoring. In this context, I mean the context menu that poups for spell suggestions. You can navigate it with keyboard arrows.

@theol0403
Copy link
Member Author

Sorry, I didn't mean quickfix like I did when refactoring. In this context, I mean the context menu that poups for spell suggestions. You can navigate it with keyboard arrows.

Sure, even though keyboard arrows are of limited use, I don't see a reason not to add it 👍

@theol0403
Copy link
Member Author

Calling it for tonight, but end is in sight 👍

I took a lot of liberty in this PR by changing whatever I saw fit, please feel free to veto anything you aren't comfortable with.

Todo in OP has been updated.

Do not capture the Escape key when parameter hints or the problem popup
is open.
Copy link
Collaborator

@trkoch trkoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could split this PR and get the non-controversial parts merged?

package.json Outdated
"displayName": "Neo Vim",
"description": "Real VIM in your Visual Studio Code",
"displayName": "VSCode Neovim",
"description": "Real VSCode Neovim Integration",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the previous description better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to discussion, but I guess I was trying to make the "branding" more consistent. "VSCode Neovim - VSCode Neovim Integration". I changed it in my last few commits, let me know if you still prefer the old version.

"command": "vscode-neovim.send",
"key": "home",
"when": "editorTextFocus && neovim.init && neovim.mode != insert || neovim.recording",
"args": "<Home>"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this break native scroll navigation when pressing Home/End?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean. In vscode, home/end moves to the beginning and end of the line - this is what neovim also does.

@@ -93,5 +93,9 @@ xnoremap <C-w>gd <Cmd>call VSCodeNotify('editor.action.revealDefinitionAside')<C
xnoremap <expr> <C-/> <SID>vscodeCommentary()
nnoremap <expr> <C-/> <SID>vscodeCommentary() . '_'

" open quickfix menu for spelling corrections and refactoring
" only keyboard arrows can be used to navigate, for a solution, see https://github.com/asvetliakov/vscode-neovim#keyboard-quickfix
nnoremap z= <Cmd>call VSCodeNotify('editor.action.quickFix')<CR>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is something of universal use. Users can remap this easily in their local installation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do agree, I think that it does not hurt to have some default functionality for a common keybinding, rather than it not working at all.

@@ -393,6 +393,106 @@
"key": "ctrl+p",
"when": "listFocus && !inputFocus"
},
{
"key": "j",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is for navigating lists with j/k?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. Taken from VSCodeVim.

@theol0403
Copy link
Member Author

Maybe you could split this PR and get the non-controversial parts merged?

I feel bad for taking forever to complete this PR, but I think everything should be done at once - the keybindings are coupled with the readme, and I don't want to work on the readme in two passes (write docs for the new bindings in the old format, and then convert to the new format). And then, since the whole readme is being refactored, it would also be a good time to change the branding and whatever.

I will be ready to submit this PR for review after I complete the new peek mode bindings and adding a table of contents. Probably I can get that done in the next couple days.

After this is merged, I'll also look into moving a lot of the information into the wiki, and possibly adding more stuff in terms of opinionated mappings - the readme is already getting very big.

@theol0403
Copy link
Member Author

I have merged #604 into my PR, I hope they still get recognition for it. I did it because I needed to modify it to also close the peek editor.

@asvetliakov
Copy link
Member

I'm eager to merge it once it done. Awesome!

@theol0403
Copy link
Member Author

Well, I think everything I plan to do is complete 🎉!

However, give me a day or two to just go over it and make sure everything is perfect. I think the only thing I might change is the order of some bindings in the readme, and possibly squash some meaningless commits.

If you want to start the review, however, feel free!

@theol0403 theol0403 changed the title [WIP] Refactor bindings and README Refactor bindings and README Jun 2, 2021
@theol0403 theol0403 marked this pull request as ready for review June 2, 2021 07:17
@theol0403
Copy link
Member Author

I have made some more improvements, and rebased/squashed some of my commits to make it easier to read and know what changed.

I think this is ready to be merged! 🎉

Of course, please make sure you agree with all the decisions I made before merging 👍

@theol0403
Copy link
Member Author

Sorry for all the force pushes, I just keep finding little things I want to change that don't deserve a new commit 😅.

Do you want me to press the merge button, or do you want to do it?

@asvetliakov
Copy link
Member

asvetliakov commented Jun 3, 2021

Go (merge it!) 🚗 😄

@theol0403
Copy link
Member Author

Last question, do you want to squash, or keep commits as-is? The latter might be better, to better isolate the individual changes, but there is also a bit of commit mess.

@asvetliakov
Copy link
Member

it's not very important. squashing helps when you're doing some kind of deploy-per-commit, but it's not relevant to this repo. so whatever you feel right. I'd squash it

@theol0403 theol0403 merged commit 8ee0c96 into vscode-neovim:master Jun 3, 2021
@theol0403 theol0403 deleted the refactor-bindings branch June 3, 2021 06:17
@theol0403
Copy link
Member Author

Finally!! Hopefully people will like the new changes 😄

Thanks again for your awesome work on this plugin ❤️! I hope to see it grow in the future.

Just a small thing, can you capitalize the i in "VSCode Neovim integration" in the github description to stay consistent?

@asvetliakov
Copy link
Member

asvetliakov commented Jun 3, 2021

awesome work! thank you

Just a small thing, can you capitalize the i in "VSCode Neovim integration" in the github description to stay consistent?

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants