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

[WIP] Prototype to fix Shift-J/K selection with anchor and cursor #708

Merged
merged 1 commit into from Dec 4, 2015

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Nov 5, 2015

related to #707

Behold my unprecedented design skills.
Anchor is cyan, and cell in between anchor and cursor have purple border, just for demo purpose and visually track bugs.

soft_selected

@Carreau
Copy link
Member Author

Carreau commented Nov 5, 2015

ping @ellisonbg , @jdfreder

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

Now with ability to toggle the marking on the multiple selection, with +/- bound to add marking to cell, remove marking from cell. thus when many cells are selected with different marking, it's easy to put them all in a state, while toggling is weird.

many marked

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

Ok, styling is a bit better, I did 1 random user testing.

First key thought for:

  • Toggling mark state : tilde and bang. (cells.marked ^true)
  • mark cells (cell.mark=true) : star
  • unmark cell (cell.mark = false) : backspace.

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

screen shot 2015-11-05 at 22 19 10

Playing a little bit more with style. I't possible to make some nice visual change, that are more visible than change in thickness. Also wit the arrow it becomes natural to "mark" with the right arrow-key, and to un-mark with the left.

@jdfreder
Copy link
Contributor

jdfreder commented Nov 6, 2015

I love the little arrows, and if it also means it solves our hotkey problems by making right/left arrow keys the natural winner, +1

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

3 useful set of keybindings:

var c = IPython.keyboard_manager.command_shortcuts;
// A
c.add_shortcut('Ctrl-space','jupyter-notebook:toggle-cell-marked')


// B
c.add_shortcut('minus','jupyter-notebook:unmark-selected-cells')
c.add_shortcut('+','jupyter-notebook:mark-selected-cells')


// C
c.add_shortcut('left','jupyter-notebook:unmark-selected-cells')
c.add_shortcut('Shift-left','jupyter-notebook:unmark-selected-cells')
c.add_shortcut('right','jupyter-notebook:mark-selected-cells')
c.add_shortcut('shift-right','jupyter-notebook:mark-selected-cells')

I don't like A.

@ellisonbg
Copy link
Contributor

I don't understand the motovation for this PR. Master currently has a very clean set of abstractions that encode the idea of a cursor or active cell (selected) and a set of cells that are the target of an action (marked). These two abstractions are needed because of how selected is inextricably tied to the browsers notion of focus. The only issue we have right now in master is the awkward functioning of the shift-j/k keyboard shortcuts and the extend_marked method those use. Let's just fix that.

This PR introduces a third abstraction (soft selected) that conflates and confuses all of this in a really awkward way. And the changes are not just implementation details that fix the shift-j/k shortcuts. It mixes it with new concepts and interactions in the UI that are overly complex.

From the beginning of when @jdfreder and I started to talk about the marked cell concept, I was aware we would have to think carefully about how shift-j/k work. My design of the marked cells included background threads thinking about it.

I see two ways of fixing it:

  • Remove the shift-j/k shortcuts and just support a Gmail style multiple style multiple selection. It is dead simple, conceptually clear and trivial to implement. It is also efficient if we pick a good shortcut for toggling marked.
  • Keep shift-j/k but add a simple state machine to the extend_marked methods that gives the canonical behavior. I think we could get most of the way there by simply looking at the marked state of the current cell before extending and using that information. We may need a notion of an anchor that these methods manage, but I am not at all convinced of that. But even if we do that should be an internal detail of these methods that doesn't propagate to the rest of the API or the user.

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

I honestly do not know how to do shift-J/K with a state machine correctly.

And again, if it's a state machine I do not know how we will describe to user how it works.

And personally I see a leakage of abstraction between marked state and selected state. Which is better now that the selected cell is implicitly marked.

This is my attempt at fixing it. In all normal user UI, not only would Shift-J/K/Up/down work with anchor/cursor, but once you have a selection moving the cursor deselect all. Which is not what the notebook does. Which we try to work around with indicators of the number of marked cell.

For me the marked marked state is useful but is the complicated part of the API. It is the state-full part that need the user to keep in it's mind.

In my opinion, there are many more user just need contiguous select than user that need marked cells.

