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

Add Jupyter action for CodeMirror indentAuto command #3175

Merged
merged 1 commit into from Jan 8, 2018

Conversation

Projects
None yet
3 participants
@gnestor
Copy link
Contributor

gnestor commented Jan 2, 2018

Closes #2591

Adds a shortcut for CodeMirror’s indentAuto command that doesn’t collide with special characters on French/German keyboards (#2535) and also adds it to both the cell editor and text/file editor.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 3, 2018

How confident are we that this shortcut is not already used for anything in some browser? Generally, whenever we add a Ctrl- shortcut, we get complaints that it overrides some other shortcut that people like.

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Jan 3, 2018

Similar sentiment expressed by @Carreau at #2591 (comment):

The typical reasonig for shortcut is the following:

No shortcut should use Alt (or Shift but in a lesser measure) by default. None. Ever.

Many non-english keyboard need Alt and or Shift to type characters. So Shift and Alt are not available.

Typically, french keyboard use Shift for Numbers. So Cmd-Shift-[ is actually "Cmd-5" on a french keyboard... which is taken, and on French keyboard typing [ actually require pressing Alt-Shift-5... so I'm not even sure how that will conflict resolve, or if this could trigger ghost shortcuts.

Hence I would prefer not to do it. What we can (and maybe should) do, is provide a set of "nice but not enabled by default" keymap that are a click away, and provide a "Querty-layout" extra, "Azerty layout extra", "Cyrillic extra", where we can go as crazy as we like.

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Jan 3, 2018

Would it be possible to add some of these "nice to have but not necessary" CodeMirror commands to the notebook actions, allowing people to access indentAuto from the command palette as well as set a custom keyboard shortcut for it? In this instance, it could be as simple as:

// Get selected cell
var selected_cell = notebook.get_selected_cell();
// Execute a CM command
selected_cell.code_mirror.execCommand('indentAuto');
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 4, 2018

I think that makes sense, yep. :-)

@gnestor gnestor force-pushed the gnestor:issue-2591 branch from 563e38c to 1007865 Jan 5, 2018

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Jan 5, 2018

},
indentUnit: 4,
theme: "ipython",
theme: "default",

This comment has been minimized.

@takluyver

takluyver Jan 8, 2018

Member

Not sure why this change is part of this PR?

This comment has been minimized.

@gnestor

gnestor Jan 8, 2018

Author Contributor

Initially, the intent was to make the CodeMirror config (shortcuts, theme, etc.) consistent between code cells in the notebook and the file editor. The code cells use the "default" theme and the editor uses the "ipython" theme. Is this intentional?

This comment has been minimized.

@takluyver

takluyver Jan 8, 2018

Member

I don't know. I don't have any particular objection if you think it's the right thing to do, but I think it would have been better to do it as a separate PR so it could be discussed separately.

Git blame shows @minrk set it to use the IPython theme three years ago in 0efd335 . Min, do you have any recollection whether there was a need for that?

This comment has been minimized.

@minrk

minrk Jan 8, 2018

Member

Oof, that's asking a lot of my foggy memory. Maybe it had to do with how magics were highlighted? Try highlighting some IPython-specific syntax and see if it works the same. If it looks fine, I've no objection.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 8, 2018

@gnestor if you want to get rid of the editor theme change, I think it's fine to merge this PR for 5.3.

@gnestor gnestor force-pushed the gnestor:issue-2591 branch from 1007865 to 630ef63 Jan 8, 2018

@gnestor gnestor modified the milestones: 5.4, 5.3 Jan 8, 2018

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Jan 8, 2018

Ok, I'm moving the syntax stuff to another PR. This is ready to merge 👍

@takluyver takluyver changed the title Add indentAuto shortcut back Add Jupyter action for CodeMirror indentAuto command Jan 8, 2018

@takluyver takluyver merged commit 7b5bd66 into jupyter:master Jan 8, 2018

4 checks passed

codecov/patch Coverage not affected when comparing 27c00bf...630ef63
Details
codecov/project 78.74% remains the same compared to 27c00bf
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 8, 2018

Thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.