Don't break user keyboard shortcut. #667

Carreau · 2015-10-26T19:25:53Z

Do differentiate copy/cup/delete selected from marked cells.

Bind the keyboard shortcut to the old behavior.

This does not prevent us from rebinding later, but at least make things
explicit.

We cannot in default action sometime act on selected, sometime on marked.
I still think that the marked/selected distinction is wrong, and that default action should at least always act on selection.

Todo:

To respond to @ellisonbg why shoud copy/delete be different than other action :

Right now copy/delete (and cut as cut is copy+delete) are the only actions that have heuristic to sometime act on selection and sometime act on marked cell.

So there are no reason to behave differently, hence they shoudl not have this heuristic.

jdfreder · 2015-10-26T19:44:46Z

I think this is a good idea. If we need to, we could throw a modifier key in front of the x,c,v to target marked cells instead of the selected cell.

Do differentiate copy/cup/delete selected from marked cells. Bind the keyboard shortcut to the **old** behavior. This does not prevent us from rebinding later, but at least make things explicit.

Carreau · 2015-10-26T21:19:27Z

Ok, reworked. Should be ready.

Need @jdfreder hawk eyes.

jdfreder · 2015-10-26T21:29:47Z

notebook/static/notebook/js/actions.js

                env.notebook.select(index);
            }
        },
-        'copy-cell' : {
+        'cut-marked-cell' : {


When talking about marked, it can be plural, so I'd cut-marked-cells

Carreau · 2015-10-26T21:39:27Z

jdfreder · 2015-10-26T21:50:40Z

👀 looking some more

jdfreder · 2015-10-26T21:52:28Z

notebook/static/notebook/js/notebook.js

+            return;
+        }
+        this._delete_cells(indices);
+    }


Semicolon here too, since this is an assignment. When it becomes an ES6 class, we can get rid of the semicolons on the function definitions.

jdfreder · 2015-10-26T21:54:42Z

notebook/static/notebook/js/notebook.js

