-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Select mode implementation with keymodel
and selectmode
settings
#5842
base: master
Are you sure you want to change the base?
Conversation
If anyone can give a try to this PR and report back I would really appreciate! From what I've tested this looks and feels really good. I didn't know I wanted SelectedMode so much before having it! 😄 I didn't change the behavior of snippets, they still stay in insert mode with a vscode selection, but that could (probably should) be changed, so that after a snippet it would go into SelectMode, this way it would look and feel the same, however you would have all SelectMode remaps and all SelectMode actions available. The only reason I didn't implement this is that it would require not capturing the TAB key like it happens in insertMode, but maybe it's better to do so. |
@@ -849,6 +965,23 @@ | |||
"description": "If enabled, dragging with the mouse activates Visual mode.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I was confused when I was looking at the config. The description should probably be updated since this is depended on vim.selectmode
. Or maybe remove completely since we have selection mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of making it "deprecated" and while it is deprecated it would change the defaults of 'selectmode' to be:
"vim.selectmode": "key", // <- it removes the 'mouse' option so it goes to Visual Mode instead of Select Mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this isn't correct. Because this setting is the equivalent of having at least mouse=nv
or mouse=a
. Because this is the setting that allows a mouse selection to start visual mode. The selectmode=mouse
makes the mouse selection start Select mode. So in order to replace that setting we would have to implement the mouse
setting. The problem is that in VIM if you have mouse=
the mouse doesn't do anything, it doesn't even move the cursor, and even though this might be possible to do, it probably won't look good, since you'll have the cursor changing place and then go back again to where it was...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically this setting on/off is the equivalent to having on
-> mouse=a
or off
-> mouse=ni
, except that when off
the vscode selection is still kept, even though VSCodeVim doesn't know about it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on
->mouse=a
oroff
->mouse=ni
the vscode selection is still kept, even though VSCodeVim doesn't know about it
until user clicks <shift+right>
for example. As long as we are not implementing mouse
option, I think we just need to clarify what vim.mouseSelectionGoesIntoVisualMode
does? Also, I don't know how hard it is to implement mouse
option?
Since it doesn't work exactly like the mouse
option, maybe updating the description to something like following would help?
If enabled, dragging with the mouse activates Visual/Select mode depending on
vim.selectmode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the description needs to be updated. And so does the README file, I think we need to create a "Selections" section on the README to explain all things related to selections, but I really didn't want to add to the already big confusion that is the README file. We need to create an organized Wiki...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/VSCodeVim/Vim/wiki/Selections
If you feel so inclined, dump stuff here - even if it's not polished, better than the current state of documentation.
I tried out some simple usages of select mode as I am not too familiar with select mode, and it works great! That's some amazing work. Thank you :) |
How simple would it be to split the following out into their own commits/PRs?
As well as some of the bug fixes unrelelated to select mode? |
*/ | ||
isCompleteAction = true; | ||
isCompleteAction = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the default value of isCompleteAction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the explanation above. When it isCompleteAction
the action is supposed to handle the recordedState.count
(either by the default way, which calls the exec
a total of recordedState.count
times or by handling it in a custom way on execCount
).
This was supposed to remember people that if the action isComplete than you have to set that to true
and then handle execCount
or let it be handled by default.
If you think this was better the other way, I can change it...
this.keymodelStopsSelection = km.includes('stopsel'); | ||
|
||
// set selectmode options | ||
const sm = this.selectmode.split(','); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is better done by defining a getter for selectmode
which returns a value like this:
{
mouse: boolean,
key: boolean,
cmd: boolean,
}
(and similar for keymodel
). Or maybe a separate method (not a real getter) to avoid messing up things like :set selectmode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would definitely look better, but I'm afraid that it won't be as readable and as simple to understand:
if (configuration.selectModeMouse) {
vs
if (configuration.selectMode().mouse) {
I could even change the current implementation adding By
like this, which could be even more readable:
if (configuration.selectModeByMouse) {
if (configuration.selectModeByKey) {
if (configuration.selectModeByCmd) {
- Implements 'keymodel' setting - Make backspace work on all visual modes - Implement '<S-BS>' and '<C-BS>' - Implement shifted special keys (arrow keys, Home/End keys, PageUp/PageDown) - Implement shifted special keys on commandLine modes - Implement 'modeBeforeEnteringVisualMode'. It allows to have the pseudo insert/replace visual modes that go back to insert/replace after escaping visual instead of going back to normal - Add rest of visual modes and replace mode to possible wrapping modes - Implement '<C-o>' in replaceMode
- This creates a new motion action MoveLeftMouse that runs when clicking - This allows to use left mouse click with operators and allows to remap the left mouse click
…ly on visual mode
- Fix <C-a>, <C-x> (Increment/Decrement number) in visual mode changing the number twice (it was getting the ranges duplicated) - fix <C-a>, <C-x> (Increment/Decrement number) in visual mode updating outside the range (in visual mode if you select '1' on the number '14' it should increase to '24' but it was going to '15')
- Implements all Select modes - Implement 'selectmode' setting - Implement SelectMode remaps
- make 'recordedState.count' be cleared after 'executeOperator' and after running a completeAction 'BaseCommand' - made the enter visual mode command 'v' be a complete action. (why was it not a complete action before???) - changed BaseCommand to have 'isCompleteAction' false by default so that we know that when setting it to true we need to either handled the count properly or have the 'runsOnceForEachCountPrefix' set to true
c65d87a
to
90bf1bc
Compare
90bf1bc
to
8604d97
Compare
It might be doable, but it might not be that easy because some of the things are related to each other and depend on each other. I tried to make the commits in a way were this things are separated. If you wanted this because of being easier to pinpoint issues, then I can just push the commits on this PR directly to master, once we feel the PR is ready, instead of merging this PR. This way all commits will be in history and should be possible to pinpoint any issues created by any of this commits. |
Makes total sense. I think the best way to do this is with a real merge commit, rather than the squash-merge we usually do. |
@berknam Thanks again for the excellent work! I don't understand every nook and cranny of this, but it all seems reasonable to me and I trust you've figured out the details well enough, so I'm ready to merge this when you are. If you think any of the TODOs you listed are blockers, let me know; otherwise, could you migrate them into their own issue(s) so we can discuss & track them separately? |
@J-Fields I'm sorry I haven't had any time to work on this project lately. I hope that by the end of this month the situation should get better and I should be able to get back to this project. |
This is awesome! I'm constantly bothered by the fact that using shift+arrowKeys to select code will leave me in NORMAL mode instead of putting me into VISUAL mode.(yes, I am a newbie who uses arrowKeys a lot😂) Hope this gets merged soon! |
Hey @berknam - do you still plan on getting back to this? I'm very happy to help fix merge conflicts and split this up so we can start merging it in |
keymodel
and selectmode
settings
Can we get this merge , this would make vscode VIM experience much better. ( Or i would end up switching to neovim soon lol) |
What this PR does / why we need it:
This started with the implementation of 'keymodel' and the shifted special keys to replace the PR #5050. However it grew larger than expected! 😅 (sorry @J-Fields )
Here is what is included now (new implementations):
<S-arrowKeys>
,<S-Home>
,<S-End>
,<S-PageUp>
,<S-PageDown>
<C-o>
on insert or replace mode, it is used just to give the same 'showmode' as normal VIM with-- (insert) --
or-- (replace) --
)<C-o>
on insert/replace mode or by using the shifted keys in insert/replace mode withstartsel
set on 'keymodel', or by doing a mouse drag selection on insert/replace mode. They show as-- (insert) VISUAL --
or-- (replace) VISUAL LINE --
for all possible combinations. The big difference of these modes is that after escaping (<Esc>
) them or leaving the mode with an action like a delete or yank operator that would usually go to normal mode, now it will go to the mode before entering visual mode (insert or replace). (More information on this modes can be found on:help operator-pending-mode
, it has info on all pseudo-modes)<C-o>
in replace mode (same as in insert mode, but it reverts back to replace mode)<LeftMouse>
:MoveLeftMouse
that runs when left clicking somewhere<LeftMouse>
) tomodeHandler.handleKeyEvent
which triggers a motion actionMoveLeftMouse
that then updates ourcursorStopPosition
according tovimState.editor.selection.active
. This not only appears to work pretty well so far but it also brings with it some great improvements like:d
and then click somewhere and it will delete fromcursorStartPosition
(where you were when pressing 'd
') until the click position (exclusive motion like in VIM)<LeftMouse>
! I don't know if this is useful at all, but it is something you can do just like you can in normal VIM. If you remap it, then our cursor won't be updated and vscode selection is changed back to our cursor.SELECT
,SELECT VISUAL
,SELECT BLOCK
and the pseudo-modes like-- (insert) SELECT --
)gh
-> enters SelectMode (also exits VisualMode)gH
-> enters SelectLineMode (also exits VisualLineMode)g<C-h>
-> enters SelectBlockMode (also exits VisualBlockMode)<C-g>
to swap between Visual and Select modesselectModeKeyBindings[NonRecursive]
for only select mode andallVisualModeKeyBindings[NonRecursive]
that work for visual and select modes.allVisualModeKeyBindings[NonRecursive]
remaps are the same asvmap
on Vim and our existingvisualModeKeyBindings[NonRecursive]
are the same asxmap
on Vim.vmap
(allVisualModeKeyBindings
) when ran in Select modes. In this situation vim temporarily changes to visual mode so that the behavior of the remap is the same regardless of the mode you're in: (:help select-mode-mapping
)vscode
that would make those situations always go to Select mode with that setting, or (my favorite option) we can do it on a per mode basis, so if we get one of those selections and we are in normal mode we would go to Visual, but if we were on insert/replace mode we would go to Select, this would be more similar to the experience the users have been getting since selections in insert mode currently behave just like Select mode.Small bug fixes included:
shouldWrapKey
to include all visual modes and replace mode (TODO: this function should be used in all places that are affected bywhichwrap
, currently it is only applied onh
andl
and the rest either don't have it correctly implemented or re-implement something similar to this function)2v
it will select 2 characters.gv
(ReselectVisual
) going left when mode is different from visual mode. It should only update left on visual mode.IncrementDecrementNumberAction
:recordedState.count
not being cleared when it should. MakerecordedState.count
be cleared afterexecuteOperator
and after running aBaseCommand
withisCompleteAction = true
, also made the enter visual mode commandv
be a complete action. (why was it not a complete action???)BaseCommand.isCompleteAction
tofalse
by default so that we know that when setting it to true we need to either handle the count properly or have therunsOnceForEachCountPrefix
set to true. This makes theisCompleteAction
actually behave like a flag that states that the action ran was complete. Fixes Repeatinggb
once when selecting Multi-Cursors means subsequentgb
are repeated that amount #4075 and can replace Fix gb with count #5077.Which issue(s) this PR fixes
Some of them are state above. Others were never created and there are quite a few that should be fixed by this but I just haven't had the time yet to look for them (I'll try to add them later).
Fixes #5716
Fixes #5668
Fixes #934
Fixes #885
Fixes #5607
Fixes #4972
Fixes #4969
Fixes #2224
Special notes for your reviewer:
This again is quite an overhaul, but I think it had to be in order to properly implement SelectMode. I tried to split the fixes on their own commits before the SelectMode commit so that we could cherry-pick if needed (except for the last fix that made changes to pretty much all actions so I made that one after SelectMode to include the new actions as well)
Please check out the
TODO
s mentioned above and give me your opinion. There is also the following todos that I would like an opinion in some of them:go to one of the select modes and then use again to run one visual/v-line/v-block command, after this
you go back to select mode, then if you escape you go back to insert mode)
for it, ideas?!
Some issues I have:
inoremap <S-Home> <S-Home><C-g>o<left>o<C-g>
(the<C-g>
are only needed if 'selectmode' has 'key'):help select-mode-mapping
, the printable keys pressed in select mode go through the insert mode remapper. In order to implement this I needed to have access to the modeHandler (usingawait getAndUpdateModeHandler()
) inside the actionCommandInsertInSelectMode
in order to resend the key, so that it goes through the insert mode remapper. I really don't like this, but I couldn't think of another way!