Update cursors with 'keyboard' kind Selections #5050
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Travis tests have failedHey @berknam, Node.js: 12if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; finpm testTravisBuddy Request Identifier: 1aeb7e20-ca40-11ea-84bb-258f0278865c |
berknam
force-pushed
the
selectionsWithKeyboard
branch
from
901a086 to
5b7ca5b
Compare
|
@berknam Could you resolve the conflicts when you have a chance? |
- Selection change events not triggered by us of type 'Keyboard' will now enter visual mode (if they are not empty selections). - This allows entering visual mode with shift+arrowKeys or shift+end, shift+home.
berknam
force-pushed
the
selectionsWithKeyboard
branch
from
5b7ca5b to
f0d35a5
Compare
|
I had to rebase this correctly. Since github uses the squash and merge it no longer recognizes the previous commits. So I had to rebase it to master. But should be good now. |
- When selecting with shift to the left or up and then performing a normal movent would result in the anchor moving to the right. - This wasn't happening on multicursor because I had the selection changing properly and was ignoring any intermediate selections - Changed these situations to instead of changing the selection with the ignore selection change flag, it calls the updateView and lets it draw the selection which means that any resulting selection change event will be properly ignored.
|
There was a bug when shift selecting to left/up and then making other movement without shift that I've now fixed. I had previously fixed it and then commented that out because the way I fixed it was by using the |
|
I maintain the VSpaceCdoe extension which tries to mimic spacemacs experience in vscode. The extension has a binding to call smart selection grow with a key binding of
The bug doesn't occur when it is triggered by vscode shortcut directly. Is there any way I can help debug this? |
|
@stevenguh Have you tried it with the latest version from this PR? Because I tried using the same remap just now and it works fine. There was a bug that I fixed on the last commit that might have been creating the issue you're seeing. Also you can look at the developer tools console that should show some debug info related to the selections. If you can't figure it out, just post it here and I can have a look. |
|
I think I figured it out. This is because this PR changed to Lines 242 to 245 in 10701cf from The issue is more involved. VSpaceCode is a command that binds to One fix I can do on my side is to not return a promise, so vim is not blocked because of keybinding execution. |
|
That change doesn't seem to be the issue. I've tested this with your extension and that part of the code is never hit until you exit the QuickPick menu. The selection change events are being enqueued but they are not called until you exit the quickpick menu. I've tested with the code from PR #5015 and the behavior from that PR and this PR are the same for me. |
|
That's super weird. Changing that line definitely fixed it for me, and I am on 10701cf. My steps to reproduce are
This is a highlighting problem for me, any operation after that will result in correct highlighting like moving the cursor to the left |
|
I see the problem. When creating the visual selection we need to change vscode selection so we need to call that line allowing the draw selection so that we ignore any resulting selection change events properly. Then when updating the selection since we are not changing it there was no need to call the I'll push a fix for that in a moment. |
- We were not changin the vscode selection when we got a selection change that updated an existing visual selection because vscode would have that selection already, but when using the extension VSpaceCode we would get all the events in a row at the end, meaning that the first event that created the visual selection would change the vscode selection (to the initial one) but the following events would not change it again which would result in the final selection not being correct. - Now we update vscode selection when updating the visual selection as well.
|
@stevenguh That should be fixed now. At least the correct selection is shown after you exit the QuickPick menu, but you should probably not block until the user exits the QuickPick menu. Why do you need to await that promise? |
|
That makes sense. Thank you for looking into this. I will update the extension later today so it's not blocking. EDIT: The extension ( |
Nice. That looks better. @stevenguh By the way just a suggestion, but if you want the selection to look better (currently in some themes the inactive selection is barely noticeable) you can try to change the setting |
|
Thank you for the suggestion. I created an issue in |
|
Sorry to piggyback on this. With the latest vim update, the smart select from normal to visual mode works sometimes, and doesn't work robustly. I am wondering is this what we needed to have a robust smart selection to visual mode? Or should I be opening a new bug to track that. EDIT: Never mind, #5250 seems to fix this issue I am experiencing. This is somewhat weird that this is happening with C# file with the official C# extension. Rough repo:
|
I can't reproduce this. When this happens check the developer tools console to see if you have something like this: That "Count left: 0" is the important part, it is supposed to always get back to 0, if it is always higher than 0 then something wrong happened and smartSelect won't work (including some other weird issues will show up). Now this usually happens when working with multicursors or snippets that create multicursors. Perhaps you typed some snippet? If that wasn't the case and you actually find a way to reproduce this without using multicursors or snippets please tell me! |
|
This is weird. I can't consistently reproduce with those steps today, but I can still hit bug from time to time. I am not using multi cursor or snippets. I also checked out the log when I hit the bug, and the diagnostic array is empty. Also I can't find a way to recover it (meaning to have the smart selection working again for that editor) once i hit the bug. Even closing the file and reopening won't fix it. I have to restart the window. I will keep you posted if I found a consistent way to repo this. |
Confirm if you have the setting |
|
I just turned that option on, it is still empty when I am in the bug. I realized that it also happened in other type of files as well not just C# editor. I don't have a consistent repo yet. EDIT: in one bugged instance, I have something like this. I smart select, and then press |
|
@berknam Could you please resolve the merge conflict when you get a chance? |
I'm looking into this, but it wasn't a simple fix the conflict as I expected. We've made some other changes with selections required by some other issue that showed up in mean time and I need to see the best way to not create that issue again. |
|
@J-Fields I've been thinking about this and I don't believe this is the right approach to this issue. I think it would be better to implement actions for the If we had 'keymodel' empty the shift right and left keys would behave like This way it would make this ready for when we implement SelectMode, since then we would only have to implement the 'selectmode' config and if that one included 'key' while 'keymodel' included 'startsel' then those keys would start a selection in SelectMode instead of VisualMode. This would also have the benefit that since we would start capturing these keys, then they will be available for remaps as well. If you agree that this is the better way, then I can change this PR or create a new one. If not, I can try to fix this PR as it is just to get it working. |
|
Another benefit would be removing this logic from the 'handleSelectionChange' function and reduce its complexity! 😄 |
|
Ah nice, I wasn't aware of
Because we'd handle all these through custom commands and be able to ignore |
Mostly yes! I would have to look still, because there are some instances that are marked as 'keyboard' selection events (like calling a I do have currently a potential fix for this PR that would make it work, but it is a little bit complex and for it to work well with multicursors I would have to copy that complexity to the multicursor logic as well. Also it doesn't keep the desired column when moving up/down and doesn't have any configuration tied in to let users opt out of it. Since it is partially done, if you want I can finish that and push it here, and then create a new PR for the other option. (I'm proposing this because I don't have any idea of how long it might take me to make the other option) |
Open
This was referenced Dec 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
now enter visual mode (if they are not empty selections).
shift+home even when on insert mode and with multi cursors.
Which issue(s) this PR fixes
fixes #2094
related to #1806
Special notes for your reviewer:
This PR is built on top and depends on PR #5015 and should not be merged before that on is merged first.This has now been rebased to master and is ready to merge.