Skip to content

enforce new selection for "remove" action #2285

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

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

saidelike
Copy link
Collaborator

Checklist

From debugging the vscode cursorless extension, it seems the selection is automatically updated by vscode itself when dealing with the "remove" action while calling editBuilder.delete() (TextEditorEdit).

However it is not the case for neovim so it was using the old selection.

I fixed it so cursorless always enforce selection for that "remove" action. Tested on neovim and vscode.

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Good stuff. @AndreasArvidsson and I had discussed at one point having a version of performEditsAndUpdateSelections that does this automatically, as I believe it would be useful in other contexts, ie if you copy-pasted this code from somewhere else they could both use that abstraction

@saidelike saidelike requested a review from pokey April 5, 2024 15:16
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Looks good with a couple more minor tweaks

Btw what would happen if you just moved this work into the performEdits function? Then we could remove it from the place you stole this code from. @AndreasArvidsson do you see any reason not to always update the cursor position the way we want? I guess for eg "change" / "clear" it's a waste of work. But my leaning would be an option to disable the behaviour when you know you're going to move the cursor anyway, but to do it by default if not

@saidelike
Copy link
Collaborator Author

Looks good with a couple more minor tweaks

Btw what would happen if you just moved this work into the performEdits function? Then we could remove it from the place you stole this code from. @AndreasArvidsson do you see any reason not to always update the cursor position the way we want? I guess for eg "change" / "clear" it's a waste of work. But my leaning would be an option to disable the behaviour when you know you're going to move the cursor anyway, but to do it by default if not

I propose to look into that in a future PR.

@saidelike saidelike requested a review from pokey April 5, 2024 15:47
@AndreasArvidsson
Copy link
Member

Looks good with a couple more minor tweaks

Btw what would happen if you just moved this work into the performEdits function? Then we could remove it from the place you stole this code from. @AndreasArvidsson do you see any reason not to always update the cursor position the way we want? I guess for eg "change" / "clear" it's a waste of work. But my leaning would be an option to disable the behaviour when you know you're going to move the cursor anyway, but to do it by default if not

The only reason I can think is if you make multiple edits and just want to update the selection at the end. I have no idea if we do that in any place and if we find that we need that behavior we could always add a parameter. Updating selection by default sounds reasonable.

@pokey pokey added this pull request to the merge queue Apr 15, 2024
Merged via the queue into cursorless-dev:main with commit 35ff9da Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants