Clean up architecture

[blink1073]

Fixes #166.

[blink1073]

Notes from discussion this afternoon: The notebook and cell models should reflect the notebook format exactly, except for metadata which is handled through cursors. All data is shared, and a change results in a contentChanged(name: string) signal.

[jasongrout]

For context, we considered the idea that things like the notebook format scrolled cell state should really mean initial scrolled state for the cell, which would mean two widgets displaying the same notebook could have different scrolled states for the same cell. However, there are lots of complications like:

• What and when should this state be saved? Should save automatically save the scrolled states of cells? Should that be a separate action? I can imagine going through and setting up the layout, and then wanting to just save a change to the content without changing the layout.
• What happens on a revert? What happens when a cell widget is recreated in a revert?

So we decided for now that disk state is model state is shared state between widgets. Even if that nbformat state is really UI information.

[blink1073]

@jasongrout, commits c80de36 and 457738b reflect the changes to the models.

[jasongrout]

The comments on metadata cursors about hiding keys defined in the nbformat spec are no longer true (for example in the [get|list]Metadata functions in cells/model.ts)

[jasongrout]

Also this note should be changed: https://github.com/jupyter/jupyter-js-notebook/pull/213/commits/daf844d5683031330c8a5d1797e6e37502574779#diff-253925527f3d36d6b2e359ea68312f08R73

[blink1073]

961fbf6 adds a simple toolbar class.

[blink1073]

@jasongrout, 1329c90 has a proposal for clipboard and undo/redo handling.

• All ActiveNotebook instances would share a clipboard with cell JSON data.
• The notebook model (and perhaps all IDocumentModels) supports a similar API to Google Drive for undo/redo handling.

https://developers.google.com/google-apps/realtime/undo

[jasongrout]

The undo/redo looks okay to me. The clipboard should be application-wide, I think. Perhaps in a separate clipboard service plugin (or perhaps attached to the application?)

[blink1073]

Agreed on the app-wide clipboard, I'll think about what that should look like.

As far as undo/redo at the notebook level, I see that as being linked to the events on the observable list of cells. We store the change events and can perform the reverse of the event as needed. For instance, reverting the document would result in an assign() call and a Replace event, which could be undone using the arguments of the event in reverse. We can combine events using beginCompoundOperation.

I don't think a general undo/redo makes sense on the notebook document. For instance, if the last thing that changed was metadata, you may have no visual representation that the metadata changed.

Undo/redo in edit mode would be handled by the CodeMirror instance.

[jasongrout]

We could punt on doing undo redo for now and just rely on code mirror undo for editing.

[blink1073]

Too late ;). I'm about to push a undo/redo capability based on changes to the cells observable list. We'll see where it gets us.

I am also adding a clipboard which is basically the MimeData type used in drag-drop for now that is passed to the NotebookPanel.

[blink1073]

See 8e9cd23 for initial implementation. I'm updating next action.ts to use the undo actions appropriately, then creating the default toolbar using those actions.

[blink1073]

The undo/redo implementation was modeled after: https://github.com/jzaefferer/undo/blob/master/undo.js

[blink1073]

Actions updated in bac69bd.

[blink1073]

I'm off tomorrow, and will pick this up on Monday. What remains to be done (outside of user acceptance testing) is to create model and widget factories. The notebook widget factory will use a registry for creating notebook instance extensions. At that point we should be ready to add ipywidget support in a follow on PR.

[blink1073]

The last commit finishes the initial implementation (including undo/redo handling), but depends on https://github.com/jupyter/jupyter-js-ui/pull/95.

[blink1073]

All of the actions that affect cells action.ts properly support undo and redo.

[blink1073]

After #214 is merged, I will rebase and update the console, then this should be good to go.

[jasongrout]

@blink1073 - a fresh checkout of your branch and building examples gives:

