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

Fixed multicursor backspacing #4880

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Conversation

benthayer
Copy link
Contributor

This is the same as #4804, but I moved it over to it's own branch instead of master. Use this one.

Copied from #4804:
I took the time to fix #4796.

Previous code logic:
It seems like #765 actually only addressed part of the problem. @marceloramires checked to see if the first range in the list was a selection and assumed that if this was a selection, the others would be selections too. If this was the case, then delete all the text in the selections. This handled deleting multicursor selections.

There was then separate logic that handled the tabbing. This logic was only built to accommodate one cursor, so if it detects a tab, it deletes it, and if it doesn't, it uses the function deleteH(-1, "char"), which handles the situation like a normal deletion. Normal deletion worked as expected for multicursor, but the tab deletion was only built considering one cursor and specifically deletes the tab for the "primary" cursor, which is the one returned by cm.getCursor().

Updated code logic:
To fix this, I consider each cursor separately. For each cursor, I first check if it is a selection. If it is, I delete the selection. Then I know it's a cursor, so I check to see that all characters on the line that come before it are spaces. If they're all spaces, we use tab delete, otherwise, we only delete one character at a time.

Note on new logic:
I also want to note that deleting one character at a time is somewhat different from the previous logic. Before, even if there was text on the line, it would still use tab logic, so if I have

stuff = 'tabs | '
012301230123012301230123

this is what backspacing would do

stuff = 'tabs | '
012301230123012301230123

Now, it just deletes one character, which is what I found PyCharm did, and what I think logically makes sense. If you have spaces after the initial text, they're no longer used for indentation, and you're more likely to care about the specific number of spaces (which deleting would make unclear). If anything, I can put tab deletion back in, but I think it should consistently be 4 spaces rather than to the nearest tabstop, which would seem random unless you're counting all the characters in the line each time you hit delete.

If all this looks good, I'm ready to merge!

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit d406b8c into jupyter:master Jun 3, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multicursor sometimes only deletes on one line when there's spaces
2 participants