Implement better synchronization of selections

[berknam]

What this PR does / why we need it:
Implement better synchronization of selections

• Store our selections when calling 'updateView' to be later ignored.
• Create a 'ignoreIntermediateSelections' flag that is set when running
  an action to ignore any selection changes triggered by any step of that
  action.
• When getting a selection change event we first check if it is one of
  our selections. If it is we ignore it, if it is not we still ignore it
  if the 'ignoreIntermediateSelections' flag is set or if we have other
  selections to ignore, because that means this selection slipped in after
  the end of the action. Otherwise we handle it.
• Create new handle for selection changes with kind 'Command'. (this is
  triggered by editor commands like 'smartSelect.grow' but is also
  triggered when we set the 'editor.selections' on 'updateView', except
  this last one will now be ignored)

Which issue(s) this PR fixes
fixes #1806

Special notes for your reviewer:
Since this has to be tested manually I would like you to test this manually as well before merging since there might be some weird cases that I forgot to test.
I did my best to test all the scenarios I could think of, and in doing so I noticed quite a few bugs with selections but they were all present even before this fix. From what I've tested this PR at least doesn't introduce more bugs. I didn't want to fix those bugs here so that this PR didn't became huge like the remapper did 😆.

With this change we are no longer handling the selections triggered by us. Which means that if 'handleSelectionChange' is called it is either from a click, some other extension using commands that trigger selection changes, the user using keybindings that don't pass through Vim and trigger selection changes, or the user using remaps to commands.

[J-Fields]

Thanks @berknam - haven't had a chance to look at this yet (and it'll require some thought on my part as this stuff is subtle). Could you try adding a test for smartSelect? A lot of the selection stuff is difficult to autotest but that should be doable

[berknam]

Sure! (When I was doing those tests manually never occurred to me that those could be auto tested, whoops 😓 ) I'll see if I can add them later tonight.

[J-Fields]

Thanks, no rush!

[berknam]

Actually this might not be possible to test. The issue is that the changing to Visual mode and updating the visual mode cursors is all done on the 'handleSelectionChanged' function. However the 'registerEventListener' on 'extensionBase' has the flag 'exitOnTests' set to true which means that on tests this event doesn't trigger the 'handleSelectionChanged' function.

If I change that flag to false, then a whole lot of tests start failing. I f I change that flag on 'master' then I get 283 failing tests.

