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

Improve context handling #4453

Merged
merged 66 commits into from
May 25, 2018
Merged
Show file tree
Hide file tree
Changes from 59 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
7a5bd00
WIP context cleanups
jasongrout Apr 23, 2018
78df852
Handle paths having multiple contexts.
jasongrout Apr 24, 2018
db40067
Some new docs about how JupyterLab deals with documents.
jasongrout Apr 25, 2018
1e298d7
WIP
jasongrout Apr 26, 2018
2e93f83
WIP
jasongrout Apr 27, 2018
9c7c4a8
WIP
jasongrout Apr 28, 2018
92d58d0
Revert back to original widget creation workflow, but insist we get b…
jasongrout May 7, 2018
3816766
WIP for MimeDocument widget
jasongrout May 9, 2018
6ede2da
WIP trying out an idea for a mime document renderer, pushing most log…
jasongrout May 10, 2018
0ff19f6
WIP converting code to use IDocumentWidget
jasongrout May 10, 2018
4aa4de6
Fix all compilation errors dealing with the move to IDocumentWidget.
jasongrout May 10, 2018
58a7a4b
Fix lint errors
jasongrout May 11, 2018
3fc6bde
WIP: Fix some tests, skip others while we are evaluating this approach.
jasongrout May 11, 2018
1107f69
Create a specialized MimeDocument document widget type.
jasongrout May 12, 2018
12dfc8e
Delay a document widget from being ready until the context is ready.
jasongrout May 12, 2018
af12ced
Use the ABCWidgetFactory type parameter defaults where we can to simp…
jasongrout May 12, 2018
55f8b32
Convert the notebook to use DocumentWidget
jasongrout May 12, 2018
631d0e1
Move notebook content generation to the widget factory.
jasongrout May 13, 2018
8b3b7e1
Remove image viewer toolbar-lookalike css.
jasongrout May 13, 2018
db7fc2d
Move dirty state handling to the document widget.
jasongrout May 13, 2018
c90b59b
Get rid of double toolbars in file editor and mime document.
jasongrout May 13, 2018
c3496d1
Switch SingletonLayout workaround from panel layout to box layout for…
jasongrout May 13, 2018
f7b03da
Adjust toolbar height to account for visible widgets.
jasongrout May 14, 2018
78e85c3
Fix toolbar collapse status based on explicit hiding, rather than cum…
jasongrout May 14, 2018
b60aaf5
Move csv viewer toolbar over to document widget toolbar.
jasongrout May 15, 2018
1a6b58d
Delete now-redundant notebook toolbar styling.
jasongrout May 15, 2018
11ffaaf
Revert part of 7a5bd00ce3df55b85c747d02a00e87c49e3288b4 dealing with …
jasongrout May 15, 2018
cbb1b6a
Make a CSVDocumentWidget which creates default content and toolbar.
jasongrout May 15, 2018
6554c24
Fix tests
jasongrout May 15, 2018
b0a3234
Merge branch 'master' into context
jasongrout May 15, 2018
5d9d5db
Fix issues with the merge.
jasongrout May 16, 2018
7ecb3c8
Fix CSV viewer toolbar tests.
jasongrout May 16, 2018
447ab04
Fix mimedocument tests.
jasongrout May 16, 2018
ebac5f0
Allow MainAreaWidget ready promises actually be Promise<any> for conv…
jasongrout May 16, 2018
ae8b29b
Add document widget tests.
jasongrout May 16, 2018
087d1d9
Fix more tests.
jasongrout May 16, 2018
0468ec2
WIP notebook tests.
jasongrout May 17, 2018
ae892b4
More notebook test fixes.
jasongrout May 17, 2018
ba2802f
Fix more tests, convert many to use async/await.
jasongrout May 17, 2018
672e843
Add the notebook panel .notebook attribute to ease the transition.
jasongrout May 17, 2018
57b5c07
Fix integrity failures.
jasongrout May 17, 2018
99c6c45
Make MainAreaWidget promise resolve *after* the spinner is removed.
jasongrout May 17, 2018
5a21d5a
When a code editor is created, clear the history after setting the in…
jasongrout May 17, 2018
7b618ba
Clear the notebook model undo history when the model is set.
jasongrout May 17, 2018
ec7d693
Move .notebook references to .content
jasongrout May 17, 2018
d72fd54
Delete the .notebook attribute for notebook panels again.
jasongrout May 18, 2018
ce1abda
Fix model initialization test.
jasongrout May 18, 2018
5da4760
path generally will be a stricter criteria than factory name, so matc…
jasongrout May 18, 2018
64212a6
Initialize a model after the context's first save/revert.
jasongrout May 18, 2018
5027e9e
Merge remote-tracking branch 'origin/master' into context
jasongrout May 20, 2018
f12be48
Just use the BoxLayout instead of trying SingletonLayout.
jasongrout May 20, 2018
7bb239a
Remove comment monologue that is no longer necessary or accurate of t…
jasongrout May 20, 2018
56e5766
Run notebook panel contentfactory tests.
jasongrout May 20, 2018
427a67e
Tweak documentation
jasongrout May 22, 2018
4efcad2
Use StackedLayout instead of BoxLayout for single children.
jasongrout May 22, 2018
21040a0
Fix review comment issues.
jasongrout May 23, 2018
bf94b1e
Document the choices around focusing content.
jasongrout May 23, 2018
d031011
We don’t need the document opener spinner anymore now that the docume…
jasongrout May 23, 2018
dba0b63
MainAreaWidget attribute name change: ready -> reveal
jasongrout May 23, 2018
414fd43
Change the main area widget reveal promise to ‘revealed’
jasongrout May 23, 2018
413f401
Fix the revealed logic for the notebook panel.
jasongrout May 23, 2018
63c41c1
Clarify logic around `isRevealed` to mean “spinner is gone, either a …
jasongrout May 23, 2018
3ab6372
Update docs per review comments.
jasongrout May 24, 2018
6749792
Move signal construction to attribute definition, consistent with the…
jasongrout May 24, 2018
693810c
Support making the content null.
jasongrout May 24, 2018
ac907af
Change MainAreaWidget to using the spinner as just a DOM node rather …
jasongrout May 25, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions docs/source/developer/documents.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Documents
---------

