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

Modal UI #3605

Merged
merged 57 commits into from Jan 11, 2014
Merged

Modal UI #3605

merged 57 commits into from Jan 11, 2014

Conversation

ellisonbg
Copy link
Member

The goal of this PR is to fix the various focus related problem in the notebook user experience. The issue for this is #3441. The source of the problem is that we used to call this.code_mirror.focus when a cell was selected. This caused all of the jumpy behavior. However, when this call was removed, the entire user experience fell apart. This PR is an attempt to restore a solid user experience while also removing the problematic focus calls.

The result is that the notebook is now a dual mode editor with a command mode and edit mode. Command mode is entered using ESC or Ctrl-m. Thus our old keyboard shortcuts still work, but multiple of them can be given after you type Ctrl-m. Edit mode is entered for the current cell by pressing Enter. This is not ready for merge, but we are thinking about trying to merge this for 1.0. But, it will need lots of testing. The code is relatively simple, but the new interaction model is quite subtle and there are numerous tweaks we could make.

Another side effect is that multiple markdown, rawtext and heading cells can now be unrendered at the same time.

Overall I think this is a massive improvement of the user experience. But, this needs tons of interactive hands on user experience testing.

Remaining issues:

  • Test various notebook and cell actions (cut, copy, paste, merge, split, etc.).
  • Keyboard shortcuts for command mode need to be turned off when the notebook area is not in focus. This is currently preventing dialogs and other things that require keyboard input from working.
  • Test and debug tooltips.
  • Test to make sure that the pager can be closed with ESC.
  • Test and debug tab completion.
  • Possibly refactor the keyboard shortcut code to make it all configurable.

Questions:

  • Do we like how Shift-Enter, Ctrl-Enter and Alt-Enter behave?
  • Should we always enter edit mode when a cell is changed type?
  • Do people like the green border to indicate edit mode?
  • Do we want to keep ESC for entering command mode?

@haard
Copy link

haard commented Jul 11, 2013

Won't some of those changes make e.g. Emacs bindings even harder to implement by locking in a vim-ish model in the implementation? Would C-c C-c be forced to become C-m C-c C-c?

I'd like to see a default keypress handling that could be overloaded with either vim-style dual mode or emacs style bindings, or whatever is cool next year.

@@ -38,6 +38,8 @@ var IPython = (function (IPython) {
this.placeholder = options.placeholder || '';
this.read_only = options.cm_config.readOnly;
this.selected = false;
this.rendered = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation.

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

Had a quick read-through, not enough to understand everything so I would like to have time to look at it more before judging.

I still don't like the if-else block with event number to choose between action, IMHO, we shoudl be able to do that using a dict with value beeing function.

It feel strange to me that switching to command-mode is handeled on a per-cell basis, but as I wrote earlier I want to understand more.

I feel more-and more that this is too big for 1.0, and that we should really discuss dual-mode editting, and give it more than 10-15 day tests.

* @method unselect
* @return is the action being taken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this statement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you actually have "@return is the action being taken" in lots of places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's like an event handler, return true if an action is taken, false if nothing happend

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

And now some usage feedback.

would bind pgup pgdown to +10 cell - 10cell to move fast.

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

Completion is broken. Should not unselect the cell on focus out as completer take focus.

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

Focusing out a cell make it jump to top of screen. Eg I'm editting a cell in the middle of my screen, I reach menu.
It jumps.

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

The different behavior when clicking in cell vs cell-editor (focus and scrolling behavior) is weird and annoying. Fell too jumpy. Impression that I want to do sometime twice (select a cell) and got 2 different behavior.

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

shift enter do not select next cell for execution.

@Carreau
Copy link
Member

Carreau commented Jul 11, 2013

I'll revert to master, is is too hard to edit and tweek a notebook with this branch.
It feel right when using only keyboard at beginning, but as soon as you use the mouse to reach stuff and modify code it becomes annoying.

I also have a lot of shortcut in my muscle memory ('C-m,a' , type import), and a few time this screwed up, interrupting the kernel (i) and making my cell a md cell (m) with port in it... sight.

What about Esc beeing the only way to switch to command mode for good, C-m switching only for 1 action, drop back to edit current selected cell ?

@jakobgager
Copy link
Contributor

If I just move out of a heading cell its not get rendered as heading cells. It needs to be executed.

Seems like Shift+Enter and Ctrl+Enter are exchanged.

@minrk
Copy link
Member

minrk commented Jul 11, 2013

If I just move out of a heading cell its not get rendered as heading cells. It needs to be executed.

I think that's by design

@minrk
Copy link
Member

minrk commented Jul 11, 2013

One comment: I think the cell highlight should be visibly different for edit / command mode

@ivanov
Copy link
Member

ivanov commented Jul 11, 2013

j and k keys MOVE the cells, and there is no way to move the cursor. I suggest making j and k move the focus around, and maybe shift-j and shift-k to move cells around

@ptone
Copy link
Contributor

ptone commented Jul 11, 2013

Some quick usability comments from playing around:

the 'h' key for help needs to be displayed somewhere in the UI - maybe only appearing during command mode.

the help screen should have a link that opens a more complete page documenting usage.

I agree with @ivanov about the J,K - as a vim user, it moves where my cursor is, not the line I'm on

maybe keyboard shotcuts could appear with hover-help on toolbar buttons

some more pronounced visual feedback of what mode is active

Should there be a way to disable dual mode completely for beginners?

@damianavila
Copy link
Member

One weird thing: if I convert as cell to markdown, and later again to a code cell, it seems to lost the grey border of the command mode (we enter the phantom mode...booo... like vim... hehe ;-) @ivanov) . I mean, you can execute it, but if you want to see the border again you have to alt+enter or ctrl+enter.

@ellisonbg
Copy link
Member Author

Can you give the exact keystrokes you used?

Sent from my iPhone

On Jul 11, 2013, at 5:13 PM, Damián Avila notifications@github.com wrote:

One weird thing: if I convert as cell to markdown, and later again to a code cell, it seems to lost the grey border of the command mode (we enter the phantom mode...booo... like vim... hehe ;-) @ivanov) . I mean, you can execute it, but if you want to see the border again you have to alt+enter or ctrl+enter.


