-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -396,6 +396,11 @@ define([ | |
* Remove the binding of shortcut `sortcut` with its action. | ||
* throw an error if trying to remove a non-exiting shortcut | ||
**/ | ||
if(!shortcut){ | ||
console.warn('trying to remove empty shortcut'); | ||
return; | ||
|
||
} | ||
shortcut = normalize_shortcut(shortcut); | ||
if( typeof(shortcut) === 'string'){ | ||
shortcut = shortcut.split(','); | ||
|
@@ -404,14 +409,15 @@ define([ | |
* The shortcut error should be explicit here, because it will be | ||
* seen by users. | ||
*/ | ||
try | ||
{ | ||
var that = this; | ||
try { | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shortbut? |
||
throw new Error('trying to remove a non-existent shortcut', shortcut); | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,4 +76,4 @@ | |
"notebookApp['" + modulePath + "']});`"].join(' ')); | ||
return notebookApp[modulePath]; | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,26 @@ define([ | |
this.edit_shortcuts = new keyboard.ShortcutManager(undefined, options.events, this.actions, this.env); | ||
this.edit_shortcuts.add_shortcuts(this.get_default_common_shortcuts()); | ||
this.edit_shortcuts.add_shortcuts(this.get_default_edit_shortcuts()); | ||
|
||
|
||
this.config = options.config; | ||
var that = this; | ||
|
||
this.config.loaded.then(function(){ | ||
|
||
(((that.config.data.keys||{}).edit||{}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is extremely dense, and I can't really follow where the different parentheses start and stop. Maybe expand it a little bit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expanded, but that look a bit ugly for just avoiding typeerror if one of them is undefined:
|
||
.unbind||[]) | ||
.forEach(function(u){that.edit_shortcuts.remove_shortcut(u)}); | ||
(((that.config.data.keys||{}).command||{}) | ||
.unbind||[]) | ||
.forEach(function(u){that.command_shortcuts.remove_shortcut(u)}); | ||
|
||
that.command_shortcuts.add_shortcuts( ((that.config.data.keys||{}).command||{}).bind); | ||
that.edit_shortcuts.add_shortcuts( ((that.config.data.keys||{}).edit ||{}).bind); | ||
|
||
} | ||
); | ||
|
||
Object.seal(this); | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something but it doesn't look like
that
is being used anywhere in this function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, just had an experiment here that I removed.