Anyway, this is a PR to see what can be done, I'm ok for it to not be merged,
but after playing with it for a day, I like it much more that the current branch than master. I even doubt of the usefulness of non-contiguous selection.

@jdfreder
Copy link
Contributor

jdfreder commented Nov 6, 2015

// C
c.add_shortcut('left','jupyter-notebook:unmark-selected-cells')
c.add_shortcut('Shift-left','jupyter-notebook:unmark-selected-cells')
c.add_shortcut('right','jupyter-notebook:mark-selected-cells')
c.add_shortcut('shift-right','jupyter-notebook:mark-selected-cells')

I definitely prefer this one.

@jdfreder
Copy link
Contributor

jdfreder commented Nov 6, 2015

Remove the shift-j/k shortcuts and just support a Gmail style multiple style multiple selection. It is dead simple, conceptually clear and trivial to implement. It is also efficient if we pick a good shortcut for toggling marked.

This is probably the best idea for now, since it seems there is a general disagreement on the behavior and falling back to providing neither is simple.

Although @Carreau I still like the arrows and the left/right kb shortcuts instead of toggle. Also, implementation details aside, the head/anchor notion of selection is definitely the most intuitive for the unexpecting user.

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

Thanks @jdfreder.

Another option would be for j,k,up,down and other "just-select* action to by default to unmark all. Not with shift of course. I think that would be less surprising.

@jhamrick
Copy link
Member

jhamrick commented Nov 6, 2015

Just want to point out that if you remove the shift-j/k shortcuts and only have toggling for marking cells, merging two cells becomes really onerous:

[mark] j [mark] shift-m

as opposed to

shift-j shift-m

which is already two keystrokes more than it used to be before multi-selection was added.

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2015

[mark] j [mark] shift-m

Note that you can leave the second [mark].

@Carreau
Copy link
Member Author

Carreau commented Nov 9, 2015

one things to fix:

The css make the tooltip disapear.

I'm worked a bit with both master and this branch this week end.
The behavior of cell marking it often annoying.

ON master, I've been caught off guard many time by extra-cell being marked, leading to cell deletion and execution of things I was not expecting.

I'me starting to lean toward -1 on multiple marking with the keyboard, I'm find with mouse+click to multiple-select, but I think any keyboard action should (after the action) reset the marked cell to all-unmarked (except Shift-J/K of course)

@ellisonbg
Copy link
Contributor

I am not sure I understand the issue, can you clarify?

Sent from my iPhone

On Nov 8, 2015, at 11:03 PM, Matthias Bussonnier notifications@github.com wrote:

one things to fix:

The css make the tooltip disapear.

I'm worked a bit with both master and this branch this week end.
The behavior of cell marking it often annoying.

ON master, I've been caught off guard many time by extra-cell being marked, leading to cell deletion and execution of things I was not expecting.

I'me starting to lean toward -1 on multiple marking with the keyboard, I'm find with mouse+click to multiple-select, but I think any keyboard action should (after the action) reset the marked cell to all-unmarked (except Shift-J/K of course)


Reply to this email directly or view it on GitHub.

@ellisonbg
Copy link
Contributor

I will say this - I do wonder if we should remove marking after any
action is performed. In the current state of master, I sometime do things
(with both continguous and non-contiguous markings) that feel like it is
weird to leave the marking after the action.

On Mon, Nov 9, 2015 at 11:34 AM, Brian Granger ellisonbg@gmail.com wrote:

I am not sure I understand the issue, can you clarify?

Sent from my iPhone

On Nov 8, 2015, at 11:03 PM, Matthias Bussonnier notifications@github.com
wrote:

one things to fix:

The css make the tooltip disapear.

I'm worked a bit with both master and this branch this week end.
The behavior of cell marking it often annoying.

ON master, I've been caught off guard many time by extra-cell being
marked, leading to cell deletion and execution of things I was not
expecting.

I'me starting to lean toward -1 on multiple marking with the keyboard,
I'm find with mouse+click to multiple-select, but I think any keyboard
action should (after the action) reset the marked cell to all-unmarked
(except Shift-J/K of course)


Reply to this email directly or view it on GitHub
#708 (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

@jdfreder
Copy link
Contributor

jdfreder commented Nov 9, 2015

Probably doesn't make sense to unmark on copy.

@Carreau
Copy link
Member Author