Reply to this email directly or view it on GitHub.

@damianavila
Copy link
Member

Can you give the exact keystrokes you used?

  1. Open a new notebook
  2. print "hi" or whatever you want...
  3. Ctrl-m m
  4. Ctrl-m y
  5. Phantom mode ;-)

@takluyver
Copy link
Member

I might be inclined to do that for meta-s as well, since I don't even know where my meta key is ;-). I'm happy for only one shortcut to be documented for each option.

@ellisonbg
Copy link
Member Author

screen shot 2014-01-09 at 4 49 27 pm

@takluyver
Copy link
Member

👍 to that

@ivanov
Copy link
Member

ivanov commented Jan 10, 2014

perhaps instead of h, help keyboard shortcuts key should be ? (shift + /) like in gmail?

}
},
'shift+o' : {
help : 'toggle output',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should read 'toggle output scrollbar' or something like that to distinguish it from 'o' above

@takluyver
Copy link
Member

Split leaves the cursor in the second part of the cell, but we only have a shortcut for join below, not join above, so if you use the shortcuts to split and immediately join, you don't get back to where you started. That's probably not a big problem when you're not testing shortcuts, but it might be an awkward asymmetry to remember. I know we considered whether split should leave the cursor above or below the split - this is a point in favour of above.

}
},
'up' : {
help : 'select previous cell',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"select previous cell (only if on top line, otherwise, go up a line" or something like that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too long, I think. I'd just leave the help strings out, so they're not listed in the shortcuts help - it should be obvious enough to users how moving the cursor works.

@minrk
Copy link
Member

minrk commented Jan 10, 2014

I think @ivanov's suggestion of matching Gmail's '?' shortcut for help makes the most sense now that we don't have a modifier key. But this brings up an issue: while we are using KeyDown, '?' is 'shift+/'; but on French keyboards, it is 'shift+,'. If we use the KeyPress event instead, then it's clear, and the shortcut is just '?' (no shift necessary).

However

Chrome doesn't fire KeyPress for non-printing characters (esc, arrows, etc.), so we can't use KeyPress exclusively. It seems to me that the most logical approach is:

  • KeyPress for regular key shortcuts
  • KeyDown for non-printing keys

But this is obviously more complex than just using KeyDown. Still, I think 'A' as a shortcut makes more sense than 'shift+a', and it would mean that we are not (as) sensitive to locale differences.

It's also possible that we should look into that in a separate PR, rather than here.

@Carreau
Copy link
Member

Carreau commented Jan 10, 2014

Keyboard shortcut in IPython is already a mess on non-english keyboard, it is always like that in webapp anyway.

IMHO, but probably in a subsequent PR, one should be able to express in the shortcut string wether the shortcut is bounded to a key, or a char. smth like ctrl+alt+char:6 (will actually be ctrl+alt+shift+§ on fr_FR) by vs ctr+meta+key:+ omitting the key:|char: woudl have a default behavior.

Eg, french again, restart kernel should be bound to char:. (make sens for consistency with qtconsole), but headers-cells need to be bound to key:1-6 (as number require shift on fr_FR).

@ellisonbg
Copy link
Member Author

OK, I have fixed all outstanding review comments:

  • Minor changes to shortcuts.
  • Use UI rather than UX to describe this in the example notebook.

ellisonbg added a commit that referenced this pull request Jan 11, 2014
Modal UI - a whole new world of fun....its like vim, but not!
@ellisonbg ellisonbg merged commit 5923544 into ipython:master Jan 11, 2014
@sjobeek
Copy link

sjobeek commented Jan 13, 2014

I have some bug reports / feedback on the CodeMirror vim-mode integration with the modal UI. Let me know if I should split this into a separate issue.

  1. Each code cell individually remembers the vim mode (normal or insert) that it was in when it looses focus, creating inconsistency when entering edit mode to edit a cell. Sometimes you start in command mode, sometimes straight to insert mode. Exiting a cell with esc > esc works fine, however executing a code cell (e.g. shift-enter) which was in insert-mode leaves it in insert mode. My suggestion is to have cells always enter vim normal-mode when the notebook activates a cell for "edit" mode
  2. Some weird issues inserting text into a heading cell in vim mode. The normal-mode vim cursor doesn't render properly, and there is some strange insertion / over-writing behavior going on when attempting to modify existing text in insertion mode in a heading cell.
  3. When editing a cell, pressing "up" while on the top line of a cell in while in vim insert mode, the focus will jump to the cell above (and probably force you to vim normal mode), while no cell jumping occurs in vim normal mode. This can strand you in an adjacent cell in an unpredictable vim-mode state. Fixing issue 1. might be enough to make this acceptable as 'normal' behavior, if it was at least predictable.
  4. The clip-board appears to be independent of the vim buffers, meaning you can't "yank" text from vim, then ctrl-v paste it into another window. This wouldn't be too big of a problem (you could just paste vim-style), however other open windows do not seem to share the same buffers. This makes it really annoying trying to copy/paste around different windows or text editors. Not sure what is possible here (synced clipboard / vim yank buffer would be best), but the current state of yank/copy/paste with anything more than a single notebook open is pretty messy.

Other than those issues it seems to be working quite well, though! This interface is a HUGE improvement over the previous one. There's definitely some "vimception" going on with the three levels of command mode -> edit/normal > insert, but it actually works pretty well together.

@ellisonbg
Copy link
Member Author

Thanks for the feedback. Can you open a Github issue and assign it to
ivanov for this stuff?

On Mon, Jan 13, 2014 at 1:36 AM, sjobeek notifications@github.com wrote:

I have some bug reports / feedback on the CodeMirror vim-mode integration
with the modal UI. Let me know if I should split this into a separate issue.

Each code cell individually remembers the vim mode (normal or insert),
creating inconsistency when entering edit mode to edit a cell. Sometimes
you start in command mode, sometimes straight to insert mode. Exiting a
cell with esc > esc works fine, however executing a code cell (e.g.
shift-enter) which was in insert-mode leaves it in insert mode. My
suggestion would to have cells always enter vim normal-mode when the
notebook activates a cell for "edit" mode
2.

Some weird issues inserting text into a heading cell in vim mode. The
normal-mode vim cursor doesn't render properly, and there is some strange
insertion / over-writing behavior going on when attempting to modify
existing text in insertion mode in a heading cell.
3.

When editing a cell, pressing "up" while on the top line of a cell in
while in vim insert mode, the focus will jump to the cell above (and
probably force you to vim normal mode), while no cell jumping occurs in vim
normal mode. This can strand you in an adjacent cell in an unpredictable
vim-mode state.

Other than those issues it seems to be working quite well, though! This
interface is a HUGE improvement over the previous one.


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

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

@sjobeek
Copy link

sjobeek commented Jan 14, 2014

I've opened the issue (#4798), but unfortunately I'm not able to assign it to ivanov.

I'll add any additional feedback to that issue.

@takluyver
Copy link
Member

Done the assignment to @ivanov

@ivanov
Copy link
Member

ivanov commented Jan 14, 2014

looks like someone else did this for you, thanks, i'll take a
look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet