update quickhelp on adding and removing shortcuts #5049

ivanov · 2014-02-06T17:53:50Z

I've been knee deep in making shortcuts work for vim mode etc, and have more PRs coming to start some discussion about that, but this one seems like a separate small change worth having.

Right now, we have an API for adding and removing shortcuts, but no visual indication in the UI of which shortcuts are actually active (the quick help you see in the notebook is built only once, and reused there-after).

With this change, we just rebuild the quickhelp dialog on keyboard shortcut changes

ellisonbg · 2014-02-08T18:09:22Z

We will have to think further about how to work this. The ShortcutManager is a very low level bit of code that needs to not depend directly on Notebook/Cell/QuickHelp, etc. The usual way we handle this is by firing an event that the QuickHelp could watch for in ShortcutManager. Can you give that a try?

ellisonbg · 2014-03-01T00:03:47Z

Any plans to address this before 2.0?

QuickHelp is rebuilt on show if the dirty bit was set.

ivanov · 2014-03-09T21:25:54Z

Ok, I've changed this to use an event, as @ellisonbg suggested. The rebuild.QuickHelp event sets the dirty bit on IPython.quick_help, and if that is set on show, the modal dialog gets rebuilt again.

Ready for review.

ivanov · 2014-03-10T18:45:59Z

Ok, I've prettified the keyboard shotrcuts here, and made them consistent with how we refer to keyboard shorcuts in our docs. For example: Ctrl-m not Ctrl+m. I'm only +0.5 to +0.25 on using dashes to combine shortcuts, it's visually quieter and a little easier to parse the constituents of combo shortcuts. On the other hand, one of our shortcuts contains an -. I just want to make sure we're consistent everywhere.

Here's are screenshots of what the two options look like:

We could also combo constituents if that makes better sense.

ivanov · 2014-03-10T18:46:48Z

I realized as soon as I posted this that the enter in press enter to enable needs to be changed, so let me do that as well.

takluyver · 2014-03-10T19:10:01Z

I prefer the -, I think.

ivanov · 2014-03-11T07:50:42Z

Ok, I've made the code more DRY, so the keyboard shortcut style is made in just one place (instead of four). Is this too loud?

damianavila · 2014-03-11T12:52:06Z

Is this too loud?

I don't think so...

takluyver · 2014-03-11T17:24:38Z

The red does look a bit too loud to me.

ellisonbg · 2014-03-11T17:47:16Z

I agree that the red is a bit too much. Also, we use + for shortcuts
rather than -.

On Tue, Mar 11, 2014 at 10:24 AM, Thomas Kluyver
notifications@github.comwrote:

The red does look a bit too loud to me.

Reply to this email directly or view it on GitHubhttps://github.com//pull/5049#issuecomment-37323697
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

takluyver · 2014-03-11T17:51:46Z

@ellisonbg the change to - is part of the change under consideration. Is there a good reason to stick with +? Personally, I think - looks nicer.

ellisonbg · 2014-03-11T17:56:41Z

Originally I picked + because I thought it made more sense when there are multiple modifiers. I mostly want consistency, so if there is consensus that Shift-Ctrl-m looks better let's use that everywhere (in the actual shortcuts and in the user visible docs). I still probably prefer + but only weakly (60/40).

jdfreder · 2014-03-11T18:01:30Z

Ok, I've made the code more DRY, so the keyboard shortcut style is made in just one place (instead of four). Is this too loud?

I really like the grey box and thin borders around it. It reminds me of GitHub's back-tick inline highlighting. Probably unrelated, but I wish back-ticks in NB markdown styled similarly (I have code in my custom CSS to do that, but I think it would be a good default).

damianavila · 2014-03-11T19:10:23Z

I still probably prefer + but only weakly (60/40).

I agree with @ellisonbg here... the + sign has the add meaning on it, and, in fact, you are adding keys when you press them simultaneously... anyway, not a big problem, let's do whatever the consensus says...

minrk · 2014-03-11T23:33:55Z

- seems a bit more legible and a bit more common to me. I don't think anybody is confused by what is communicated there.

