jupyter / jupyter-js-notebook

JupyterLab notebook
BSD 3-Clause "New" or "Revised" License
46 stars 17 forks source link

NotebookCells / Notebook separation #115

Closed jasongrout closed 8 years ago

jasongrout commented 8 years ago

The separation between the Notebook widget class and the NotebookCells widget class is not very clean. Currently the Notebook class is muddling about with the inner structure of the NotebookCells layout (https://github.com/jupyter/jupyter-js-notebook/blob/master/src/notebook/widget.ts#L422). Also, the notebook model is passed wholesale to the NotebookCells class (i.e., it's not a submodel).

Some options for making this cleaner include:

  1. Make the createCell function a static member of NotebookCells, and then move the onCellsChanged down to the NotebookCells class.
  2. Create a new higher level widget that encompasses both the notebook and the toolbar, say "NotebookContainer" or something. Then merge NotebookCells back into Notebook, taking out the toolbar parts and putting them in NotebookContainer. (Note: I don't really like the name "NotebookContainer" as it's too broad.) Basically this means that the "notebook" is really still just the list of cells, and it is embedded inside of something else that provides some additional elements in a tab.

Thoughts? I kind of like (2).

blink1073 commented 8 years ago

How about the following:

class NotebookWidget extends Widget {
   static createCell(cell: ICellModel): BaseCellWidget {}
   constructor(model: INotebookModel) {}
}

class NotebookToolbar extends Widget {
   constructor(manager: INotebookManager) {}
 }

class ActiveNotebook extends Widget {
   static createNotebook(model: INotebookModel): NotebookWidget {}
   static createToolbar(manager: NotebookManager): NotebookToolbar {}
   constructor(manager: INotebookManager) {}
}
blink1073 commented 8 years ago

or, if we want to leave model out to NotebookManager:

class NotebookWidget extends Widget {
   static createCell(cell: ICellModel): BaseCellWidget {}
   constructor(model: INotebookModel) {}
}

class NotebookToolbar extends Widget {
   constructor(model: INotebookModel, manager: INotebookManager) {}
 }

class ActiveNotebook extends Widget {
   static createNotebook(model: INotebookModel): NotebookWidget {}
   static createToolbar(model: INotebookModel, manager: NotebookManager): NotebookToolbar {}
   constructor(model: INotebookModel, manager: INotebookManager) {}
}
blink1073 commented 8 years ago

I'd vote to leave it in, though. Anything with a handle on the model should be able to mutate it, it is in charge of maintaining its own self-consistency, and the views react to changes in model state.

blink1073 commented 8 years ago

I went down the path of removing model from NotebookManager, but we need a delete and a copy stack for each model. We would have to move delete/undelete and copy/paste actions to the model make the manager model agnostic. That seems like a fair thing to do, do you agree?

blink1073 commented 8 years ago

Cut could be implemented as a copy and a delete.