Carreau commented Nov 9, 2015

I will say this - I do wonder if we should remove marking after any
action is performed. In the current state of master, I sometime do things
(with both contiguous and non-contiguous markings) that feel like it is
weird to leave the marking after the action.

I'm +1 on this.

Probably doesn't make sense to unmark on copy.

I don't see why it doesn't.

I am not sure I understand the issue, can you clarify?

Select a bunch of cell, merge them, J,J,J, DD : oops, deleted the merged cells.

Split Cell, Down, Shift enter : oops , also executed cd..

@jdfreder
Copy link
Contributor

jdfreder commented Nov 9, 2015

I don't see why it doesn't.

Consistency with other programs, because copy doesn't affect selection / mark state in any program I can think of.

@Carreau
Copy link
Member Author

Carreau commented Nov 9, 2015

Consistency with other programs, because copy doesn't affect selection / mark state in any program I can think of.

Fair enough,

Then do you agree that J/K alone should deselect all like in any other programs ?

@jdfreder
Copy link
Contributor

jdfreder commented Nov 9, 2015

Then do you agree that J/K alone should deselect all like in any other programs ?

Definitely!

hmmm, good point.

@ellisonbg
Copy link
Contributor

Um no, then how do you do discontiguous selections?

On Mon, Nov 9, 2015 at 3:10 PM, Jonathan Frederic notifications@github.com
wrote:

Then do you agree that J/K alone should deselect all like in any other
programs ?

Definitely!


Reply to this email directly or view it on GitHub
#708 (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
Copy link
Member Author

Carreau commented Nov 9, 2015

With the mouse.

That's how most of the UIs of desktop browser works.

Or that the point of my other PR with multiple selection and marking, where marking is an rare action that you do when you really need discontinuous selection,
which frankly is rare.

Also the discontinuous selection have weird semantics, like if you mark every other cell and merge, where is the merged cell inserted ?

@ellisonbg
Copy link
Contributor

I don't understand.

Everyday, for hours and hours each day, I use a UI (Gmail) that only has
discontiguous multiple selection driven by a keyboard (j/k/x). It is one of
the most productive user interactions of any software system I have ever
used. I am willing to think through where our current design has problems,
but I am not willing to throw the baby out with the bath water.

I agree that some actions should not work on discontiguous selections -
that is why we have methods on the Notebook class for querying if a set of
cells is contiguous or not. Actions that should only apply to contiguous
selections should use these methods to test the current selection.

On Mon, Nov 9, 2015 at 5:19 PM, Matthias Bussonnier <
notifications@github.com> wrote:

With the mouse.

That's how most of the UIs of desktop browser works.

Or that the point of my other PR with multiple selection and marking,
where marking is an rare action that you do when you really need
discontinuous selection,
which frankly is rare.

Also the discontinuous selection have weird semantics, like if you mark
every other cell and merge, where is the merged cell inserted.


Reply to this email directly or view it on GitHub
#708 (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
Copy link
Member Author

Carreau commented Nov 10, 2015

Everyday, for hours and hours each day, I use a UI (Gmail) that only has
discontiguous multiple selection driven by a keyboard (j/k/x)

this UI is disabled by default You have to enable it in your preferences.

It is one of
the most productive user interactions of any software system I have ever
used.

I dont' say that marking is bad in general I completely understand the usage, though the manipulation you make in gmail are quite different from what you do in a notebook. Like in Gmail you cannot merge mails.
And each thread is roughly unrelated to each other :non-contiguous selection make sens. The probability you select thread N+1 is uncorrelated to Thread N. It is not the case in the notebook.

j/x sequences to select N cells is twice the number of keystrokes that you need if using Shift-J, so I believe multiple selection is more useful that non-contiguous marking.

I do understand the need of discontinuous marking, and I do not dismiss it's utility.
I completely agree that we need an API for that. What I disagree with is the semantics on how it's applied. and I believe that the way it's implemented right now is incompatible with a non confusing UI/keyborad shortcut.

I think that one of the possible solution is to have multiple selection.

I agree that some actions should not work on discontiguous selections -
that is why we have methods on the Notebook class for querying if a set of
cells is contiguous or not. Actions that should only apply to contiguous
selections should use these methods to test the current selection.