src/index.ts(9,3): error TS2305: Module '"[snip]/nb/examples/console/node_modules/jupyter-js-notebook/lib/index"' has no exported member 'ConsolePanel'.
src/index.ts(9,17): error TS2305: Module '"[snip]/nb/examples/console/node_modules/jupyter-js-notebook/lib/index"' has no exported member 'ConsoleWidget'.
src/index.ts(9,32): error TS2305: Module '"[snip]/nb/examples/console/node_modules/jupyter-js-notebook/lib/index"' has no exported member 'ConsoleModel'.

Is this a known issue?

[blink1073]

Yes, because the console is broken in this PR pending #214. I've been building the notebook example from within itself.

[jasongrout]

I just looked through the notebook undo stuff. A couple of comments:

1. It's only handling undo/redo of the cells container, but not notebook metadata and other cell changes. Of course, like you mentioned before, the editor can handle text undo of cell source. It's an open question, I think, if we want to support undo/redo of metadata.
2. It feels weird to have an undo object that has a reference to the model, but is stored in the model. In fact, the only thing the undo thing needs is (a) a way to copy/create list elements, and (b) a reference to the list. Perhaps the undo should just be folded into the list implementation: ObservableUndoableList or something.

[jasongrout]

note to self: related to undo, here is a page listing a bunch of immutable-type libraries for javascript: https://github.com/markerikson/redux-ecosystem-links/blob/master/immutable-data.md

[blink1073]

The current implementation is used to allow a snapshot of the cell model. We need to know to call .toJSON on the cells to serialize them, and we need the model in order to ask it how to create a new cell given cell data.

[blink1073]

We could have an ObservableUndoAbleList<IClonable> where the objects need to be able to clone themselves.

[blink1073]

An immutable version of this list would have to store the entire state of the list for each change, as opposed to just the change metadata.

[jasongrout]

Right, here's a possible sketch

interface IJSONable {
    toJSON(): any;
}

class Cell implements IJSONable {
    toJSON() {
        return {};
    }
}

class IOList<T extends IJSONable> {
    constructor(factory: (any) => Cell) {
        this._factory = factory;
    }

    private _factory: (any) => Cell;
}

let factory = (any) => {return new Cell();}
let x = new IOList<Cell>(factory);

[jasongrout]

A smart immutable structure would share substructures where it could, rather than storing the entire list for every change.

[blink1073]

Moved undo/redo logic onto the cells list in 5cc1f84.

[jasongrout]

For future reference, @blink1073 and I discussed the undo/redo, and I think the conclusions were:

1. it's a much bigger project to provide comprehensive, intuitive undo/redo (for example, what should we do about notebook or cell metadata changes? Are there other model/widget changes we want to undo/redo?)
2. the current solution is a stop-gap that implements the current notebook undelete behavior in a more consistent way
3. we should address this later, since the priority right now is getting a demo out there in people's hands

[blink1073]

Rebased to pull in the changes to the console, will rectify the console this weekend.

[blink1073]

This is now "ready", aside from discussion about how we want to handle the contentChanged signal payload.

It might make more sense to use contentChanged<model: IDocumentModel, IChangeArgs<any>> and separately add metadataChanged<model: IDocumentModel, IChangeArgs<any>, where a change to metadata would trigger both signals.

[blink1073]

The more I think about it, the more I think contentChanged should be a void signal, stateChanged should be used for simple values that change, and add metadataChanged because it is a complex value like the cells.

[blink1073]

@afshin, I ended up merging the console model into the widget, but left the rest of the pieces largely untouched. Do you mind reviewing console/widget.ts?

[blink1073]

Commit 8a85d23 adds tab completion to the notebook, and 9db2167 adds the ability to select text in the outputs, two of Fernando's concerns.

[jasongrout]

Thanks! Let's merge and iterate at this point. I'll post an issue with some things that I'm seeing with completion, for example.

[jasongrout]

I pushed version 0.20.0 with this merged.

[blink1073]

If we update jupyter-js-plugins now, it would be missing the ability to create new notebooks other than the default, switch kernels, and open images. I say we merge https://github.com/jupyter/jupyter-js-ui/pull/97 with just the kernel selector and quick-launch creators and then do a proper update to jupyter-js-plugins. I will be able to finish those tomorrow.