+
+    Notebook.prototype._delete_cells = function(indices) {
+        if(indices === undefined){
+            throw new Error('need indices to delete cells');


Curious, why'd you split delete_cells from _delete_cells?

The private one should always raise if no indices are given. The public one can do heuristics, that reduce the number of potential bugs.

jdfreder · 2015-10-26T21:56:30Z

ok, done looking

jdfreder · 2015-10-26T21:59:10Z

is this ready to go?

Carreau · 2015-10-26T21:59:22Z

Pushed fixes. Travis is slow.

Carreau · 2015-10-26T22:00:15Z

I've killed a few builds on old commits.

Carreau · 2015-10-26T22:01:11Z

Also I'd like @ellisonbg to not give a -1 before merging.

jdfreder · 2015-10-26T22:01:14Z

In the meantime I'll check it out and give it a test run.

jdfreder · 2015-10-26T22:29:04Z

Seems to work as advertised!

ellisonbg · 2015-10-27T01:25:47Z

I have been using cell marking for the last week or so to prepare talks,
etc. The current behavior of using (selected or marked) is extremely
intuitive, efficient and matches the semantics of other UIs that support
multiple selection.

I think if we allow multiple selection in any form, but don't bind the
standard keyboard shortcuts to work both with selected and marked, it will
be horrifically confusing.

So, yes, initially -1 - but you knew that was coming :)

I am open to a number of changes that will clarify the selected/marked idea:

An audit of which actions work with marked+selected versus only selected
A clear, unified API for actions that work with marked+selected versus
only selected
Anything else that will clarify the different abstractions while
providing a clear user experience.

This is not just about cut/copy/paste. Many other actions apply to multiple
cells in a conceptually clean way (merging, running, hide/clear
input/output). Are we going to have two keyboard shortcuts for all of
those? That sounds super crazy and a waste of precious keyboard shortcuts.

On Mon, Oct 26, 2015 at 3:25 PM, Matthias Bussonnier <
notifications@github.com> wrote:

Do differentiate copy/cup/delete selected from marked cells.

Bind the keyboard shortcut to the old behavior.

This does not prevent us from rebinding later, but at least make things
explicit.

Ping @ellisonbg https://github.com/ellisonbg, @jhamrick
https://github.com/jhamrick , @jdfreder https://github.com/jdfreder

We cannot in default action sometime act on selected, sometime on marked.
I still think that the marked/selected distinction is wrong, and that
default action should at least always act on selection.

Todo:

test more

check for cut.

To respond to @ellisonbg https://github.com/ellisonbg why shoud
copy/delete be different than other action :

Right now copy/delete (and cut as cut is copy+delete) are the only
actions that have heuristic to sometime act on selection and sometime act
on marked cell.

So there are no reason to behave differently, hence they shoudl not have

this heuristic.

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

#667
Commit Summary

Don't break user keyboard shortcut.

File Changes

M notebook/static/base/js/keyboard.js
https://github.com/jupyter/notebook/pull/667/files#diff-0 (2)

M notebook/static/notebook/js/actions.js
https://github.com/jupyter/notebook/pull/667/files#diff-1 (23)

M notebook/static/notebook/js/keyboardmanager.js
https://github.com/jupyter/notebook/pull/667/files#diff-2 (4)

M notebook/static/notebook/js/maintoolbar.js
https://github.com/jupyter/notebook/pull/667/files#diff-3 (2)

M notebook/static/notebook/js/menubar.js
https://github.com/jupyter/notebook/pull/667/files#diff-4 (4)

M notebook/static/notebook/js/notebook.js
https://github.com/jupyter/notebook/pull/667/files#diff-5 (53)

M notebook/static/notebook/js/toolbar.js
https://github.com/jupyter/notebook/pull/667/files#diff-6 (3)

Patch Links:

https://github.com/jupyter/notebook/pull/667.patch

https://github.com/jupyter/notebook/pull/667.diff

—
Reply to this email directly or view it on GitHub
#667.

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Carreau · 2015-10-27T01:38:59Z

I agree with your point that it is more powerful,

Though we cannot change keyboard shortcut in between minor releases.
Like we cannot change notebook style.

The changing keyboard shortcut is more annoying as a sequence of shortcut in 4.1 can lead to a different state than 4.0. (Merge leave cells in a marked state which influence later actions).

4.0, select, split, up d,d delete first cell
4.1, select, split, up d,d delete second cell

So user that have taken habits for the last 3 month with in a couple of minutes need to rewire their brain, and might not even be aware of that change. Especially since the actions that are affected are destructive (delete).

I can see middle grounds though:

Ensure that after all actions the cells get unmarked, including split, and merge, so common workflow like split or merge, then do something else (delete) will not change.

or,

Make sure that actions act on the union of (marked + selected cell)

Side note, I can update this PR so that default actions are still the same, but still get the API granularity of selected vs marked cell. Would that be more acceptable ?

To rephrase where I stand, I am not against the new behavior. I am against changing the behavior of keyboard shortcuts in between 4.0 and 4.1.

ellisonbg · 2015-10-27T02:05:49Z

Ahh, thanks for the clarification.

I agree with you that actions that don't involve marked cells should not
change in any way in a minor release. Also, there are some actions that I
don't think make sense on marked cells (such as split).

Would you consider it a keyboard shortcut breakage if all previous keyboard
shortcuts work exactly the same if there is only a selected cell, but have
new behavior if there are marked cells?

On Mon, Oct 26, 2015 at 9:39 PM, Matthias Bussonnier <
notifications@github.com> wrote:

I agree with your point that it is more powerful,

Though we cannot change keyboard shortcut in between minor releases.
Like we cannot change notebook style.

The changing keyboard shortcut is more annoying as a sequence of shortcut
in 4.1 can lead to a different state than 4.0. (Merge leave cells in a
marked state which influence later actions).

4.0, select, split, up d,d delete first cell

4.1, select, split, up d,d delete second cell

So user that have taken habits for the last 3 month with in a couple of
minutes need to rewire their brain, and might not even be aware of that
change. Especially since the actions that are affected are destructive
(delete).

I can see middle grounds though:

Ensure that after all actions the cells get unmarked, including
split, and merge, so common workflow like split or merge, then do something
else (delete) will not change.

or,

Make sure that actions act on the union of (marked + selected cell)

Side note, I can update this PR so that default actions are still the
same, but still get the API granularity of selected vs marked cell. Would
that be more acceptable ?

To rephrase where I stand, I am not against the new behavior. I am against
changing the behavior of keyboard shortcuts in between 4.0 and 4.1.

—
Reply to this email directly or view it on GitHub
#667 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Carreau · 2015-10-27T02:12:01Z

Would you consider it a keyboard shortcut breakage if all previous keyboard
shortcuts work exactly the same if there is only a selected cell, but have
new behavior if there are marked cells?

Depends on which behavior, I can think of a few that would be ok. But probably not if you look at my other comment where this change in behavior is surprising and does unexpected actions.

I would be ok for example for DD to refuse deleting if there where marked cells, but the selected cell was not one of the marked cell.

Let's discuss that tomorrow.

jdfreder · 2015-10-27T14:47:08Z

Though we cannot change keyboard shortcut in between minor releases.
Like we cannot change notebook style.

I wouldn't be so black and white about this, especially if it means sacrificing the user experience.

Would you consider it a keyboard shortcut breakage if all previous keyboard shortcuts work exactly the same if there is only a selected cell, but have new behavior if there are marked cells?

I think this is the best way forward, even if it means the shortcuts don't work as normal when things are marked. I think that's okay, because marked cells are a new context that hasn't existed pre 4.1.

I would be ok for example for DD to refuse deleting if there where marked cells, but the selected cell was not one of the marked cell.

I think the simpler, more intuitive behaviour would be for DD to delete the marked cells if they exist, and to otherwise delete the selected cell.

Carreau · 2015-10-27T15:14:07Z

I think the simpler, more intuitive behaviour would be for DD to delete the marked cells if they exist, and to otherwise delete the selected cell.

You completely miss the point, that what they are doing now, and what cause the issue.

Question A: What does DD does if the first cell of the notebook is selected ?

Question B: I have the following notebook:

How do I delete the selected cell ?

I wouldn't be so black and white about this, especially if it means sacrificing the user experience.

No if is not destructive, but here the same action on 4.0 and 4.1 can delete work.

jdfreder · 2015-10-27T15:18:51Z

I must still be missing the point,

Question A: What does DD does if the first cell of the notebook is selected ?

Deletes the first cell

Question B: I have the following notebook:

Deletes the selected cell, unless there are marked cells somewhere on the page, even if out of view.

jdfreder · 2015-10-27T15:28:36Z

Other than shift up/down (maybe we should remove this), it doesn't seem easy to accidentally mark cells. Therefor, I don't know why it is a surprise that DD acts on the marked cells and not the selected.

I would be ok for example for DD to refuse deleting if there where marked cells, but the selected cell was not one of the marked cell.

I could see this being a good compromise for now, and revisiting for 5.0

jhamrick · 2015-10-27T16:28:38Z

@jdfreder The issue is that users have been using d,d to delete cells up until now, and if they encounter a case where there is a marked cell that is different from the selected cell, users are going to be surprised. Personally,

Deletes the selected cell, unless there are marked cells somewhere on the page, even if out of view.

is very unintuitive to me. The cells that are being operated on are the ones I'm looking at. Since the few follows the selection around (not the marked cells), my first assumption would be that things like deleting cells operates on the one that I have selected, especially if the marked cells aren't even visible.

To address the issue for now, I think the following two things will be sufficient:

Always unmark cells after an operation (e.g. merge, split). Users aren't going to know how to unmark cells themselves (some will figure it out, many probably won't) so we shouldn't put them in a situation that will be confusing for them in the first place.
(@Carreau 's suggestion) If, in the chance there is a marked cell somewhere, make sure that surprising behavior can't happen. Any destructive operations (merge, split, delete, etc.) should be disabled if the selected cell is not also a marked cell.

Carreau · 2015-10-27T16:45:41Z

Question A: What does DD does if the first cell of the notebook is selected ?
Deletes the first cell

Wrong, delete the first unless something else is marked. Now you need to explain what marked is.

Question B:

You missed the question. A user have the following notebook and ask you what to do to delete.
What is your response.

If you are teaching with the notebook to 40 people you need to look after, and have to scroll to 10 page to know wether DD will delete the selected cell or not, then there is a UI problem.

If a user ask you how to delete a cell and you have to go through 1 3 page explanation on how to mark/unmark cell and why 2 user that have the same thing on screen have the same click on the same menu do 2 different things you have a UI problem.

You cannot in a UI rely on hidden state. If the visible state is out of the view it is hidden state.

jdfreder · 2015-10-27T16:57:11Z

I see where you guys are coming from, I agree, we should change the behavior before the release.

@ellisonbg I'm starting to think that, fundamentally, decoupling the ability to mark from selection (DOM element focus) was not the right idea. Maybe we should have decoupled it only for command mode - because command mode shouldn't need elements on the page to be focused (user isn't inputting into CodeMirror). This would mean no need for marks, and everything could be multi-select (contiguous or not). It seems like the root of the confusion is the fact that there are now 2 modes of selection.

How does GMail, and other applications like it, handle this?

Carreau · 2015-10-27T17:29:38Z

How does GMail, and other applications like it, handle this?

The Gmail UI changes when you "mark" a message.
And you have to mark a message to act on it.

jhamrick · 2015-10-27T17:32:09Z

In gmail, if you're in the list view I believe you have to mark messages (check them) before you can do anything. So that would be equivalent to saying you can't do anything with the selection, but have to first mark any cells you want to mark. However, I don't know if we should follow that model because (1) in gmail they use checkboxes, which makes it really obvious what is selected and what isn't, (2) you are limited in the number of emails displayed at a time, while we do not limit the number of cells, so having things be off-screen is less of a problem in gmail, and (3) users are used to having the selection operate on cells, and modifying that would be a fairly major change.

Carreau · 2015-10-27T17:32:47Z

. Maybe we should have decoupled it only for command mode - because command mode shouldn't need elements on the page to be focused (user isn't inputting into CodeMirror). This would mean no need for marks, and everything could be multi-select (contiguous or not). It seems like the root of the confusion is the fact that there are now 2 modes of selection.

That wouldn't change a lot, you still have this difference between the "cursor" cell and the "marked" cell. it's always confusing as to know wether the actions affect marked or cursor.

My take on that would be that all actions by default should affect union(marked, cursor) moreover it give a meaning to actions like "merge" where the cell would be inserted at the cursor posiition.

jdfreder · 2015-10-27T17:34:58Z

My take on that would be that all actions by default should affect union(marked, cursor) moreover it give a meaning to actions like "merge" where the cell would be inserted at the cursor posiition.

I think like @jhamrick mentioned in #669 , if we make them look the same, union(marked, cursor) would make sense. Do we agree that this is worth trying? I am +1

jhamrick · 2015-10-27T17:36:36Z

👍 on operating on union(marked, cursor), and styling the cursor so that it looks like it's a marked cell (so that really union(marked, cursor) == marked because the cursor looks like it is marked).

minrk · 2015-10-28T09:42:06Z

👍 as well, good summary, @jhamrick.

coveralls · 2017-08-22T09:51:07Z

Changes Unknown when pulling fe5d9fb on Carreau:actions-on-selected into ** on jupyter:master**.

coveralls · 2017-08-22T10:44:10Z

Changes Unknown when pulling fe5d9fb on Carreau:actions-on-selected into ** on jupyter:master**.

takluyver · 2017-08-22T10:46:17Z

I have updated Coveralls' settings to disable the comments again.

Don't break user keyboard shortcut.

59a1ddd

Do differentiate copy/cup/delete selected from marked cells. Bind the keyboard shortcut to the **old** behavior. This does not prevent us from rebinding later, but at least make things explicit.

Carreau force-pushed the actions-on-selected branch from bd36e29 to 59a1ddd Compare October 26, 2015 20:54

Carreau added 3 commits October 26, 2015 13:55

remove non-shim

d60d725

use private function

12cde3e

extra fixes

602a5f9

jdfreder reviewed Oct 26, 2015
View reviewed changes

Carreau added 2 commits October 26, 2015 14:34

pluralize marked cell**s**

eea45bf

semicolon; semiconlon everywhere

91f0d37

merked -> marked

2d45369

jdfreder reviewed Oct 26, 2015
View reviewed changes

plurals

4373453

Carreau force-pushed the actions-on-selected branch from b7a0a37 to 4373453 Compare October 26, 2015 21:53

moar semicolons

fe5d9fb

jdfreder reviewed Oct 26, 2015
View reviewed changes

minrk added this to the 4.1 milestone Oct 27, 2015

jdfreder mentioned this pull request Oct 27, 2015

UI prototype to show hidden state #669

Closed

Carreau closed this Oct 27, 2015

jhamrick mentioned this pull request Oct 28, 2015

Marked cells styling #675

Merged

github-actions bot added the status:resolved-locked label Apr 5, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2021

Don't break user keyboard shortcut. #667

Don't break user keyboard shortcut. #667

Uh oh!

Conversation

Carreau commented Oct 26, 2015

Uh oh!

jdfreder commented Oct 26, 2015

Uh oh!

Carreau commented Oct 26, 2015

Uh oh!

jdfreder Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

Carreau commented Oct 26, 2015

Uh oh!

jdfreder commented Oct 26, 2015

Uh oh!

jdfreder Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

jdfreder Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

Carreau Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

jdfreder Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

jdfreder commented Oct 26, 2015

Uh oh!

jdfreder commented Oct 26, 2015

Uh oh!

Carreau commented Oct 26, 2015

Uh oh!

Carreau commented Oct 26, 2015

Uh oh!

Carreau commented Oct 26, 2015

Uh oh!

jdfreder commented Oct 26, 2015

Uh oh!

jdfreder commented Oct 26, 2015

Uh oh!

ellisonbg commented Oct 27, 2015

this heuristic.

Uh oh!

Carreau commented Oct 27, 2015

Uh oh!

ellisonbg commented Oct 27, 2015

Uh oh!

Carreau commented Oct 27, 2015

Uh oh!

jdfreder commented Oct 27, 2015

Uh oh!

Carreau commented Oct 27, 2015

Uh oh!

jdfreder commented Oct 27, 2015

Uh oh!

jdfreder commented Oct 27, 2015

Uh oh!

jhamrick commented Oct 27, 2015

Uh oh!

Carreau commented Oct 27, 2015

Uh oh!

jdfreder commented Oct 27, 2015

Uh oh!

Carreau commented Oct 27, 2015

Uh oh!

jhamrick commented Oct 27, 2015

Uh oh!

Carreau commented Oct 27, 2015

Uh oh!

jdfreder commented Oct 27, 2015

Uh oh!

jhamrick commented Oct 27, 2015

Uh oh!

minrk commented Oct 28, 2015

Uh oh!

coveralls commented Aug 22, 2017

Uh oh!

coveralls commented Aug 22, 2017 •

edited

Loading