I'm worried of the complexity involved here, and I would like to to reach 90% of users with 10% of the efforts. One of the reason I liked to videochat with you is to clarify the meaning of words selection/marking/soft_selection and so on and so forth, as most of the discussion are impaired by us using different term for same things, and same terms for different things.

So let's try to clarify what action we want the user to be able to do, scenario that we think should not happend/are confusing (regardless of implementation) and look at various possible implementation. I'll open a google docs for that.

@Carreau
Copy link
Member Author

Carreau commented Nov 10, 2015

I'm trying to list problematic use case here: https://ipython.hackpad.com/Multiple-selection-W7KzQxYqMZp

@Carreau
Copy link
Member Author

Carreau commented Nov 18, 2015

ping @fperez

@Carreau
Copy link
Member Author

Carreau commented Nov 20, 2015

Some updates after discussion with @fperez on tuesday.

Shift-click now select a range, (I had to unbind on('focus') and keep relying on onClick which I'm not super happy about.

@Carreau Carreau added this to the 4.1 milestone Nov 20, 2015
@ellisonbg
Copy link
Contributor

Here is a prototype of the visual design from @cameronoelsen

screen shot 2015-11-20 at 2 03 33 pm

Adds MTD shadow on the selected cell, 3px wide multi-cell indicator.

@Carreau
Copy link
Member Author

Carreau commented Dec 5, 2015

Wooohoooo, sorry, was busy for the last 2 days, will catch up this week-end !

@fperez
Copy link
Member

fperez commented Dec 6, 2015

wow, hadn't seen this had been merged, +1000 for this going in. Thanks everyone for all the hard work that went into making it happen! And cheers to that awesome gif ;)

@fperez
Copy link
Member

fperez commented Dec 13, 2015

Hey all, I have a quick question... I know this is already merged, but the behavior I'm seeing on selection looks like this:

image

which has no highlight at all, and is in fact quite different from where the discussion seemed to have converged. I'm running off master right now (commit caa23d0). Did things change post merge or did I miss anything elsewhere? It seemed we'd agreed that some kind of highlight was indeed what we wanted...

@ellisonbg
Copy link
Contributor

Hmmm, that is the old behavior. You are either running 4.0.x stable or need
to refresh your browser cache to get the new stuff.

On Sun, Dec 13, 2015 at 11:38 AM, Fernando Perez notifications@github.com
wrote:

Hey all, I have a quick question... I know this is already merged, but the
behavior I'm seeing on selection looks like this:

[image: image]
https://cloud.githubusercontent.com/assets/57394/11768829/b6168358-a18d-11e5-8ec0-d63e9bf85fe2.png

which has no highlight at all, and is in fact quite different from where
the discussion seemed to have converged. I'm running off master right now
(commit caa23d0
caa23d0).
Did things change post merge or did I miss anything elsewhere? It seemed
we'd agreed that some kind of highlight was indeed what we wanted...


Reply to this email directly or view it on GitHub
#708 (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
Copy link
Member Author

Carreau commented Dec 13, 2015

The most obvious reason would be that import notebook git the right version but jupyter-notebook executable has an hardcoded path to a python-executable that have 4.0.6

@fperez
Copy link
Member

fperez commented Dec 13, 2015

Very odd...

denali[~]> jupyter notebook --version
4.1.0.b1

and that screenshot was after multiple cache dumps and page reloads.

But OK, if you guys aren't seeing that, then I'll keep digging. As long as it's just me, carry on...

ps - Brian, I'll ping you later today :)

@Carreau
Copy link
Member Author

Carreau commented Dec 13, 2015

Very odd...

indeed... we should improve --debug to log the on disk path of files that are served.

@fperez
Copy link
Member

fperez commented Dec 14, 2015

Ah, Brian helped me... JS newbie here... npm run build missing :)

@Carreau
Copy link
Member Author

Carreau commented Dec 14, 2015

@fperez I would suggest you c.NotebookApp.ignore_minified_js=True in your py/json config file.

@fperez
Copy link
Member

fperez commented Dec 14, 2015

Ah, great! That's the flag I was looking for and couldn't remember...

That stuff should be prominently in our dev docs/readmes to help people trying to run from source, and who aren't npm ninjas (like me :)

