From 2829b4b3f39798510daccc313e16225f653e20ed Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Mon, 11 Feb 2019 17:02:59 -0800 Subject: [PATCH 1/7] Expose a restored promise on InstanceTracker (but not IInstanceTracker). --- packages/apputils/src/instancetracker.ts | 50 +++++++++++++++++------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/packages/apputils/src/instancetracker.ts b/packages/apputils/src/instancetracker.ts index 525ac04fa6fc..57322e6f25e5 100644 --- a/packages/apputils/src/instancetracker.ts +++ b/packages/apputils/src/instancetracker.ts @@ -7,7 +7,7 @@ import { ArrayExt, each, find } from '@phosphor/algorithm'; import { CommandRegistry } from '@phosphor/commands'; -import { ReadonlyJSONObject } from '@phosphor/coreutils'; +import { PromiseDelegate, ReadonlyJSONObject } from '@phosphor/coreutils'; import { IDisposable } from '@phosphor/disposable'; @@ -334,20 +334,39 @@ export class InstanceTracker this._restore = options; - return Promise.all(promises).then(([saved]) => { - return Promise.all( - saved.ids.map((id, index) => { - const value = saved.values[index]; - const args = value && (value as any).data; - if (args === undefined) { - return state.remove(id); - } - - // Execute the command and if it fails, delete the state restore data. - return registry.execute(command, args).catch(() => state.remove(id)); - }) - ); - }); + return Promise.all(promises) + .then(([saved]) => { + return Promise.all( + saved.ids.map((id, index) => { + const value = saved.values[index]; + const args = value && (value as any).data; + if (args === undefined) { + return state.remove(id); + } + + // Execute the command and if it fails, delete the state restore data. + return registry + .execute(command, args) + .catch(() => state.remove(id)); + }) + ); + }) + .then(val => { + this._restored.resolve(void 0); + return val; + }); + } + + /** + * A promise resolved when the instance tracker has been restored. + * + * #### Notes + * This promise is not exposed on the IInstanceTracker interface. + * It is intended to allow for the owner/creator of an instance tracker + * to perform additional actions after restoration in specialized use-cases. + */ + get restored(): Promise { + return this._restored.promise; } /** @@ -448,6 +467,7 @@ export class InstanceTracker } private _restore: InstanceTracker.IRestoreOptions | null = null; + private _restored = new PromiseDelegate(); private _tracker = new FocusTracker(); private _currentChanged = new Signal(this); private _widgetAdded = new Signal(this); From 8f82a8b27f01555e71dddbb778dd5f27813ef3b9 Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Mon, 11 Feb 2019 17:31:09 -0800 Subject: [PATCH 2/7] WIP restoring outputs. --- packages/notebook-extension/package.json | 2 + packages/notebook-extension/src/index.ts | 102 +++++++++++++++++++--- packages/notebook-extension/tsconfig.json | 3 + 3 files changed, 96 insertions(+), 11 deletions(-) diff --git a/packages/notebook-extension/package.json b/packages/notebook-extension/package.json index d9ae506caa0f..8fb04d29ae46 100644 --- a/packages/notebook-extension/package.json +++ b/packages/notebook-extension/package.json @@ -36,6 +36,7 @@ "@jupyterlab/cells": "^1.0.0-alpha.3", "@jupyterlab/codeeditor": "^1.0.0-alpha.3", "@jupyterlab/coreutils": "^3.0.0-alpha.3", + "@jupyterlab/docmanager": "^1.0.0-alpha.3", "@jupyterlab/filebrowser": "^1.0.0-alpha.3", "@jupyterlab/launcher": "^1.0.0-alpha.3", "@jupyterlab/mainmenu": "^1.0.0-alpha.3", @@ -46,6 +47,7 @@ "@phosphor/coreutils": "^1.3.0", "@phosphor/disposable": "^1.1.2", "@phosphor/messaging": "^1.2.2", + "@phosphor/properties": "^1.1.2", "@phosphor/widgets": "^1.6.0" }, "devDependencies": { diff --git a/packages/notebook-extension/src/index.ts b/packages/notebook-extension/src/index.ts index 097b06a11bc6..8a72b1e935c6 100644 --- a/packages/notebook-extension/src/index.ts +++ b/packages/notebook-extension/src/index.ts @@ -11,6 +11,7 @@ import { import { Dialog, ICommandPalette, + InstanceTracker, MainAreaWidget, showDialog } from '@jupyterlab/apputils'; @@ -26,7 +27,9 @@ import { URLExt } from '@jupyterlab/coreutils'; -import { UUID } from '@phosphor/coreutils'; +import { IDocumentManager } from '@jupyterlab/docmanager'; + +import { JSONObject, UUID } from '@phosphor/coreutils'; import { DisposableSet } from '@phosphor/disposable'; @@ -68,6 +71,8 @@ import { ReadonlyJSONObject, JSONValue } from '@phosphor/coreutils'; import { Message, MessageLoop } from '@phosphor/messaging'; +import { AttachedProperty } from '@phosphor/properties'; + import { Menu } from '@phosphor/widgets'; /** @@ -251,6 +256,7 @@ const trackerPlugin: JupyterFrontEndPlugin = { provides: INotebookTracker, requires: [ NotebookPanel.IContentFactory, + IDocumentManager, IEditorServices, IRenderMimeRegistry ], @@ -480,6 +486,7 @@ function activateCellTools( function activateNotebookHandler( app: JupyterFrontEnd, contentFactory: NotebookPanel.IContentFactory, + docManager: IDocumentManager, editorServices: IEditorServices, rendermime: IRenderMimeRegistry, palette: ICommandPalette | null, @@ -508,6 +515,9 @@ function activateNotebookHandler( }); const { commands } = app; const tracker = new NotebookTracker({ namespace: 'notebook' }); + const clonedOutputs = new InstanceTracker({ + namespace: 'cloned-outputs' + }); // Handle state restoration. if (restorer) { @@ -517,13 +527,22 @@ function activateNotebookHandler( name: panel => panel.context.path, when: services.ready }); + restorer.restore(clonedOutputs, { + command: CommandIDs.createOutputView, + args: widget => Private.clonedOutputArgs.get(widget), + name: widget => { + const args = Private.clonedOutputArgs.get(widget); + return `${args.path}:${args.index}`; + }, + when: tracker.restored + }); } let registry = app.docRegistry; registry.addModelFactory(new NotebookModelFactory({})); registry.addWidgetFactory(factory); - addCommands(app, services, tracker); + addCommands(app, docManager, services, tracker, clonedOutputs); if (palette) { populatePalette(palette, services); } @@ -815,8 +834,10 @@ function activateNotebookHandler( */ function addCommands( app: JupyterFrontEnd, + docManager: IDocumentManager, services: ServiceManager, - tracker: NotebookTracker + tracker: NotebookTracker, + clonedOutputs: InstanceTracker ): void { const { commands, shell } = app; @@ -1487,11 +1508,35 @@ function addCommands( }); commands.addCommand(CommandIDs.createOutputView, { label: 'Create New View for Output', - execute: args => { + execute: async args => { + let cell: CodeCell | undefined; + let current: NotebookPanel | undefined; + // If we are given a notebook path and cell index, then + // use that, otherwise use the current active cell. + let path = args.path as string | undefined | null; + let index = args.index as number | undefined | null; + if (path && index !== undefined && index !== null) { + current = docManager.findWidget(path, FACTORY) as NotebookPanel; + if (!current) { + return; + } + await current.context.ready; + const cells = current.content.widgets; + if (cells.length < args.index) { + return; + } + cell = current.content.widgets[index] as CodeCell; + } else { + current = getCurrent({ ...args, activate: false }); + if (!current) { + return; + } + cell = current.content.activeCell as CodeCell; + index = current.content.activeCellIndex; + path = current.context.path; + } // Clone the OutputArea - const current = getCurrent({ ...args, activate: false }); - const nb = current.content; - const content = (nb.activeCell as CodeCell).cloneOutputArea(); + const content = cell.cloneOutputArea(); // Create a MainAreaWidget const widget = new MainAreaWidget({ content }); widget.id = `LinkedOutputView-${UUID.uuid4()}`; @@ -1506,11 +1551,28 @@ function addCommands( mode: 'split-bottom' }); + const updateCloned = () => { + path = current.context.path; + index = current.content.activeCellIndex; + + // Store the args for creating the output area so they + // may be used to restore the cloned output. + Private.clonedOutputArgs.set(widget, { path, index }); + clonedOutputs.save(widget); + }; + updateCloned(); + current.context.pathChanged.connect(updateCloned); + current.content.model.cells.changed.connect(updateCloned); + + // Add the cloned output to the output instance tracker. + clonedOutputs.add(widget); + // Remove the output view if the parent notebook is closed. - nb.disposed.connect( - widget.dispose, - widget - ); + current.content.disposed.connect(() => { + current.context.pathChanged.disconnect(updateCloned); + current.content.model.cells.changed.disconnect(updateCloned); + widget.dispose(); + }); }, isEnabled: isEnabledAndSingleSelected }); @@ -2093,3 +2155,21 @@ function populateMenus( getKernel: current => current.session.kernel } as IHelpMenu.IKernelUser); } + +/** + * A namespace for module private functionality. + */ +namespace Private { + /** + * A property for tracking the arguments used to + * create cloned cell outputs. This is used to restore + * them upon page refresh. + */ + export const clonedOutputArgs = new AttachedProperty< + MainAreaWidget, + JSONObject + >({ + name: 'cloned', + create: () => ({}) + }); +} diff --git a/packages/notebook-extension/tsconfig.json b/packages/notebook-extension/tsconfig.json index 56d36e8af0dd..ff5d92896b36 100644 --- a/packages/notebook-extension/tsconfig.json +++ b/packages/notebook-extension/tsconfig.json @@ -21,6 +21,9 @@ { "path": "../coreutils" }, + { + "path": "../docmanager" + }, { "path": "../filebrowser" }, From 334b57587b9f4564a79a546784d191ca8d0a8045 Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Tue, 12 Feb 2019 20:24:39 -0800 Subject: [PATCH 3/7] Implement a ClonedOutputArea widget that loads a notebook output once it's ready. --- packages/notebook-extension/package.json | 2 +- packages/notebook-extension/src/index.ts | 136 ++++++++++++++++------- 2 files changed, 94 insertions(+), 44 deletions(-) diff --git a/packages/notebook-extension/package.json b/packages/notebook-extension/package.json index 8fb04d29ae46..7112a9e0f4f6 100644 --- a/packages/notebook-extension/package.json +++ b/packages/notebook-extension/package.json @@ -44,10 +44,10 @@ "@jupyterlab/rendermime": "^1.0.0-alpha.3", "@jupyterlab/services": "^4.0.0-alpha.3", "@jupyterlab/statusbar": "^1.0.0-alpha.3", + "@phosphor/algorithm": "^1.1.2", "@phosphor/coreutils": "^1.3.0", "@phosphor/disposable": "^1.1.2", "@phosphor/messaging": "^1.2.2", - "@phosphor/properties": "^1.1.2", "@phosphor/widgets": "^1.6.0" }, "devDependencies": { diff --git a/packages/notebook-extension/src/index.ts b/packages/notebook-extension/src/index.ts index 8a72b1e935c6..0930448ddb0e 100644 --- a/packages/notebook-extension/src/index.ts +++ b/packages/notebook-extension/src/index.ts @@ -29,7 +29,9 @@ import { import { IDocumentManager } from '@jupyterlab/docmanager'; -import { JSONObject, UUID } from '@phosphor/coreutils'; +import { ArrayExt } from '@phosphor/algorithm'; + +import { UUID } from '@phosphor/coreutils'; import { DisposableSet } from '@phosphor/disposable'; @@ -71,9 +73,7 @@ import { ReadonlyJSONObject, JSONValue } from '@phosphor/coreutils'; import { Message, MessageLoop } from '@phosphor/messaging'; -import { AttachedProperty } from '@phosphor/properties'; - -import { Menu } from '@phosphor/widgets'; +import { Panel, Menu } from '@phosphor/widgets'; /** * The command IDs used by the notebook plugin. @@ -515,7 +515,9 @@ function activateNotebookHandler( }); const { commands } = app; const tracker = new NotebookTracker({ namespace: 'notebook' }); - const clonedOutputs = new InstanceTracker({ + const clonedOutputs = new InstanceTracker< + MainAreaWidget + >({ namespace: 'cloned-outputs' }); @@ -529,11 +531,11 @@ function activateNotebookHandler( }); restorer.restore(clonedOutputs, { command: CommandIDs.createOutputView, - args: widget => Private.clonedOutputArgs.get(widget), - name: widget => { - const args = Private.clonedOutputArgs.get(widget); - return `${args.path}:${args.index}`; - }, + args: widget => ({ + path: widget.content.path, + index: widget.content.index + }), + name: widget => `${widget.path}:${widget.index}`, when: tracker.restored }); } @@ -1520,12 +1522,6 @@ function addCommands( if (!current) { return; } - await current.context.ready; - const cells = current.content.widgets; - if (cells.length < args.index) { - return; - } - cell = current.content.widgets[index] as CodeCell; } else { current = getCurrent({ ...args, activate: false }); if (!current) { @@ -1533,34 +1529,22 @@ function addCommands( } cell = current.content.activeCell as CodeCell; index = current.content.activeCellIndex; - path = current.context.path; } - // Clone the OutputArea - const content = cell.cloneOutputArea(); // Create a MainAreaWidget + const content = new Private.ClonedOutputArea({ + notebook: current, + cell, + index + }); const widget = new MainAreaWidget({ content }); - widget.id = `LinkedOutputView-${UUID.uuid4()}`; - widget.title.label = 'Output View'; - widget.title.icon = NOTEBOOK_ICON_CLASS; - widget.title.caption = current.title.label - ? `For Notebook: ${current.title.label}` - : 'For Notebook:'; - widget.addClass('jp-LinkedOutputView'); current.context.addSibling(widget, { ref: current.id, mode: 'split-bottom' }); const updateCloned = () => { - path = current.context.path; - index = current.content.activeCellIndex; - - // Store the args for creating the output area so they - // may be used to restore the cloned output. - Private.clonedOutputArgs.set(widget, { path, index }); clonedOutputs.save(widget); }; - updateCloned(); current.context.pathChanged.connect(updateCloned); current.content.model.cells.changed.connect(updateCloned); @@ -2161,15 +2145,81 @@ function populateMenus( */ namespace Private { /** - * A property for tracking the arguments used to - * create cloned cell outputs. This is used to restore - * them upon page refresh. + * A widget hosting a cloned output area. */ - export const clonedOutputArgs = new AttachedProperty< - MainAreaWidget, - JSONObject - >({ - name: 'cloned', - create: () => ({}) - }); + export class ClonedOutputArea extends Panel { + constructor(options: ClonedOutputArea.IOptions) { + super(); + this._notebook = options.notebook; + this._index = options.index !== undefined ? options.index : -1; + this._cell = options.cell || null; + this.id = `LinkedOutputView-${UUID.uuid4()}`; + this.title.label = 'Output View'; + this.title.icon = NOTEBOOK_ICON_CLASS; + this.title.caption = this._notebook.title.label + ? `For Notebook: ${this._notebook.title.label}` + : 'For Notebook:'; + this.addClass('jp-LinkedOutputView'); + + // Wait for the notebook to be loaded before + // cloning the output area. + this._notebook.context.ready.then(() => { + if (!this._cell) { + this._cell = this._notebook.content.widgets[this._index] as CodeCell; + } + if (!this._cell || this._cell.model.type !== 'code') { + this.dispose(); + return; + } + const clone = this._cell.cloneOutputArea(); + this.addWidget(clone); + }); + } + + /** + * The index of the cell in the notebook. + */ + get index(): number { + return this._cell + ? ArrayExt.findFirstIndex( + this._notebook.content.widgets, + c => c === this._cell + ) + : this._index; + } + + /** + * The path of the notebook for the cloned output area. + */ + get path(): string { + return this._notebook.context.path; + } + + private _notebook: NotebookPanel; + private _index: number; + private _cell: CodeCell | null = null; + } + + /** + * ClonedOutputArea statics. + */ + export namespace ClonedOutputArea { + export interface IOptions { + /** + * The notebook associated with the cloned output area. + */ + notebook: NotebookPanel; + + /** + * The cell for which to clone the output area. + */ + cell?: CodeCell; + + /** + * If the cell is not available, provide the index + * of the cell for when the notebook is loaded. + */ + index?: number; + } + } } From 0ea402171f81fdebbd677bed083c3d931dd65142 Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Tue, 12 Feb 2019 20:37:45 -0800 Subject: [PATCH 4/7] Fix name for restoration. --- packages/notebook-extension/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/notebook-extension/src/index.ts b/packages/notebook-extension/src/index.ts index 0930448ddb0e..6ae18c41fc19 100644 --- a/packages/notebook-extension/src/index.ts +++ b/packages/notebook-extension/src/index.ts @@ -535,7 +535,7 @@ function activateNotebookHandler( path: widget.content.path, index: widget.content.index }), - name: widget => `${widget.path}:${widget.index}`, + name: widget => `${widget.content.path}:${widget.content.index}`, when: tracker.restored }); } From 56da756d588918a06959bd5b7096a4f9ff87b3d3 Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Wed, 13 Feb 2019 08:27:08 -0800 Subject: [PATCH 5/7] Asyncify the instance tracker restore function. --- packages/apputils/src/instancetracker.ts | 41 +++++++++--------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/packages/apputils/src/instancetracker.ts b/packages/apputils/src/instancetracker.ts index 57322e6f25e5..adfdb6599ec6 100644 --- a/packages/apputils/src/instancetracker.ts +++ b/packages/apputils/src/instancetracker.ts @@ -325,7 +325,7 @@ export class InstanceTracker * multiple instance trackers and, when ready, asks them each to restore their * respective widgets. */ - restore(options: InstanceTracker.IRestoreOptions): Promise { + async restore(options: InstanceTracker.IRestoreOptions): Promise { const { command, registry, state, when } = options; const namespace = this.namespace; const promises = when @@ -334,36 +334,25 @@ export class InstanceTracker this._restore = options; - return Promise.all(promises) - .then(([saved]) => { - return Promise.all( - saved.ids.map((id, index) => { - const value = saved.values[index]; - const args = value && (value as any).data; - if (args === undefined) { - return state.remove(id); - } - - // Execute the command and if it fails, delete the state restore data. - return registry - .execute(command, args) - .catch(() => state.remove(id)); - }) - ); + const [saved] = await Promise.all(promises); + const val = await Promise.all( + saved.ids.map((id, index) => { + const value = saved.values[index]; + const args = value && (value as any).data; + if (args === undefined) { + return state.remove(id); + } + + // Execute the command and if it fails, delete the state restore data. + return registry.execute(command, args).catch(() => state.remove(id)); }) - .then(val => { - this._restored.resolve(void 0); - return val; - }); + ); + this._restored.resolve(void 0); + return val; } /** * A promise resolved when the instance tracker has been restored. - * - * #### Notes - * This promise is not exposed on the IInstanceTracker interface. - * It is intended to allow for the owner/creator of an instance tracker - * to perform additional actions after restoration in specialized use-cases. */ get restored(): Promise { return this._restored.promise; From f737b09f6f9efad204a078ce3f2a01eefb857eea Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Wed, 13 Feb 2019 08:27:28 -0800 Subject: [PATCH 6/7] Add `restored` to the IInstanceTracker interface. --- packages/apputils/src/instancetracker.ts | 13 +++++++++++++ packages/notebook-extension/src/index.ts | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/apputils/src/instancetracker.ts b/packages/apputils/src/instancetracker.ts index adfdb6599ec6..70092b610bda 100644 --- a/packages/apputils/src/instancetracker.ts +++ b/packages/apputils/src/instancetracker.ts @@ -52,6 +52,19 @@ export interface IInstanceTracker extends IDisposable { */ readonly size: number; + /** + * A promise that is resolved when the instance tracker has been + * restored from a serialized state. + * + * #### Notes + * Most client code will not need to use this, since they can wait + * for the whole application to restore. However, if an extension + * wants to perform actions during the application restoration, but + * after the restoration of another instance tracker, they can use + * this promise. + */ + readonly restored: Promise; + /** * Find the first widget in the tracker that satisfies a filter function. * diff --git a/packages/notebook-extension/src/index.ts b/packages/notebook-extension/src/index.ts index 6ae18c41fc19..2b38c04977cc 100644 --- a/packages/notebook-extension/src/index.ts +++ b/packages/notebook-extension/src/index.ts @@ -536,7 +536,7 @@ function activateNotebookHandler( index: widget.content.index }), name: widget => `${widget.content.path}:${widget.content.index}`, - when: tracker.restored + when: tracker.restored // After the notebook widgets (but not contents). }); } From bf70f2c9acfcb01a31b5fa692a42b9348ebab7b7 Mon Sep 17 00:00:00 2001 From: Ian Rose Date: Wed, 13 Feb 2019 08:40:22 -0800 Subject: [PATCH 7/7] Address review comments. --- packages/apputils/src/instancetracker.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/apputils/src/instancetracker.ts b/packages/apputils/src/instancetracker.ts index 70092b610bda..779359cb8a41 100644 --- a/packages/apputils/src/instancetracker.ts +++ b/packages/apputils/src/instancetracker.ts @@ -339,6 +339,10 @@ export class InstanceTracker * respective widgets. */ async restore(options: InstanceTracker.IRestoreOptions): Promise { + if (this._hasRestored) { + throw new Error('Instance tracker has already restored'); + } + this._hasRestored = true; const { command, registry, state, when } = options; const namespace = this.namespace; const promises = when @@ -348,7 +352,7 @@ export class InstanceTracker this._restore = options; const [saved] = await Promise.all(promises); - const val = await Promise.all( + const values = await Promise.all( saved.ids.map((id, index) => { const value = saved.values[index]; const args = value && (value as any).data; @@ -360,8 +364,8 @@ export class InstanceTracker return registry.execute(command, args).catch(() => state.remove(id)); }) ); - this._restored.resolve(void 0); - return val; + this._restored.resolve(undefined); + return values; } /** @@ -468,6 +472,7 @@ export class InstanceTracker } } + private _hasRestored = false; private _restore: InstanceTracker.IRestoreOptions | null = null; private _restored = new PromiseDelegate(); private _tracker = new FocusTracker();