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

Delete matching bracket upon backspace #1267

Merged
merged 7 commits into from
Feb 7, 2017
Merged

Delete matching bracket upon backspace #1267

merged 7 commits into from
Feb 7, 2017

Conversation

ursuscamp
Copy link
Contributor

@ursuscamp ursuscamp commented Feb 3, 2017

#1228

When in insert mode, if a user is deleting an opening bracket, and a matching closing bracket is immediately after it, also delete the closing bracket. Corresponding test included.

@xconverge
Copy link
Member

related #1085 #1084

At the time it made sense to put this off in hopes that it was part of the vscode api...

@xconverge
Copy link
Member

and here is the vscode issue

microsoft/vscode#17490

@ursuscamp
Copy link
Contributor Author

I didn't know that. If you still prefer to wait, I won't hem and haw about it.

@xconverge
Copy link
Member

I will let @johnfn decide, I think it would resolve a few open issues

The problem is without the proper vscode fix, multicursor stuff is tougher I think, do you know if your fix works with multi cursors?

Also, your way is nice!

@ursuscamp
Copy link
Contributor Author

Certainly seems to:

multicursor

@xconverge
Copy link
Member

Is there a way that you can wrap the functionality using the editor option of whether a user has the "editor.autoClosingBrackets": true?

I think that this is great, might be worth adding a comment to the vscode issue to see what the status is or if they have any thoughts

@ursuscamp
Copy link
Contributor Author

It now checks for editor.autoClosingBrackets setting before deleting closing bracket.

@xconverge
Copy link
Member

I would also add single and double quotes as well for this behavior. And < is not included in the default vscode behavior

@ursuscamp
Copy link
Contributor Author

I added double, single and weird-thing () quotes to the behavior and removed angle brackets. I added the quotes to the PairMatcher.pairings` object. I was worried about what downstream affects it might have on the text objects associated with quotes, but from my testing I can't see any problems.

@xconverge
Copy link
Member

regarding weird-thing (back-tick) and <, what I meant was if you disable the vim plugin, and try various ones, only some of them actually auto delete ( { [ ' " I think are all of them not ` <

no big deal, would just leave it as is for now until @johnfn can give input

@johnfn
Copy link
Member

johnfn commented Feb 5, 2017

First of all, thanks for the PR. This is definitely an issue that has been annoying a lot of people for a long time.

Unfortunately, this problem is actually an annoying one. How does it work with stuff like you type in "(blah)" but then backspace blah and the (? We will somehow need to distinguish a ( that was just entered from a ( which was entered a few keystrokes ago.

The reason that I have held off solving this is that the correct solution to the problem is to perform all text edits and deletes via vscode api, rather than in the manual way that we are currently doing. Sadly, I believe this would require a large refactor to do correctly, so it's probably better to just manually cover these cases for now.

@ursuscamp
Copy link
Contributor Author

You're welcome!

In this case @johnfn, it behaves the same if you enter "()" and backspace the "(", as well as "(blah)" and backspace the "(blah". In both cases, it removes the matching bracket. I believed this to be the desired behavior.

@johnfn
Copy link
Member

johnfn commented Feb 5, 2017

Whoops, I never realized that. Alright, this seems good to me then. :)

// Don't delete bracket unless autoClosingBrackets is set
if (!vscode.workspace.getConfiguration().get("editor.autoClosingBrackets")) { return undefined; }

let deleteRange = new vscode.Range(currentPosition, currentPosition.getLeftThroughLineBreaks());
Copy link
Member

Choose a reason for hiding this comment

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

You can make all these declarations const.


if ("{[(\"'`".indexOf(deleteText) > -1) {
let matchPosition = currentPosition.add(new PositionDiff(0, 1));
if (matchPosition) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this if statement?

@johnfn
Copy link
Member

johnfn commented Feb 5, 2017

Oops, I always forget to release my comments. I have some small suggestions for improvement, but we should be fine here for the most part.

@ursuscamp
Copy link
Contributor Author

Thanks for suggestions @johnfn. Done.

@xconverge
Copy link
Member

this also fixes #1241

@ursuscamp
Copy link
Contributor Author

@johnfn Is there something I am missing with this?

@johnfn johnfn merged commit 3c957ba into VSCodeVim:master Feb 7, 2017
@johnfn
Copy link
Member

johnfn commented Feb 7, 2017

Nope, it's fine :)

Dunno if I mentioned this, but apologies for any late responses - I am currently on vacation, hopping all around and not really using my laptop all too much.

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