@fperez
Copy link
Member

fperez commented Dec 14, 2015

btw, is that for the jupyter_notebook_config.py file, or something else?

@captainsafia
Copy link
Member

@fperez: Good point and something I've been meaning to do since I learned about the command myself!

I've opened up and issue and will be looking at it later tonight.

@willingc
Copy link
Member

@captainsafia
Copy link
Member

@willingc: Derp! Thanks for linking. Very useful.

👍

@Carreau
Copy link
Member Author

Carreau commented Dec 14, 2015

That stuff should be prominently in our dev docs/readmes to help people trying to run from source, and who aren't npm ninjas (like me :)

It's here in the docs but we can do better.

It's also in the contributing.md

btw, is that for the jupyter_notebook_config.py file, or something else?

Yes, or JSON config, of command line flag, as you wish.

@Carreau Carreau deleted the fix-shift-jk branch December 14, 2015 22:06
@fperez
Copy link
Member

fperez commented Dec 14, 2015

Sounds good, thanks so much folks! We are indeed getting better here, I really appreciate it :)

@fperez
Copy link
Member

fperez commented Dec 14, 2015

And I apologize to all of you who have helped so much if, in my getting lost in some of our new complexity, I sometimes sound a bit impatient. Grumpy old man and all that... I will do better :)

@willingc
Copy link
Member

@fperez Thanks for sharing your experience honestly (and humbly). You have the benefit of knowing where to ask. Not every user will.

Just to understand the root cause of the initial issue that you had @ellisonbg help with. You were installing and building the docs locally on your system and were getting errors. Which is a reasonable use case for end users that may do a git clone, want to build the docs locally, and try to do so before building the whole project and its dev environment locally.

Grumpy Cat is smiling ;)
2015-10-07 17 56 43

@ellisonbg
Copy link
Contributor

:)

On Mon, Dec 14, 2015 at 4:51 PM, Carol Willing notifications@github.com
wrote:

@fperez https://github.com/fperez Thanks for sharing your experience
honestly (and humbly). You have the benefit of knowing where to ask. Not
every user will.

Just to understand the root cause of the initial issue that you had
@ellisonbg https://github.com/ellisonbg help with. You were installing
and building the docs locally on your system and were getting errors. Which
is a reasonable use case for end users that may do a git clone, want to
build the docs locally, and try to do so before building the whole project
and its dev environment locally.

Grumpy Cat is smiling ;)
[image: 2015-10-07 17 56 43]
https://cloud.githubusercontent.com/assets/2680980/11798734/5656f8bc-a282-11e5-8f85-31e40e66abd6.jpg


Reply to this email directly or view it on GitHub
#708 (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

@fperez
Copy link
Member

fperez commented Dec 15, 2015

Thanks for happy grumpy cat :)

But to clarify: no, it wasn't a doc build, it was trying to run the notebook itself from a git master checkout. But I forgot to run npm run build, so my JS assets were out of date. And, not having stated either in my config or at the command line the ignore_minified_js=True flag, then I was getting stale JS assets.

The behavior was thus rather confusing: at the command-line, a jupyter notebook --version call looked OK, since it reported a current version, which was indeed true. But that current version was 'half-current', if you will, since it was 'python-current', but 'javascript-old', as it was still running old JS code, from whenever I had last run a JS build.

So, the summary is, you either:

  • get religious about always running npm run build
  • or you configure your setup with the ignore_minified_js=True flag.

Since I'm forgetful and I also find the npm build slow, I went for the latter...

@willingc
Copy link
Member

@fperez Thanks for clarifying and the detailed explanation. I've taken your feedback and opened a doc issue so that we don't miss revisiting and improving our docs on this. #872

@fperez
Copy link
Member

fperez commented Dec 15, 2015

Wonderful, thanks!!

@Carreau
Copy link
Member Author

Carreau commented Dec 15, 2015

And I apologize to all of you who have helped so much if, in my getting lost in some of our new complexity, I sometimes sound a bit impatient. Grumpy old man and all that... I will do better :)

We always appreciate feedback from new users ! (Only mildly joking)

You can also nuke all your .min.js files, so if you ever forgot the --ignore... flag, the jsdoes cannot be loaded and the page crashes (which make things more obvious).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants