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

Changing to normal mode when clicking is not clearing RecordedState #5719

Open
berknam opened this issue Nov 20, 2020 · 4 comments · May be fixed by #5842
Open

Changing to normal mode when clicking is not clearing RecordedState #5719

berknam opened this issue Nov 20, 2020 · 4 comments · May be fixed by #5842
Labels

Comments

@berknam
Copy link
Contributor

berknam commented Nov 20, 2020

Describe the bug
Currently when vscode creates a selection (like when using the "Find" widget) we go to visual mode. If the user then presses escape on the widget or closes the widget we are still in visual mode and if we press escape or use an operator everything works fine. However if the user clicks somewhere to get out of the widget instead of pressing escape or closing the widget, then the handleSelectionChange function changes the mode to normal, however since there was no action executed the RecordedState doesn't change and if we were on insert mode when calling the "Find" widget, the RecordedState will still have the action that was used to go to insert mode on the actionsRun.

This was mentioned by @ElvenSpellmaker here #3372 (comment).

To Reproduce
Steps to reproduce the behavior:

  1. Go to insert mode by pressing i or a
  2. Press <C-f> or call the "Find" widget from the command pallete
  3. Write a word to search (this step is probably not needed)
  4. Click somewhere to get the focus out of the "Find" widget (I usually press escape, that's why I never noticed this... 😄 )
  5. It goes into normal mode
  6. Press i or a to go to insert mode again and it fails...

Expected behavior
The recorded state should be cleared when we change to normal mode after a mouse click.

Additional Context
@J-Fields We could simply clear the vimState.recordedState when changing mode after the click or we could add that logic to the VimState.setMode function. Which one do you think it's better?

@J-Fields
Copy link
Member

Interestingly enough, I can't reproduce this with those steps, but I have seen the same sort of thing recently - I think a recent change might have uncovered another way of triggering this, but I'm not sure what that is, exactly. So probably putting it in setMode is better?

@sql-koala
Copy link
Contributor

Hi @berknam
If you work on this, could you take a look at this?
#5077

It is about a bug in "gb" action (create multicursor) that has a similar internal cause (recordedState is not cleared).
Maybe this can also be fixed in setMode.

@berknam
Copy link
Contributor Author

berknam commented Nov 28, 2020

@sql-koala that is a different issue I think. It happens because the recordedState.count is not being cleared when it should. For instance when you run a motion the executeMovement function does clear the count, but executeOperator doesn't and neither does the execCount of a BaseCommand, that should do it if it was a completeAction.

I have a fix for that already. The problem is I've been working on implementing a new feature that has just kept growing and growing. I really don't know when to stop! 😓 (that fix happens to be in the middle of that, oops!)

I really need to learn to start splitting everything, but the problem is that I'm implementing something and then realize: Oh! For this to work properly, I need to implement that other thing as well. And that goes on and on and it escalates pretty quickly.

On the bright side, though, I might have just implemented keyboard selections with shifted keys, SelectMode, keymodel setting, selectmode setting and maybe even mouse setting, the various multiple -- (insert/replace) visual/v-line/v-block/select/s-line/s-block -- pseudo-modes that happen when you go to visual/select mode when you are on insert/replace mode and when it leaves that visual/select mode it goes back to the previous insert/replace instead of normal mode. This makes the SelectMode look and feel pretty good when you select something on insert mode (either via mouse or shifted keys)

@berknam
Copy link
Contributor Author

berknam commented Dec 3, 2020

@J-Fields So upon looking further into this we can't make the change in vimState.setCurrentMode because that is used a lot in the middle of actions. Some actions set the mode to Normal and then later change to another mode, like the ChangeOperator, for example, changes to Normal when running the YankOperator and DeleteOperator and then changes to InsertMode.

I found another way to do this that I think it's better. We can separate a single click from a click selection or a double click selection. So what I'm doing is, when we get a single click we send a special key (<LeftMouse>) to modeHandler.handleKeyEvent which triggers a motion action MoveLeftMouse that then updates our cursorStopPosition according to vimState.editor.selection.active. This not only appears to work pretty well so far but it also brings with it some great improvements like:

  1. You can use left mouse click as a motion like in normal VIM. You can press d and then click somewhere and it will delete from cursorStartPosition (where you were when pressing 'd') until the click position (exclusive motion like in VIM)

  2. You can remap <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.

From what I've tested this seems to work pretty well. And that no.1 can be pretty good (when you are already holding the mouse in your hand.

BTW, I know I haven't been helping much lately but I've had some busy weeks, sorry about that.

However, remember when I said I was going to implement the shifted keys actions like mentioned here #5050 (comment)? Well... I started working on that by implementing keymodel, then I realized that to properly implement it I needed to have SelectMode, so I implemented it along with selectmode setting! All of this things were related to each other, so I ended up implementing pretty much all of them without even noticing. So I have another big refactor/overhaul again, oops 😓 😅

The problem is that, because I was busy, I was working on this only a few bits at a time on different machines, so I kept amending a 'wip' commit, just to be able to synchronize the work between both machines. So now I'm going to have to split all changes on different commits (also along the way I might have made some simple fixes here and there that were bothering me... 😅 )

I will try to push a draft PR soon so you can try it out and then tell me if it is better to split it all up piece by piece or if we can agglomerate some things together.

@berknam berknam linked a pull request Dec 7, 2020 that will close this issue
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@berknam @J-Fields @sql-koala and others