Fixed multicursor backspacing #4804
Conversation
|
@kevin-bates Can you please take a look at this or direct me to someone else who can? Thanks |
|
Thanks for the ping Ben. I'm really not qualified to look at this since I don't spend time on the front-end. I'm going to defer to two of the folks that are on the file's history that have been fairly active lately - Jason and Grant. Thanks for the contribution! cc: @jasongrout @gnestor |
|
Kevin, thanks for passing this on. Jason and Grant, let me know if you have any questions, and I'd be happy to answer. |
|
Any updates? |
|
Hi Ben, here are a couple more "pings" for folks that have had some activity on this specific file. Hopefully you'll get some feedback soon. |
|
@kevin-bates since nobody has reviewed this yet, would it be okay if I tried to find an active contributor to the CodeMirror library to review this, even though they might not have experience with jupyter? |
|
Hi Ben - sorry for the frustration. It certainly can't hurt to have a CodeMirror contributor review your changes. It seems that this repo is under-staffed at the moment so perhaps having this been reviewed at all will kick things into motion. |
|
@marijnh I see you've been active on the CodeMirror codebase and have written code that is somewhat similar to what I've written here. I've been unable to find someone who works on the Jupyter codebase to review this code. Would you be able to review my code? I would appreciate it |
|
@bthayer2365 Hi. Sorry, too busy to start reviewing patches in other projects. |
|
@bthayer2365 - Here are a couple more suggestions to try to get this reviewed...
|
3 participants
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 bycm.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
this is what backspacing would do
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!