Of course, my preference would be to use ^⇧⌥⌘ ⏎ symbols with no delimiter, but I think that's not the way we are going.

ellisonbg · 2014-03-13T22:34:33Z

The style of the keyboard shortcuts in the help dialog and tour should be more subtle. Probably just monospaced text with a border - standard color (not red). Red means warning or error.
The Keyboard shortcuts in the tour are now lowercase (enter/esc).

ellisonbg · 2014-03-13T22:50:13Z

Do we want to allow - as a separator in actual keyboard shortcuts (sc.replace('-','+'))?

ellisonbg · 2014-03-13T22:51:12Z

IPython/html/static/notebook/js/quickhelp.js

@@ -18,6 +18,10 @@ var IPython = (function (IPython) {
    QuickHelp.prototype.show_keyboard_shortcuts = function () {
        // toggles display of keyboard shortcut dialog
        var that = this;
+        if ( this.force_rebuild ) {
+            delete(this.shortcut_dialog);


you verify that this is sufficient to actually delete the DOM element from the page as well.

I'm actually not versed enough on front-end stuff to be able to say anything definitive about that. What would be a way to verify this? Is there some DOM view I don't know about?

The thing to do is look at the DOM in the developer tools to make sure there aren't two quick help DOM elements after a rebuild. If there is, you will probably have to add a call to jQuery's remove method to clean up the dialog. But I think our dialog logic may already handle this.

ellisonbg · 2014-03-13T22:52:35Z

The User Interface notebook will also have to be updated - it uses the same code to build a list of shortcuts in the output of a cell. Let't wait to do that until #5337 is merged though.

jdfreder · 2014-03-13T22:59:40Z

Do we want to allow - as a separator in actual keyboard shortcuts

That would be really awkward with the alt+- combo (alt--). I guess we could have to add an alias for - - minus.

(I didn't mean to close/reopen this issue btw, my finger slipped on my computer's track pad).

do alliterate, aforementioned color alternative alias is amaranth

ivanov · 2014-03-18T18:12:15Z

Ok, I've change the style for <code> to by black, instead of crimson. This doesn't affect anything in our cells, as those were already styled separately, and only applies to <code> tags that are used elsewhere. Now it looks like this:

minrk · 2014-03-19T20:13:02Z

This is looking just about ready. I think the last question to resolve is + vs - for delimiters: docs use - as delimiter, while shortcut API uses +.

Should we

switch the API to use -, and just be consistent everywhere?
let the API support both?
use + in quick-help, because the border/background around keys makes most of the +/- argument readability argument moot.
just leave it

I would lean toward either switching to - in the API or just leaving it as-is. Supporting both delimiters seems like unnecessary complexity.

Further options in the API that avoid any of the issues that come with using a legitimate shortcut key as a delimiter:

use space as a delimiter
use two characters as delimiter (e.g. ||)
use arrays, rather than parsing strings

ivanov · 2014-03-19T23:03:10Z

I'm working on making the API use -.

ivanov · 2014-03-20T05:32:41Z

ok, i think the API now expects and uses - as the delimiter. Getting the alt-- and a standalone - keyboard shortcuts was a bit tricky, but I've got it now, Travis willing.

ivanov · 2014-03-20T09:20:36Z

Travis is green, and there was much rejoicing!

update quickhelp on adding and removing shortcuts

ivanov added 4 commits March 9, 2014 12:24

update notebook quickhelp on new shortcuts

1516c1f

removing keyboard shortcuts should also update help

6a7d9d6

rebuild.QuickHelp event sets the dirty bit

90e006c

QuickHelp is rebuilt on show if the dirty bit was set.

remove recreation of QuickHelp

38a67ad

ivanov added this to the 2.0 milestone Mar 9, 2014

ivanov added 2 commits March 10, 2014 11:13

first pass at capitalizing keyboard shortcuts

d1b4808

prettify combo keyboard shortcuts

f930740

fix capitalization of mode-switching keys

8be479a

DRY: style in one place, removed code duplication

bf507b3

ellisonbg reviewed Mar 13, 2014
View reviewed changes

jdfreder closed this Mar 13, 2014

jdfreder reopened this Mar 13, 2014

ivanov added 2 commits March 18, 2014 10:31

semicolon fixes buttress half of my js commits

6b4d564

don't color code in crimson

f2973ab

do alliterate, aforementioned color alternative alias is amaranth

ivanov added 3 commits March 18, 2014 11:16

remove stale modal dialog from the DOM

497c5f6

capitalize keyboard shortcuts in the tour

66e5cf4

wider style for keyboard shortcuts

7a0eafa

use - for shortcut separators

8be8acc

ivanov added 3 commits March 19, 2014 16:24

handle keyboard shortcuts ending with '-'

fcfd33d

better handling of minus

62cb538

really fix the '-' key shortcuts now

9163c1a

fix MacOS specific shortcuts, thanks @minrk

87372b4

minrk added a commit that referenced this pull request Mar 20, 2014

Merge pull request #5049 from ivanov/quickhelp-update

90ee037

update quickhelp on adding and removing shortcuts

minrk merged commit 90ee037 into ipython:master Mar 20, 2014

minrk deleted the quickhelp-update branch March 20, 2014 21:35

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#5049 from ivanov/quickhelp-update

b279e8b

update quickhelp on adding and removing shortcuts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update quickhelp on adding and removing shortcuts #5049

update quickhelp on adding and removing shortcuts #5049

ivanov commented Feb 6, 2014

ellisonbg commented Feb 8, 2014

ellisonbg commented Mar 1, 2014

ivanov commented Mar 9, 2014

ivanov commented Mar 10, 2014

ivanov commented Mar 10, 2014

takluyver commented Mar 10, 2014

ivanov commented Mar 11, 2014

damianavila commented Mar 11, 2014

takluyver commented Mar 11, 2014

ellisonbg commented Mar 11, 2014

takluyver commented Mar 11, 2014

ellisonbg commented Mar 11, 2014

jdfreder commented Mar 11, 2014

damianavila commented Mar 11, 2014

minrk commented Mar 11, 2014

ellisonbg commented Mar 13, 2014

ellisonbg commented Mar 13, 2014

ellisonbg Mar 13, 2014

ivanov Mar 13, 2014

ellisonbg Mar 14, 2014

ellisonbg commented Mar 13, 2014

jdfreder commented Mar 13, 2014

ivanov commented Mar 18, 2014

minrk commented Mar 19, 2014

ivanov commented Mar 19, 2014

ivanov commented Mar 20, 2014

ivanov commented Mar 20, 2014

update quickhelp on adding and removing shortcuts #5049

update quickhelp on adding and removing shortcuts #5049

Conversation

ivanov commented Feb 6, 2014

ellisonbg commented Feb 8, 2014

ellisonbg commented Mar 1, 2014

ivanov commented Mar 9, 2014

ivanov commented Mar 10, 2014

ivanov commented Mar 10, 2014

takluyver commented Mar 10, 2014

ivanov commented Mar 11, 2014

damianavila commented Mar 11, 2014

takluyver commented Mar 11, 2014

ellisonbg commented Mar 11, 2014

takluyver commented Mar 11, 2014

ellisonbg commented Mar 11, 2014

jdfreder commented Mar 11, 2014

damianavila commented Mar 11, 2014

minrk commented Mar 11, 2014

ellisonbg commented Mar 13, 2014

ellisonbg commented Mar 13, 2014

ellisonbg Mar 13, 2014

Choose a reason for hiding this comment

ivanov Mar 13, 2014

Choose a reason for hiding this comment

ellisonbg Mar 14, 2014

Choose a reason for hiding this comment

ellisonbg commented Mar 13, 2014

jdfreder commented Mar 13, 2014

ivanov commented Mar 18, 2014

minrk commented Mar 19, 2014

ivanov commented Mar 19, 2014

ivanov commented Mar 20, 2014

ivanov commented Mar 20, 2014