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 #7401

Merged
merged 5 commits into from Oct 25, 2019
Merged

Fixed Multicursor backspacing #7401

merged 5 commits into from Oct 25, 2019

Conversation

@benthayer
Copy link
Member

@benthayer benthayer commented Oct 22, 2019

References

fixes #7205

Other references:
jupyter/notebook#4880
jupyter/notebook#4796

Code changes

Iterates through selections, and handle backspacing appropriately. There are three cases:

  1. Selection
    • Delete everything in selection
  2. Leading spaces
    • Delete to previous indentation
  3. Other
    • Delete previous character

3. Can be broken down into the cases of the beginning of the line too, but they behave as you would expect

This solution is similar but not exactly the same as for the same problem in jupyter notebook jupyter/notebook#4880

User-facing changes

Basically, backspacing when using multicursor works as expected now whereas before it didn't handle tabs correctly.

Backwards-incompatible changes

None

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Oct 22, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 22, 2019

Thanks @bthayer2365! I don't have a working laptop at the moment to test but this logic looks great to me!

@benthayer
Copy link
Member Author

@benthayer benthayer commented Oct 22, 2019

Great, looking forward to you testing it!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 24, 2019

Very nice! This is identical to the behavior I see in VS Code. I think these build errors were temporary, kicking the builds.

@benthayer
Copy link
Member Author

@benthayer benthayer commented Oct 24, 2019

Linux integrity errors:

ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1364:5 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1365:5 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1366:10 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1368:7 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1369:7 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1370:7 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1374:9 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1377:11 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1378:11 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1382:23 - == should be ===
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1383:27 - != should be !==
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1384:15 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1384:19 - Duplicate variable: 'from'
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1391:13 - Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: /home/vsts/work/1/s/packages/codemirror/src/editor.ts:1391:17 - Duplicate variable: 'from'

I fixed all the var/let issues. Changing var to let for from should remove the duplicate variable error. Also fixed == and != errors.

Windows JS error:

2019-10-24T04:11:02.3193805Z @jupyterlab/test-codemirror: Failed to collect coverage from D:\a\1\s\packages\codemirror\src\editor.ts
2019-10-24T04:11:02.3195636Z @jupyterlab/test-codemirror: ERROR: ..\..\packages\codemirror\src\editor.ts: Emit skipped
2019-10-24T04:11:02.3196024Z @jupyterlab/test-codemirror: STACK: TypeError: ..\..\packages\codemirror\src\editor.ts: Emit skipped

I don't know how to interpret these. Maybe changing things from var to let will clear that up, but if not, could you let me know what to do?

The Linux Docs error doesn't seem to be my fault, so I'm leaving it as is.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 25, 2019

The error you saw is being tracked in #6415. Yes, the docs failure is temporary until we release 1.2. Thanks again, and congratulations on your first contribution to JupyterLab!

@blink1073 blink1073 merged commit ac01227 into jupyterlab:master Oct 25, 2019
7 of 9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 25, 2019

@blink1073 - should this be backported to 1.2?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 25, 2019

@meeseeksdev backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Oct 25, 2019
blink1073 added a commit that referenced this issue Oct 25, 2019
…1-on-1.x

Backport PR #7401 on branch 1.x (Fixed Multicursor backspacing)
@benthayer benthayer deleted the backspace branch Oct 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants