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

Make keyboard shortcuts declarative #1234

Merged
merged 4 commits into from Mar 24, 2016
Merged

Make keyboard shortcuts declarative #1234

merged 4 commits into from Mar 24, 2016

Conversation

@Carreau
Copy link
Member

@Carreau Carreau commented Mar 20, 2016

look in config for keys.command.unbind, keys.edit.unbind for which shortcuts to unbind,
then keys.command.bind, keys.edit.bind for new shortcut to bind.

Done after feedback at JupyterDays.

Command Palette hover of Shortcut area show the action name as a tooltip so that users know what to bind.

Cleanup Js here and there, and replace (command) in command palette with (command mode) to remove confusion with Cmd keys on Mac.

this._remove_leaf(shortcut, this._shortcuts);
if (!suppress_help_update) {
// update the keyboard shortcuts notebook help
this.events.trigger('rebuild.QuickHelp');
}
} catch (ex) {
console.warn('shortbut', shortcut, '...',this._shortcuts);

This comment has been minimized.

@minrk

minrk Mar 21, 2016
Member

shortbut?


this.config.loaded.then(function(){

(((that.config.data.keys||{}).edit||{})

This comment has been minimized.

@minrk

minrk Mar 21, 2016
Member

This is extremely dense, and I can't really follow where the different parentheses start and stop. Maybe expand it a little bit?

This comment has been minimized.

@Carreau

Carreau Mar 21, 2016
Author Member

expanded, but that look a bit ugly for just avoiding typeerror if one of them is undefined:

 +            try {
 +                edit_unbind = that.config.data.keys.edit.unbind;
 +            } catch (e) {
 +                if (e instanceof TypeError) {
 +                    edit_unbind = [];
 +                } else {
 +                    throw e;
 +                }
 +            }
@minrk minrk added this to the 5.0 milestone Mar 21, 2016
@minrk
Copy link
Member

@minrk minrk commented Mar 21, 2016

A few minor comments, but 👍

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Mar 22, 2016

Is this API compatible?

Sent from my iPhone

On Mar 20, 2016, at 3:58 PM, Matthias Bussonnier notifications@github.com wrote:

look in config for keys.command.unbind, keys.edit.unbind for which shortcuts to unbind,
then keys.command.bind, keys.edit.bind for new shortcut to bind.

Done after feedback at JupyterDays.

Command Palette hover of Shortcut area show the action name as a tooltip so that users know what to bind.

Cleanup Js here and there, and replace (command) in command palette with (command mode) to remove confusion with Cmd keys on Mac.

You can view, comment on, or merge this pull request online at:

#1234

Commit Summary

Make keyboard shortcuts declaratives
File Changes

M notebook/static/base/js/keyboard.js (10)
M notebook/static/deprecated-imports.js (2)
M notebook/static/notebook/js/celltoolbarpresets/attachments.js (3)
M notebook/static/notebook/js/celltoolbarpresets/default.js (1)
M notebook/static/notebook/js/celltoolbarpresets/rawcell.js (1)
M notebook/static/notebook/js/celltoolbarpresets/slideshow.js (1)
M notebook/static/notebook/js/commandpalette.js (2)
M notebook/static/notebook/js/keyboardmanager.js (20)
M notebook/static/notebook/js/main.js (4)
M notebook/static/notebook/js/notebook.js (3)
M notebook/static/notebook/js/quickhelp.js (2)
M notebook/static/notebook/less/commandpalette.less (5)
M notebook/static/services/config.js (4)
Patch Links:

https://github.com/jupyter/notebook/pull/1234.patch
https://github.com/jupyter/notebook/pull/1234.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

@Carreau
Copy link
Member Author

@Carreau Carreau commented Mar 22, 2016

Is this API compatible?

With what ?

@Carreau
Copy link
Member Author

@Carreau Carreau commented Mar 22, 2016

@willingc added a bit of documentation. Can I get your feedback on it ?

@Carreau
Copy link
Member Author

@Carreau Carreau commented Mar 22, 2016

Is this API compatible?
With what ?

More especially: it is purely an addition of a config section. It maps what user were doing in their custom.js with a corresponding config entry (we can bikeshed on the name if you like).

There is 0 API deletion, 0 API addition, 0 API modification. Just the provisional (added in the docs) addition of a config section in the docs.

So I'm not sure what your question of API compatibility means.


DEclarative Custom Keymaps is a provisional feature with unstable API which is not
guarantied to be keep in future versions odf the notebook, and can be
removed or changed without warnings.

This comment has been minimized.

@SamLau95

SamLau95 Mar 22, 2016
Contributor

spelling nits:

DEclarative -> Declarative
guarantied -> guaranteed
be keep -> be kept
odf -> of
@@ -404,8 +408,8 @@ define([
* The shortcut error should be explicit here, because it will be
* seen by users.
*/
try
{
var that = this;

This comment has been minimized.

@SamLau95

SamLau95 Mar 22, 2016
Contributor

I might be missing something but it doesn't look like that is being used anywhere in this function?

This comment has been minimized.

@Carreau

Carreau Mar 22, 2016
Author Member

Oh, yeah, just had an experiment here that I removed.

}
}

edit_unbind.forEach(function(u){that.edit_shortcuts.remove_shortcut(u);});

This comment has been minimized.

@SamLau95

SamLau95 Mar 22, 2016
Contributor

Can this be shorted to:

edit_unbind.forEach(that.edit_shortcuts.remove_shortcut);

?

and for the command_unbind.forEach below.

This comment has been minimized.

@Carreau

Carreau Mar 22, 2016
Author Member

No, I actually tried, and it was calling the prototype in the wrong context, telling me that undefined has no attribute i_dont-remember_what my guess is that forEach will actually .bind(..) the callback to something else.

This comment has been minimized.

@SamLau95

SamLau95 Mar 22, 2016
Contributor

Ah, you're right. I think it's because a function has the scope of the object it was defined on, so by default remove_shortcut has the scope of the edit_shortcuts object.

ES6 gives a very concise way to get around most this problems with arrow functions, but alas, it's not supported by most browsers yet.

This comment has been minimized.

@Carreau

Carreau Mar 22, 2016
Author Member

Yeah, I know. We could think of using es6 transpiler as now we use webpack.... we might be able to use:

edit_unbind.forEach(that.edit_shortcuts.remove_shortcut, that.edit_shortcuts); but I'm not sure if:

  1. it works,
  2. it's understandable by everyone.

So I'll be in favor of something obvious.

@Carreau
Copy link
Member Author

@Carreau Carreau commented Mar 23, 2016

All updated.

@minrk minrk merged commit c940237 into jupyter:master Mar 24, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
minrk added a commit that referenced this pull request Mar 24, 2016
Make keyboard shortcuts declarative
@minrk
Copy link
Member

@minrk minrk commented Mar 24, 2016

Thanks!

@willingc
Copy link
Member

@willingc willingc commented Mar 24, 2016

@Carreau Thanks! I'll give a look at the content now that it's merged. Spent yesterday cleaning up the other notebook stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.0
Merged PRs
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.