JupyterLab can be extended in two ways via:
JupyterLab can be extended in several ways:

- Extensions (top level): Application extensions extend the
functionality of JupyterLab itself, and we cover them in the
Expand All @@ -15,6 +15,16 @@ JupyterLab can be extended in two ways via:
For this section, the term 'document' refers to any visual thing that
is backed by a file stored on disk (i.e. uses Contents API).

Overview of document architecture
---------------------------------

A 'document' in JupyterLab is represented by a model instance implementing the `IModel http://jupyterlab.github.io/jupyterlab/interfaces/_docregistry_src_registry_.documentregistry.imodel.html`__ interface. The model interface is intentionally fairly small, and concentrates on representing the data in the document and signaling changes to that data. Each model has an associated `context http://jupyterlab.github.io/jupyterlab/interfaces/_docregistry_src_registry_.documentregistry.icontext.html`__ instance as well. The context for a model is the bridge between the internal data of the document, stored in the model, and the file metadata and operations possible on the file, such as save and revert. Since many objects will need both the context and the model, the context contains a reference to the model as its `.model` attribute.

A single file path can have multiple different models (and hence different contexts) representing the file. For example, a notebook can be opened with a notebook model and with a text model. Models for the same file path do not directly communicate with each other.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this sentence was kind of misleading. File views sharing the same model do communicate with each other. Perhaps "Different models for the same file path"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.


`Document widgets http://jupyterlab.github.io/jupyterlab/interfaces/_docregistry_src_registry_.documentregistry.ireadywidget.html`__ represent a view of a document model. There can be multiple document widgets associated with a single document model, and they naturally stay in sync with each other since they are views on the same underlying data model.


The `Document
Registry <http://jupyterlab.github.io/jupyterlab/classes/_docregistry_src_registry_.documentregistry.html>`__
is where document types and factories are registered. Plugins can
Expand All @@ -32,10 +42,10 @@ Document Registry

*Document widget extensions* in the JupyterLab application can register:

- widget factories
- model factories
- widget extension factories
- file types
- model factories for specific file types
- widget factories for specific model factories
- widget extension factories
- file creators

