Implement multi-cell selection #151

blink1073 · 2016-04-06T17:09:27Z

jasongrout · 2016-04-06T19:17:04Z

src/cells/model.ts

-  private _tags = '';
-  private _name = '';
+  private _tags = '{}';
+  private _name = '{}';


Should the default _name be '{}' or '""'? The empty string makes more sense to me.

It can't be empty, since that causes the JSON.parse() to fail, agreed that tags should be [] though.

blink1073 · 2016-04-06T19:24:22Z

Metadata defaults updated in f583525

jasongrout · 2016-04-06T19:25:30Z

The selection logic is a bit more subtle in the current notebook. Essentially, only the cells between the active cell and the selection anchor are selected. So, for example, shift-J, shift-K should only leave the anchor selected. This is similar to how selection of characters in a word processor works.

blink1073 · 2016-04-06T19:29:57Z

Understood, updating.

jasongrout · 2016-04-06T19:32:40Z

Thanks. I'm excited to see this get in!

blink1073 · 2016-04-06T19:41:59Z

Updated (see gif above).

jasongrout · 2016-04-06T20:14:49Z

src/notebook/manager.ts

+      for (let i = 0; i < this.model.cells.length; i++) {
+        let cell = this.model.cells.get(i);
+        if (this.model.isSelected(cell)) {
+          count++;


Since we only care if any cell besides prev is selected, we can break as soon as we find one.

Actually, under the current constraints (only a single contiguous selection), we only need to check to see if the cell before prev is selected.

In the future we'll probably have real (non-contiguous) multiple selection, and we'll have to rethink how all this logic works.

jasongrout · 2016-04-06T20:30:02Z

Movement without shift should deselect as well: Shift-J J should deselect all cells.

blink1073 · 2016-04-06T20:47:59Z

Thanks, updated.

jasongrout · 2016-04-06T22:48:53Z

src/notebook/manager.ts


+  /**
+   * Select the next cell.
+   */


We shouldn't call this "select.." if we are deselecting cells. Maybe "activateNext" since it is changing the active cell?

jasongrout · 2016-04-06T23:17:05Z

@ellisonbg - it looks like @blink1073 has implemented the correct logic. I just wanted to loop you in for terminology: above/below vs prev/next, and select vs. active. I know we had a long discussion and specific definitions of these - I just don't remember exactly what they were :). I see in https://github.com/jupyter/notebook/blob/41d6da235cbf3bcf6d7f818e11a066e0fd12ff8b/notebook/static/notebook/js/actions.js#L14 that it seems like above/below is pretty interchangeable with prev/next. It seems like the actions in the current notebook indicate that the currently active cell is always selected, whereas we've distinguished between selected cells and the active cell (i.e., we can have no cells selected). Thoughts?

blink1073 · 2016-04-07T00:24:08Z

We could use currentCellIndex and selected, where the "current" cell is always selected, and then add a class to the "current" cell if it is not the only selected cell.

blink1073 · 2016-04-07T00:24:45Z

I also think we should standardize on next/prev or above/below but have no preference.

jasongrout · 2016-04-07T19:16:34Z

Yes, I noticed you used both (above/below in the function title, prev/next as variables in the code) 😄

blink1073 · 2016-04-07T19:41:42Z

The existing Help Shortcuts dialog uses above/below, and I do remember having this conversation with Brian last October, let's go with that. @jasongrout, you okay with the current/selected semantics I proposed?

jasongrout · 2016-04-07T19:49:04Z

@blink1073: +1 to your current/selected semantics, though I think it's still okay to use active instead of current.

blink1073 · 2016-04-07T19:51:46Z

I was thinking of it more like a "cursor position".

blink1073 · 2016-04-07T19:57:14Z

Let's leave it as active for now though, since a lot of code is using it.

blink1073 · 2016-04-07T19:57:23Z

We can discuss tomorrow at the meeting.

jasongrout · 2016-04-07T20:19:13Z

Good idea.

On Thu, Apr 7, 2016 at 3:57 PM Steven Silvester notifications@github.com
wrote:

We can discuss tomorrow at the meeting.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#151 (comment)

blink1073 · 2016-04-07T20:32:39Z

Updated.

SylvainCorlay · 2016-04-07T21:55:42Z

Btw, with @Carreau we re-discussed jupyter/notebook#881 at the dev meeting.

ellisonbg · 2016-04-08T05:17:11Z

For the above/below and next/preview naming, please see the list of
action/command names in the existing notebook:

https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/js/actions.js#L65

We spent an entire day auditing those back in new york last fall with
JupyterLab in mind and made very careful decisions about the naming of
those things. The short summary is that there are very subtle differences -
sometimes above/below makes sense, other times next/previous does.

Also, as much as possible, please follow the command naming conventions
that we created there in JupyterLab.

On Thu, Apr 7, 2016 at 2:55 PM, Sylvain Corlay notifications@github.com
wrote:

Btw, with @Carreau https://github.com/Carreau we re-discussed
jupyter/notebook#881 jupyter/notebook#881 at
the dev meeting.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#151 (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

ellisonbg · 2016-04-08T05:17:56Z

Can someone summarize how selected/active are currently being used in
JupyterLab api?

On Thu, Apr 7, 2016 at 10:16 PM, Brian Granger ellisonbg@gmail.com wrote:

For the above/below and next/preview naming, please see the list of
action/command names in the existing notebook:

https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/js/actions.js#L65

We spent an entire day auditing those back in new york last fall with
JupyterLab in mind and made very careful decisions about the naming of
those things. The short summary is that there are very subtle differences -
sometimes above/below makes sense, other times next/previous does.

Also, as much as possible, please follow the command naming conventions
that we created there in JupyterLab.

On Thu, Apr 7, 2016 at 2:55 PM, Sylvain Corlay notifications@github.com
wrote:

Btw, with @Carreau https://github.com/Carreau we re-discussed
jupyter/notebook#881 jupyter/notebook#881 at
the dev meeting.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#151 (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

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

jasongrout · 2016-04-08T06:10:59Z

active indicates the current cell (i.e., has edit focus, or command
focus). It's the one with the border, the one shift-enter applies to, etc.

selected sets a selection property on the cell, and there can be more than
one cell selected. I think the current state of this PR has it so that
there can never be just one cell selected, and selected cells are always a
single contiguous block.

On Fri, Apr 8, 2016 at 1:17 AM Brian E. Granger notifications@github.com
wrote:

Can someone summarize how selected/active are currently being used in
JupyterLab api?

On Thu, Apr 7, 2016 at 10:16 PM, Brian Granger ellisonbg@gmail.com
wrote:

For the above/below and next/preview naming, please see the list of
action/command names in the existing notebook:

https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/js/actions.js#L65

We spent an entire day auditing those back in new york last fall with
JupyterLab in mind and made very careful decisions about the naming of
those things. The short summary is that there are very subtle
differences -
sometimes above/below makes sense, other times next/previous does.

Also, as much as possible, please follow the command naming conventions
that we created there in JupyterLab.

On Thu, Apr 7, 2016 at 2:55 PM, Sylvain Corlay <notifications@github.com

wrote:

Btw, with @Carreau https://github.com/Carreau we re-discussed
jupyter/notebook#881 jupyter/notebook#881 at
the dev meeting.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
<
#151 (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

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

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#151 (comment)

blink1073 · 2016-04-08T11:48:09Z

As it stands, the "active" cell is considered selected, as in isSelected() will check to see if it is the active cell index, so there is always at least one cell selected.

jasongrout · 2016-04-08T22:15:09Z

Thanks for clarifying, @blink1073

jasongrout · 2016-04-08T22:26:31Z

A fresh checkout of the repo, plus the "command line instructions" above to merge this into master, gives this when running npm run build:example

src/index.ts(85,7): error TS2322: Type 'JavascriptRenderer' is not assignable to type 'IRenderer<Widget>'.
  Types of property 'render' are incompatible.
    Type '(mimetype: string, data: string) => Widget' is not assignable to type '(mimetype: string, data: string) => Widget'.
      Type 'Widget' is not assignable to type 'Widget'.
        Types of property 'layout' are incompatible.
          Type 'Layout' is not assignable to type 'Layout'.
            Property 'initialize' is protected but type 'Layout' is not a class derived from 'Layout'.
src/index.ts(89,47): error TS2345: Argument of type 'RenderMime<Widget>' is not assignable to parameter of type 'RenderMime<Widget>'.
  Types of property 'clone' are incompatible.
    Type '() => RenderMime<Widget>' is not assignable to type '() => RenderMime<Widget>'.
      Type 'RenderMime<Widget>' is not assignable to type 'RenderMime<Widget>'.

jasongrout · 2016-04-08T22:40:01Z

Here are the conflicts I have when I install:

npm WARN unavoidable conflict lodash { '[snip]/jupyter-js-notebook/node_modules/jupyter-js-ui/node_modules/ansi-to-html/node_modules/karg/node_modules/lodash': '4.9.0',
npm WARN unavoidable conflict   '[snip]/jupyter-js-notebook/node_modules/jupyter-js-ui/node_modules/ansi-to-html/node_modules/karg/node_modules/noon/node_modules/simple-plist/node_modules/plist/node_modules/xmlbuilder/node_modules/lodash': '3.10.1' }
npm WARN unavoidable conflict Not de-duplicating
esnextguardian@1.2.1 node_modules/ansi-to-html/node_modules/karg/node_modules/noon/node_modules/cson/node_modules/esnextguardian
backbone@1.2.0 node_modules/backbone
└── underscore@1.8.3
underscore@1.8.3 node_modules/underscore

> fsevents@1.0.11 install [snip]/jupyter-js-notebook/node_modules/karma/node_modules/chokidar/node_modules/fsevents
> node-pre-gyp install --fallback-to-build

[fsevents] Success: "[snip]/jupyter-js-notebook/node_modules/karma/node_modules/chokidar/node_modules/fsevents/lib/binding/Release/node-v46-darwin-x64/fse.node" is installed via remote

> fsevents@1.0.11 install [snip]/jupyter-js-notebook/node_modules/webpack/node_modules/watchpack/node_modules/chokidar/node_modules/fsevents
> node-pre-gyp install --fallback-to-build

[fsevents] Success: "[snip]/jupyter-js-notebook/node_modules/webpack/node_modules/watchpack/node_modules/chokidar/node_modules/fsevents/lib/binding/Release/node-v46-darwin-x64/fse.node" is installed via remote

> jupyter-js-notebook@0.17.1 postinstall [snip]/app/tmp
> npm dedupe

npm WARN unavoidable conflict lodash { '[snip]/jupyter-js-notebook/node_modules/jupyter-js-ui/node_modules/ansi-to-html/node_modules/karg/node_modules/lodash': '4.9.0',
npm WARN unavoidable conflict   '[snip]/jupyter-js-notebook/node_modules/jupyter-js-ui/node_modules/ansi-to-html/node_modules/karg/node_modules/noon/node_modules/simple-plist/node_modules/plist/node_modules/xmlbuilder/node_modules/lodash': '3.10.1' }
npm WARN unavoidable conflict Not de-duplicating
npm WARN unavoidable conflict jupyter-js-services { '[snip]/jupyter-js-notebook/node_modules/jupyter-js-services': '0.7.1',
npm WARN unavoidable conflict   '[snip]/jupyter-js-notebook/node_modules/jupyter-js-ui/node_modules/jupyter-js-services': '0.6.6' }
npm WARN unavoidable conflict Not de-duplicating

blink1073 · 2016-04-08T23:56:08Z

Are you using node 5?

jasongrout · 2016-04-09T00:09:48Z

Nope, 4.2.4.

On Fri, Apr 8, 2016 at 7:56 PM Steven Silvester notifications@github.com
wrote:

Are you using node 5?

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#151 (comment)

blink1073 · 2016-04-09T00:11:17Z

Let me try rebasing, Darian had patched something this morning.

blink1073 · 2016-04-09T00:11:45Z

Rebased.

jasongrout · 2016-04-09T00:12:09Z

I'm also looking at merging your jupyter-js-ui PR and upgrading
jupyer-js-ui.

On Fri, Apr 8, 2016 at 8:11 PM Steven Silvester notifications@github.com
wrote:

Let me try rebasing, Darian had patched something this morning.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#151 (comment)

jasongrout · 2016-04-09T00:24:15Z

I get the same errors with the rebased branch. It still looks to me like there is a dedupe-type problem.

blink1073 · 2016-04-09T00:30:19Z

Hmm, you can either upgrade to node 5 or we can wait for phosphor-ui ;)

jasongrout · 2016-04-09T00:44:10Z

I'm going to hammer on this for a bit. The error clearly indicates there are some mismatched version numbers that are causing duplication.

blink1073 · 2016-04-09T00:57:18Z

Try running npm update now and see if that fixes it.

blink1073 · 2016-04-09T00:58:01Z

Actually, you may have to nuke node_modules.

jasongrout · 2016-04-09T01:34:34Z

I've been killing node modules every time. I'll try with the updated
dependencies.

On Fri, Apr 8, 2016, 20:58 Steven Silvester notifications@github.com
wrote:

Try running npm update now and see if that fixes it.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#151 (comment)

jasongrout · 2016-04-09T15:51:42Z

All right, trying with node 5.10.1 worked fine, so I'll merge this and keep digging about the problem on node 4.x. Thanks!

jasongrout reviewed Apr 6, 2016
View reviewed changes

blink1073 force-pushed the multiple-selection branch from a6b3011 to 9a4188c Compare April 6, 2016 21:57

jasongrout reviewed Apr 6, 2016
View reviewed changes

blink1073 mentioned this pull request Apr 7, 2016

Line number handling jupyter/jupyter-js-plugins#150

Merged

blink1073 force-pushed the multiple-selection branch from 457a0a9 to 145dcec Compare April 8, 2016 13:49

blink1073 added 5 commits April 8, 2016 19:11

Implement multi-cell selection

c840fba

Better handling of selection

f58e85d

More improvements to multiselection

c35d5b0

Fix variable name

80fc3d5

Clean up selection logic and rename next/prev to above/below

0c7995f

blink1073 force-pushed the multiple-selection branch from 145dcec to 0c7995f Compare April 9, 2016 00:11

jasongrout merged commit f1b9cd0 into jupyter:master Apr 9, 2016

blink1073 deleted the multiple-selection branch April 14, 2016 19:19

Implement multi-cell selection #151

Implement multi-cell selection #151

Uh oh!

Conversation

blink1073 commented Apr 6, 2016

Uh oh!

jasongrout Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

blink1073 Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

blink1073 commented Apr 6, 2016

Uh oh!

jasongrout commented Apr 6, 2016

Uh oh!

blink1073 commented Apr 6, 2016

Uh oh!

jasongrout commented Apr 6, 2016

Uh oh!

blink1073 commented Apr 6, 2016

Uh oh!

jasongrout Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasongrout Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasongrout commented Apr 6, 2016

Uh oh!

blink1073 commented Apr 6, 2016

Uh oh!

jasongrout Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasongrout commented Apr 6, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

jasongrout commented Apr 7, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

jasongrout commented Apr 7, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

jasongrout commented Apr 7, 2016

Uh oh!

blink1073 commented Apr 7, 2016

Uh oh!

SylvainCorlay commented Apr 7, 2016

Uh oh!

ellisonbg commented Apr 8, 2016

Uh oh!

ellisonbg commented Apr 8, 2016

Uh oh!

jasongrout commented Apr 8, 2016

Uh oh!

blink1073 commented Apr 8, 2016

Uh oh!

jasongrout commented Apr 8, 2016

Uh oh!

jasongrout commented Apr 8, 2016

Uh oh!

jasongrout commented Apr 8, 2016

Uh oh!

blink1073 commented Apr 8, 2016

Uh oh!

jasongrout commented Apr 9, 2016

Uh oh!

blink1073 commented Apr 9, 2016

Uh oh!

blink1073 commented Apr 9, 2016