This is due to the fact that on 'updateView' when we set the 'editor.selections' it will eventually trigger that selection changed event but there is no way for us to await for that so we keep going and then later on that event is trigger and changes our cursors messing up the test. (we are talking about milliseconds here but our tests are run in milliseconds as well, that is why that's an issue with tests but not in normal use)

I don't think we can fix that issue, which means these types of tests might not be possible. I'm still looking into it.

[J-Fields]

Ah, gotcha. That's a bit annoying and maybe something we should address some time, but not a huge deal for now

[berknam]

Actually I think I just found a way to make this work with that function enabled on tests with all tests passing! 😁
I have previously tried to have a 'vimState.ignoreSelectionChanged' flag that would be set on 'updateView' when we set the 'vimState.editor.selections' and that flag would prevent the selection change event from calling the handler function and then unset that flag. But that wasn't working either.

But now I realized that the reason it wasn't working is because sometimes we could call the 'updateView' more than once before any selection event triggered and then when we got the first selection changed it would unset the flag and the second selection changed event would call the handler and mess things up.

So instead of a 'boolean' flag I now changed it to use a 'number' counter that keeps track of how many selections do we need to ignore and on 'updateView' we increase that counter by one, and when we get a selection we decrease by one.

This seems to be working fine and all tests pass now. I will do more testing and when I'm confident enough this is a good solution I'll push to this PR.

[berknam]

I think I've finally managed to get this working properly. I went through quite a few different iterations until getting to the current one.
First when using just a count to keep track of selections to be ignored wouldn't work because when we were setting the selection to be the same as it already was that wouldn't trigger a selection change event. So I started checking if the selection was different than the current one before increasing the count. Then the problem was that there were selection change events triggered in the middle of our actions, so I started keeping a count of actions to ignore and actions enqueued, and on 'updateView' I would set the selections to ignore to the enqueued selections plus 1 to make sure we would ignore the intermediate selections as well.
But that wasn't enough either because when testing I couldn't count on the order of the selections triggered events and it would also ignore selections triggered by remaps to commands.
So the final solution is I set a flag 'ignoreIntermediateSelections' when running actions to try to ignore all intermediate selection changes, then on 'updateView' I check if the selection is different and will trigger a change and then push a 'hashed' string of that selection to a string array. Then when getting a new selection change I first check if we have that selection in our selections to ignore, if we do then we ignore it and remove it from the array. If it isn't in our array but our array isn't empty it means that was an intermediate selection that was called after the end of the 'runAction' so we ignore it too. Otherwise the selection is handled. I tested this as best as I could. I also created test for the 'grow' command with multiple cursors and it works.

This still needs to be tested manually, but now that we have our tests running with this selection change events enabled I think we can be more confident that this is working and that any future changes should be ok if the tests pass.

[J-Fields]

Then the problem was that there were selection change events triggered in the middle of our actions

As an aside - this is something I'd like to reduce and avoid in the future. As much as possible, we should have a single boundary between VSCode's view of the world and our own, and that should be managed by ModeHandler.

[J-Fields]

Is there any way to trigger a selection change that we should handle, but then switch editors before we get here?

[berknam]

Yes. I can't exactly reproduce it all the times. But sometimes when you Ctrl+Tab between tabs, it will send a selection change with e.textEditor.document of a.txt when vscode.window.activeTextEditor.document is already at document b.txt, for example.
This usually ends up calling two selection change events with position (0, 0) with wrong document and then a third selection change event with the right document and the right position.
I sometimes can reproduce this when I run a debug session after closing one that already had a few files open. Then on the second session when I Ctrl+Tab this sometimes happens. This also happens sometimes when I first open vscode.

I've also had a situation a couple of times when this selection change event is called with vscode.window.activeTextEditor as undefined. I've not been able to reproduce this but I think this might either be some issue in vscode calling events late or it might be related to our taskQueue.

I decided to keep this check here because when it wasn't there it would result in a TaskQueue: Error running task. getLineLength() called with out-of-bounds line when we got one of this weird selection change events.

[berknam]

To make it even weirder the way I found that this happens more consistently is when one of the files you're switching with is the settings.json. I have no idea why but this file is the one that is always making this happen to me...

[berknam]

Also when this starts happening it does it with Ctrl+Tab and also by simply clicking on the tab to change file

[J-Fields]

At some point soon I'd like to move away from using window.activeTextEditor (implicitly in our custom TextEditor static methods) in favor of explicitly referencing vimState.editor, which should eliminate those errors in this case. Once that is done we can probably remove the check, since we won't accidentally reference the wrong editor/document. For now, I'm not worried about it. This all seems like a step in the right direction, I just need to do some manual testing then I'll merge.

[berknam]

Then the problem was that there were selection change events triggered in the middle of our actions

As an aside - this is something I'd like to reduce and avoid in the future. As much as possible, we should have a single boundary between VSCode's view of the world and our own, and that should be managed by ModeHandler.

With this PR, this will already be mostly true. With this PR all the selection change events we get are not from us (they are either triggered by the user with a keybinding that we don't handle or by another extension, or by the editor when changing tabs). All our actions that use commands that trigger a selection change will have that selection change event being ignored. This is not a problem because those actions (like the foldfix hack) will read the editor selection after the movement, they are not counting on the cursor being updated by the selection change event.

The only situations where we trigger selection change events and then handle them and update our cursors is when calling commands on remaps. But this is fine, it is not only expected but it is also necessary, I think. (Otherwise the commands from our remaps wouldn't work properly).

[J-Fields]

Yep, I agree. I think there are some places where referencing selections is unnecessary, and I'd like to remove those, but this does effectively insulate the rest of the system from the places where setting selections explicitly is necessary.

[J-Fields]

Thank you @berknam! There does seem to be an issue with desiredColumn not getting updated after a command like Expand Selection, but I can take a look at that after this is merged.