`Widget Factories <http://jupyterlab.github.io/jupyterlab/classes/_docregistry_src_registry_.documentregistry.html#addwidgetfactory>`__
Expand Down
30 changes: 15 additions & 15 deletions examples/notebook/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function createApp(manager: ServiceManager.IManager): void {
let nbWidget = docManager.open(NOTEBOOK) as NotebookPanel;
let palette = new CommandPalette({ commands });

const editor = nbWidget.notebook.activeCell && nbWidget.notebook.activeCell.editor;
const editor = nbWidget.content.activeCell && nbWidget.content.activeCell.editor;
const model = new CompleterModel();
const completer = new Completer({ editor, model });
const connector = new KernelConnector({ session: nbWidget.session });
Expand All @@ -135,7 +135,7 @@ function createApp(manager: ServiceManager.IManager): void {
handler.editor = editor;

// Listen for active cell changes.
nbWidget.notebook.activeCellChanged.connect((sender, cell) => {
nbWidget.content.activeCellChanged.connect((sender, cell) => {
handler.editor = cell && cell.editor;
});

Expand Down Expand Up @@ -170,15 +170,15 @@ function createApp(manager: ServiceManager.IManager): void {
commands.addCommand(cmdIds.invokeNotebook, {
label: 'Invoke Notebook',
execute: () => {
if (nbWidget.notebook.activeCell.model.type === 'code') {
if (nbWidget.content.activeCell.model.type === 'code') {
return commands.execute(cmdIds.invoke);
}
}
});
commands.addCommand(cmdIds.selectNotebook, {
label: 'Select Notebook',
execute: () => {
if (nbWidget.notebook.activeCell.model.type === 'code') {
if (nbWidget.content.activeCell.model.type === 'code') {
return commands.execute(cmdIds.select);
}
}
Expand Down Expand Up @@ -206,48 +206,48 @@ function createApp(manager: ServiceManager.IManager): void {
commands.addCommand(cmdIds.runAndAdvance, {
label: 'Run and Advance',
execute: () => {
NotebookActions.runAndAdvance(nbWidget.notebook, nbWidget.context.session);
NotebookActions.runAndAdvance(nbWidget.content, nbWidget.context.session);
}
});
commands.addCommand(cmdIds.editMode, {
label: 'Edit Mode',
execute: () => { nbWidget.notebook.mode = 'edit'; }
execute: () => { nbWidget.content.mode = 'edit'; }
});
commands.addCommand(cmdIds.commandMode, {
label: 'Command Mode',
execute: () => { nbWidget.notebook.mode = 'command'; }
execute: () => { nbWidget.content.mode = 'command'; }
});
commands.addCommand(cmdIds.selectBelow, {
label: 'Select Below',
execute: () => NotebookActions.selectBelow(nbWidget.notebook)
execute: () => NotebookActions.selectBelow(nbWidget.content)
});
commands.addCommand(cmdIds.selectAbove, {
label: 'Select Above',
execute: () => NotebookActions.selectAbove(nbWidget.notebook)
execute: () => NotebookActions.selectAbove(nbWidget.content)
});
commands.addCommand(cmdIds.extendAbove, {
label: 'Extend Above',
execute: () => NotebookActions.extendSelectionAbove(nbWidget.notebook)
execute: () => NotebookActions.extendSelectionAbove(nbWidget.content)
});
commands.addCommand(cmdIds.extendBelow, {
label: 'Extend Below',
execute: () => NotebookActions.extendSelectionBelow(nbWidget.notebook)
execute: () => NotebookActions.extendSelectionBelow(nbWidget.content)
});
commands.addCommand(cmdIds.merge, {
label: 'Merge Cells',
execute: () => NotebookActions.mergeCells(nbWidget.notebook)
execute: () => NotebookActions.mergeCells(nbWidget.content)
});
commands.addCommand(cmdIds.split, {
label: 'Split Cell',
execute: () => NotebookActions.splitCell(nbWidget.notebook)
execute: () => NotebookActions.splitCell(nbWidget.content)
});
commands.addCommand(cmdIds.undo, {
label: 'Undo',
execute: () => NotebookActions.undo(nbWidget.notebook)
execute: () => NotebookActions.undo(nbWidget.content)
});
commands.addCommand(cmdIds.redo, {
label: 'Redo',
execute: () => NotebookActions.redo(nbWidget.notebook)
execute: () => NotebookActions.redo(nbWidget.content)
});

let category = 'Notebook Operations';
Expand Down
2 changes: 1 addition & 1 deletion packages/application/src/mimerenderers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
} from '@jupyterlab/apputils';

import {
MimeDocument, MimeDocumentFactory, DocumentRegistry
MimeDocumentFactory, DocumentRegistry, MimeDocument
} from '@jupyterlab/docregistry';

import {
Expand Down
72 changes: 55 additions & 17 deletions packages/apputils/src/mainareawidget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,12 @@ class MainAreaWidget<T extends Widget = Widget> extends Widget {
this.title.changed.connect(this._updateContentTitle, this);
content.disposed.connect(() => this.dispose());

if (options.populated) {
if (options.reveal) {
layout.addWidget(spinner);
this.populated = options.populated;
this.populated.then(() => {
this._isPopulated = true;
// Make sure the ready promise is a Promise<void> to avoid leaking any
// results.
this._reveal = options.reveal.then(() => {
this._isRevealed = true;
const active = document.activeElement === spinner.node;
spinner.dispose();
layout.addWidget(content);
Expand All @@ -88,13 +89,14 @@ class MainAreaWidget<T extends Widget = Widget> extends Widget {
BoxLayout.setStretch(error, 1);
spinner.dispose();
layout.addWidget(error);
throw(error);
});
// Handle no populated promise.
} else {
spinner.dispose();
layout.addWidget(content);
this._isPopulated = true;
this.populated = Promise.resolve(void 0);
this._isRevealed = true;
this._reveal = Promise.resolve(undefined);
}
}

Expand All @@ -109,16 +111,18 @@ class MainAreaWidget<T extends Widget = Widget> extends Widget {
readonly toolbar: Toolbar;

/**
* Whether the widget is fully populated.
* Whether the widget is revealed.
*/
get isPopulated(): boolean {
return this._isPopulated;
get isRevealed(): boolean {
return this._isRevealed;
}

/**
* A promise that resolves when the widget is fully populated.
* A promise that resolves when the widget is ready to be revealed.
*/
readonly populated: Promise<void>;
get reveal(): Promise<void> {
return this._reveal;
}

/**
* Handle the DOM events for the widget.
Expand All @@ -135,7 +139,7 @@ class MainAreaWidget<T extends Widget = Widget> extends Widget {
case 'mouseup':
case 'mouseout':
let target = event.target as HTMLElement;
if (this._isPopulated &&
if (this._isRevealed &&
this.toolbar.node.contains(document.activeElement) &&
target.tagName !== 'SELECT') {
this._focusContent();
Expand Down Expand Up @@ -166,7 +170,7 @@ class MainAreaWidget<T extends Widget = Widget> extends Widget {
* Handle `'activate-request'` messages.
*/
protected onActivateRequest(msg: Message): void {
if (this._isPopulated) {
if (this._isRevealed) {
this._focusContent();
} else {
this._spinner.node.focus();
Expand Down Expand Up @@ -222,16 +226,21 @@ class MainAreaWidget<T extends Widget = Widget> extends Widget {
* Give focus to the content.
*/
private _focusContent(): void {
// Focus the content node if we aren't already focused on it or a
// descendent.
if (!this.content.node.contains(document.activeElement)) {
this.content.node.focus();
}
// Give the content a chance to activate.

// Activate the content asynchronously (which may change the focus).
this.content.activate();
}

private _changeGuard = false;
private _isPopulated = false;
private _spinner = new Spinner();

private _isRevealed = false;
private _reveal: Promise<void>;
}


Expand All @@ -256,8 +265,37 @@ namespace MainAreaWidget {
toolbar?: Toolbar;

/**
* An optional promise for when the content is fully populated.
* An optional promise for when the content is ready to be revealed.
*/
reveal?: Promise<any>;
}

/**
* An options object for main area widget subclasses providing their own
* default content.
*
* #### Notes
* This makes it easier to have a subclass that provides its own default
* content. This can go away once we upgrade to TypeScript 2.8 and have an
* easy way to make a single property optional, ala
* https://stackoverflow.com/a/46941824
*/
export
interface IOptionsOptionalContent<T extends Widget = Widget> extends Widget.IOptions {
/**
* The child widget to wrap.
*/
content?: T;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your current thinking on ownership issues for the content of a MainAreaWidget? When this is passed in, does ownership transfer to the main area widget, or does it stick with the new caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has to transfer ownership to the MainAreaWidget. In phosphor, widgets reparent/own their children, dispose their children, move their children in the DOM to be under them, etc.


/**
* The toolbar to use for the widget. Defaults to an empty toolbar.
*/
toolbar?: Toolbar;

/**
* An optional promise for when the content is ready to be revealed.
*/
populated?: Promise<void>;
reveal?: Promise<any>;
}

}
Loading