From 778e7deab0d0202635137754b60115e6cd237121 Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Sat, 21 Dec 2019 12:17:32 +0000 Subject: [PATCH 1/9] Guarantee `titleCase` never fails, even if undefined `str` is passed in. --- packages/coreutils/src/text.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/coreutils/src/text.ts b/packages/coreutils/src/text.ts index 7892a1b11a7e..8778c47801df 100644 --- a/packages/coreutils/src/text.ts +++ b/packages/coreutils/src/text.ts @@ -102,7 +102,7 @@ export namespace Text { * @returns the same string, but with each word capitalized. */ export function titleCase(str: string) { - return str + return (str || '') .toLowerCase() .split(' ') .map(word => word.charAt(0).toUpperCase() + word.slice(1)) From 1b2374296ead12cf399236b774256f94c79aa52e Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Sat, 21 Dec 2019 12:18:00 +0000 Subject: [PATCH 2/9] Make optional dependencies explicitly typed. --- packages/filebrowser-extension/src/index.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/filebrowser-extension/src/index.ts b/packages/filebrowser-extension/src/index.ts index 7fbd542e31f2..0ecfe1d8e71c 100644 --- a/packages/filebrowser-extension/src/index.ts +++ b/packages/filebrowser-extension/src/index.ts @@ -228,10 +228,7 @@ function activateFactory( refreshInterval: options.refreshInterval, state: options.state === null ? undefined : options.state || state }); - const widget = new FileBrowser({ - id, - model - }); + const widget = new FileBrowser({ id, model }); // Add a launcher toolbar item. let launcher = new ToolbarButton({ @@ -263,8 +260,8 @@ function activateBrowser( labShell: ILabShell, restorer: ILayoutRestorer, settingRegistry: ISettingRegistry, - commandPalette: ICommandPalette, - mainMenu: IMainMenu + commandPalette: ICommandPalette | null, + mainMenu: IMainMenu | null ): void { const browser = factory.defaultBrowser; const { commands } = app; From 203832db68f89112bdeaddd1f8f9045647892a37 Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Sat, 21 Dec 2019 13:04:46 +0000 Subject: [PATCH 3/9] Add `restore` flag to file browser widget instantiation options. --- packages/filebrowser/src/browser.ts | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/filebrowser/src/browser.ts b/packages/filebrowser/src/browser.ts index c40c4facd8dd..d6d2ffa48631 100644 --- a/packages/filebrowser/src/browser.ts +++ b/packages/filebrowser/src/browser.ts @@ -64,19 +64,17 @@ export class FileBrowser extends Widget { this._manager = model.manager; this._crumbs = new BreadCrumbs({ model }); this.toolbar = new Toolbar(); - this._directoryPending = false; - let newFolder = new ToolbarButton({ + + const newFolder = new ToolbarButton({ iconClassName: 'jp-NewFolderIcon', onClick: () => { this.createNewDirectory(); }, tooltip: 'New Folder' }); - - let uploader = new Uploader({ model }); - - let refresher = new ToolbarButton({ + const uploader = new Uploader({ model }); + const refresher = new ToolbarButton({ iconClassName: 'jp-RefreshIcon', onClick: () => { void model.refresh(); @@ -94,13 +92,16 @@ export class FileBrowser extends Widget { this.toolbar.addClass(TOOLBAR_CLASS); this._listing.addClass(LISTING_CLASS); - let layout = new PanelLayout(); + const layout = new PanelLayout(); + layout.addWidget(this.toolbar); layout.addWidget(this._crumbs); layout.addWidget(this._listing); - this.layout = layout; - void model.restore(this.id); + + if (options.restore !== false) { + void model.restore(this.id); + } } /** @@ -309,5 +310,12 @@ export namespace FileBrowser { * The default is a shared instance of `DirListing.Renderer`. */ renderer?: DirListing.IRenderer; + + /** + * Whether a file browser automatically restores state when instantiated. + * + * The default is `true`. + */ + restore?: boolean; } } From faa0226ccf6d59a10358f8eb920bd4945bd910a4 Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Mon, 23 Dec 2019 12:54:53 +0000 Subject: [PATCH 4/9] Create a tree resolver token and interface. --- packages/application/src/frontend.ts | 33 ++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/application/src/frontend.ts b/packages/application/src/frontend.ts index d876aa87edad..ee07b75f5efc 100644 --- a/packages/application/src/frontend.ts +++ b/packages/application/src/frontend.ts @@ -305,6 +305,39 @@ export namespace JupyterFrontEnd { readonly workspaces: string; }; } + + /** + * The application tree resolver token. + * + * #### Notes + * Not all Jupyter front-end applications will have a tree resolver + * implemented on the client-side. This token should not be required as a + * dependency if it is possible to make it an optional dependency. + */ + export const ITreeResolver = new Token( + '@jupyterlab/application:ITreeResolver' + ); + + /** + * An interface for a front-end tree route resolver. + */ + export interface ITreeResolver { + /** + * A promise that resolves to the routed tree paths or null. + */ + readonly paths: Promise; + } + + /** + * A namespace for tree resolver types. + */ + export namespace ITreeResolver { + /** + * The browser and file paths if the tree resolver encountered and handled + * a tree URL or `null` if not. + */ + export type Paths = { browser: string; file: string } | null; + } } /** From f4083170644932e23a28341313d08ccce767b6e0 Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Mon, 23 Dec 2019 12:56:42 +0000 Subject: [PATCH 5/9] Rewrite tree handler plugin to be a tree resolver instead. --- packages/application-extension/package.json | 2 + packages/application-extension/src/index.tsx | 109 ++++++++++++------- 2 files changed, 71 insertions(+), 40 deletions(-) diff --git a/packages/application-extension/package.json b/packages/application-extension/package.json index c2d8a8c86d1b..036bb15cee89 100644 --- a/packages/application-extension/package.json +++ b/packages/application-extension/package.json @@ -40,6 +40,8 @@ "@jupyterlab/apputils": "^2.0.0-alpha.4", "@jupyterlab/coreutils": "^4.0.0-alpha.4", "@lumino/algorithm": "^1.2.1", + "@lumino/coreutils": "^1.4.0", + "@lumino/disposable": "^1.3.2", "@lumino/widgets": "^1.9.4", "react": "~16.9.0" }, diff --git a/packages/application-extension/src/index.tsx b/packages/application-extension/src/index.tsx index 8e2eec0f749e..d73eb8207090 100644 --- a/packages/application-extension/src/index.tsx +++ b/packages/application-extension/src/index.tsx @@ -33,6 +33,10 @@ import { import { each, iter, toArray } from '@lumino/algorithm'; +import { PromiseDelegate } from '@lumino/coreutils'; + +import { DisposableDelegate, DisposableSet } from '@lumino/disposable'; + import { Widget, DockLayout } from '@lumino/widgets'; import * as React from 'react'; @@ -247,63 +251,88 @@ const router: JupyterFrontEndPlugin = { }; /** - * The tree route handler provider. + * The default tree route resolver plugin. */ -const tree: JupyterFrontEndPlugin = { - id: '@jupyterlab/application-extension:tree', +const tree: JupyterFrontEndPlugin = { + id: '@jupyterlab/application-extension:tree-resolver', autoStart: true, requires: [JupyterFrontEnd.IPaths, IRouter, IWindowResolver], + provides: JupyterFrontEnd.ITreeResolver, activate: ( app: JupyterFrontEnd, paths: JupyterFrontEnd.IPaths, router: IRouter, resolver: IWindowResolver - ) => { + ): JupyterFrontEnd.ITreeResolver => { const { commands } = app; const treePattern = new RegExp(`^${paths.urls.tree}([^?]+)`); const workspacePattern = new RegExp( `^${paths.urls.workspaces}/[^?/]+/tree/([^?]+)` ); - - commands.addCommand(CommandIDs.tree, { - execute: async (args: IRouter.ILocation) => { - const treeMatch = args.path.match(treePattern); - const workspaceMatch = args.path.match(workspacePattern); - const match = treeMatch || workspaceMatch; - const path = match ? decodeURI(match[1]) : undefined; - const workspace = PathExt.basename(resolver.name); - const query = URLExt.queryStringToObject(args.search ?? ''); - const fileBrowserPath = query['file-browser-path']; - - // Remove the file browser path from the query string. - delete query['file-browser-path']; - - // Remove the tree portion of the URL. - const url = - (workspaceMatch - ? URLExt.join(paths.urls.workspaces, workspace) - : paths.urls.app) + - URLExt.objectToQueryString(query) + - args.hash; - - router.navigate(url); - - try { - await commands.execute('filebrowser:open-path', { path }); - - if (fileBrowserPath) { - await commands.execute('filebrowser:open-path', { - path: fileBrowserPath - }); + const set = new DisposableSet(); + const delegate = new PromiseDelegate(); + + set.add( + commands.addCommand(CommandIDs.tree, { + execute: async (args: IRouter.ILocation) => { + if (set.isDisposed) { + return; } - } catch (error) { - console.warn('Tree routing failed.', error); + + const treeMatch = args.path.match(treePattern); + const workspaceMatch = args.path.match(workspacePattern); + const match = treeMatch || workspaceMatch; + const file = match ? decodeURI(match[1]) : undefined; + const workspace = PathExt.basename(resolver.name); + const query = URLExt.queryStringToObject(args.search ?? ''); + const browser = query['file-browser-path']; + + // Remove the file browser path from the query string. + delete query['file-browser-path']; + + // Remove the tree portion of the URL. + const url = + (workspaceMatch + ? URLExt.join(paths.urls.workspaces, workspace) + : paths.urls.app) + + URLExt.objectToQueryString(query) + + args.hash; + + // Route to the cleaned URL. + router.navigate(url); + + // Clean up artifacts immediately upon routing. + set.dispose(); + + delegate.resolve({ browser, file }); } + }) + ); + set.add( + router.register({ command: CommandIDs.tree, pattern: treePattern }) + ); + set.add( + router.register({ command: CommandIDs.tree, pattern: workspacePattern }) + ); + + // If a route is handled by the router without the tree command being + // invoked, resolve to `null` and clean up artifacts. + const listener = () => { + console.log('listener in tree plugin', set.isDisposed); + if (set.isDisposed) { + return; } - }); + set.dispose(); + delegate.resolve(null); + }; + router.routed.connect(listener); + set.add( + new DisposableDelegate(() => { + router.routed.disconnect(listener); + }) + ); - router.register({ command: CommandIDs.tree, pattern: treePattern }); - router.register({ command: CommandIDs.tree, pattern: workspacePattern }); + return { paths: delegate.promise }; } }; From ecc5b5f83a1cd7fdacad1703db2bb05ce100206a Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Mon, 23 Dec 2019 12:57:27 +0000 Subject: [PATCH 6/9] Add `populate` flag to file browser restoration. --- packages/filebrowser/src/browser.ts | 4 +--- packages/filebrowser/src/model.ts | 23 +++++++++++++++-------- packages/filebrowser/src/tokens.ts | 18 +++++++++++++----- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/packages/filebrowser/src/browser.ts b/packages/filebrowser/src/browser.ts index d6d2ffa48631..1ff17f4a5f4d 100644 --- a/packages/filebrowser/src/browser.ts +++ b/packages/filebrowser/src/browser.ts @@ -99,9 +99,7 @@ export class FileBrowser extends Widget { layout.addWidget(this._listing); this.layout = layout; - if (options.restore !== false) { - void model.restore(this.id); - } + void model.restore(this.id, options.restore !== false); } /** diff --git a/packages/filebrowser/src/model.ts b/packages/filebrowser/src/model.ts index 45752679dc72..b618e872b816 100644 --- a/packages/filebrowser/src/model.ts +++ b/packages/filebrowser/src/model.ts @@ -338,24 +338,32 @@ export class FileBrowserModel implements IDisposable { * * @param id - The unique ID that is used to construct a state database key. * + * @param populate - If `false`, the restoration ID will be set but the file + * browser state will not be fetched from the state database. + * * @returns A promise when restoration is complete. * * #### Notes * This function will only restore the model *once*. If it is called multiple * times, all subsequent invocations are no-ops. */ - restore(id: string): Promise { + async restore(id: string, populate = true): Promise { + const key = `file-browser-${id}:cwd`; const state = this._state; - const restored = !!this._key; - if (!state || restored) { - return Promise.resolve(void 0); + const restored = this._key !== key; + + // Set the file browser key for state database fetch/save. + this._key = key; + + if (!populate || !state || restored) { + this._restored.resolve(undefined); + return; } const manager = this.manager; - const key = `file-browser-${id}:cwd`; const ready = manager.services.ready; return Promise.all([state.fetch(key), ready]) - .then(([value]) => { + .then(async ([value]) => { if (!value) { this._restored.resolve(undefined); return; @@ -370,9 +378,8 @@ export class FileBrowserModel implements IDisposable { }) .catch(() => state.remove(key)) .then(() => { - this._key = key; this._restored.resolve(undefined); - }); // Set key after restoration is done. + }); } /** diff --git a/packages/filebrowser/src/tokens.ts b/packages/filebrowser/src/tokens.ts index 94daab96c4a0..fa2f4d712d0d 100644 --- a/packages/filebrowser/src/tokens.ts +++ b/packages/filebrowser/src/tokens.ts @@ -78,6 +78,19 @@ export namespace IFileBrowserFactory { */ driveName?: string; + /** + * The time interval for browser refreshing, in ms. + */ + refreshInterval?: number; + + /** + * Whether to restore the file browser state after instantiation. + * + * #### Notes + * The default value is `true`. + */ + restore?: boolean; + /** * The state database to use for saving file browser state and restoring it. * @@ -86,10 +99,5 @@ export namespace IFileBrowserFactory { * database will be automatically passed in and used for state restoration. */ state?: IStateDB | null; - - /** - * The time interval for browser refreshing, in ms. - */ - refreshInterval?: number; } } From d8c72ee0de83c700a56b95c5aa2b01caac7ad4f8 Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Mon, 23 Dec 2019 12:58:19 +0000 Subject: [PATCH 7/9] Use the tree resolver as an optional dependency to populate the default file browser. --- packages/application-extension/src/index.tsx | 1 - packages/filebrowser-extension/src/index.ts | 53 ++++++++++++++++--- .../filebrowser-extension/style/index.css | 4 ++ 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/packages/application-extension/src/index.tsx b/packages/application-extension/src/index.tsx index d73eb8207090..ce75423bb774 100644 --- a/packages/application-extension/src/index.tsx +++ b/packages/application-extension/src/index.tsx @@ -318,7 +318,6 @@ const tree: JupyterFrontEndPlugin = { // If a route is handled by the router without the tree command being // invoked, resolve to `null` and clean up artifacts. const listener = () => { - console.log('listener in tree plugin', set.isDisposed); if (set.isDisposed) { return; } diff --git a/packages/filebrowser-extension/src/index.ts b/packages/filebrowser-extension/src/index.ts index 0ecfe1d8e71c..198668e3b695 100644 --- a/packages/filebrowser-extension/src/index.ts +++ b/packages/filebrowser-extension/src/index.ts @@ -4,6 +4,7 @@ import { ILabShell, ILayoutRestorer, + IRouter, JupyterFrontEnd, JupyterFrontEndPlugin } from '@jupyterlab/application'; @@ -134,7 +135,8 @@ const factory: JupyterFrontEndPlugin = { activate: activateFactory, id: '@jupyterlab/filebrowser-extension:factory', provides: IFileBrowserFactory, - requires: [IIconRegistry, IDocumentManager, IStateDB] + requires: [IIconRegistry, IDocumentManager], + optional: [IStateDB, IRouter, JupyterFrontEnd.ITreeResolver] }; /** @@ -209,12 +211,14 @@ export default plugins; /** * Activate the file browser factory provider. */ -function activateFactory( +async function activateFactory( app: JupyterFrontEnd, icoReg: IIconRegistry, docManager: IDocumentManager, - state: IStateDB -): IFileBrowserFactory { + state: IStateDB | null, + router: IRouter | null, + tree: JupyterFrontEnd.ITreeResolver | null +): Promise { const { commands } = app; const tracker = new WidgetTracker({ namespace }); const createFileBrowser = ( @@ -228,7 +232,8 @@ function activateFactory( refreshInterval: options.refreshInterval, state: options.state === null ? undefined : options.state || state }); - const widget = new FileBrowser({ id, model }); + const restore = options.restore; + const widget = new FileBrowser({ id, model, restore }); // Add a launcher toolbar item. let launcher = new ToolbarButton({ @@ -245,9 +250,43 @@ function activateFactory( return widget; }; - const defaultBrowser = createFileBrowser('filebrowser'); - return { createFileBrowser, defaultBrowser, tracker }; + // Manually restore the default file browser. + const id = 'filebrowser'; + const defaultBrowser = createFileBrowser(id, { restore: false }); + const plugin = { createFileBrowser, defaultBrowser, tracker }; + const restoring = 'jp-mod-restoring'; + + defaultBrowser.addClass(restoring); + + if (!router) { + void defaultBrowser.model.restore(id).then(() => { + defaultBrowser.removeClass(restoring); + }); + return plugin; + } + + const listener = async () => { + router.routed.disconnect(listener); + + const paths = await tree?.paths; + if (paths) { + // Restore the model without populating it. + await defaultBrowser.model.restore(id, false); + if (paths.file) { + await commands.execute(CommandIDs.openPath, { path: paths.file }); + } + if (paths.browser) { + await commands.execute(CommandIDs.openPath, { path: paths.browser }); + } + } else { + await defaultBrowser.model.restore(id); + } + defaultBrowser.removeClass(restoring); + }; + router.routed.connect(listener); + + return plugin; } /** diff --git a/packages/filebrowser-extension/style/index.css b/packages/filebrowser-extension/style/index.css index a8fa0cfabee4..f9a4609659de 100644 --- a/packages/filebrowser-extension/style/index.css +++ b/packages/filebrowser-extension/style/index.css @@ -4,3 +4,7 @@ |----------------------------------------------------------------------------*/ @import url('~@jupyterlab/filebrowser/style/index.css'); + +#filebrowser.jp-mod-restoring .jp-DirListing-content { + display: none; +} From b7fabfa0a030920bfda88a022a2d41d5e366c67b Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Mon, 23 Dec 2019 15:01:16 +0000 Subject: [PATCH 8/9] Refactor file browser restore method and fix instantiation logic. --- packages/filebrowser/src/browser.ts | 4 ++- packages/filebrowser/src/model.ts | 45 ++++++++++++++++------------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/packages/filebrowser/src/browser.ts b/packages/filebrowser/src/browser.ts index 1ff17f4a5f4d..d6d2ffa48631 100644 --- a/packages/filebrowser/src/browser.ts +++ b/packages/filebrowser/src/browser.ts @@ -99,7 +99,9 @@ export class FileBrowser extends Widget { layout.addWidget(this._listing); this.layout = layout; - void model.restore(this.id, options.restore !== false); + if (options.restore !== false) { + void model.restore(this.id); + } } /** diff --git a/packages/filebrowser/src/model.ts b/packages/filebrowser/src/model.ts index b618e872b816..057f69148afb 100644 --- a/packages/filebrowser/src/model.ts +++ b/packages/filebrowser/src/model.ts @@ -348,38 +348,43 @@ export class FileBrowserModel implements IDisposable { * times, all subsequent invocations are no-ops. */ async restore(id: string, populate = true): Promise { + const { manager } = this; const key = `file-browser-${id}:cwd`; const state = this._state; - const restored = this._key !== key; + const restored = !!this._key; + + if (restored) { + return; + } // Set the file browser key for state database fetch/save. this._key = key; - if (!populate || !state || restored) { + if (!populate || !state) { this._restored.resolve(undefined); return; } - const manager = this.manager; - const ready = manager.services.ready; - return Promise.all([state.fetch(key), ready]) - .then(async ([value]) => { - if (!value) { - this._restored.resolve(undefined); - return; - } + await manager.services.ready; - const path = (value as ReadonlyJSONObject)['path'] as string; - const localPath = manager.services.contents.localPath(path); - return manager.services.contents - .get(path) - .then(() => this.cd(localPath)) - .catch(() => state.remove(key)); - }) - .catch(() => state.remove(key)) - .then(() => { + try { + const value = await state.fetch(key); + + if (!value) { this._restored.resolve(undefined); - }); + return; + } + + const path = (value as ReadonlyJSONObject)['path'] as string; + const localPath = manager.services.contents.localPath(path); + + await manager.services.contents.get(path); + await this.cd(localPath); + } catch (error) { + await state.remove(key); + } + + this._restored.resolve(undefined); } /** From 6e0b32e4b86d950dd2a879b7a80a57c66481b28c Mon Sep 17 00:00:00 2001 From: "Afshin T. Darian" Date: Mon, 23 Dec 2019 16:16:22 +0000 Subject: [PATCH 9/9] Fix strict null check errors. --- packages/application-extension/src/index.tsx | 4 ++-- packages/application/src/frontend.ts | 2 +- packages/filebrowser-extension/src/index.ts | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/application-extension/src/index.tsx b/packages/application-extension/src/index.tsx index ce75423bb774..14e5fabd7cdf 100644 --- a/packages/application-extension/src/index.tsx +++ b/packages/application-extension/src/index.tsx @@ -282,10 +282,10 @@ const tree: JupyterFrontEndPlugin = { const treeMatch = args.path.match(treePattern); const workspaceMatch = args.path.match(workspacePattern); const match = treeMatch || workspaceMatch; - const file = match ? decodeURI(match[1]) : undefined; + const file = match ? decodeURI(match[1]) : ''; const workspace = PathExt.basename(resolver.name); const query = URLExt.queryStringToObject(args.search ?? ''); - const browser = query['file-browser-path']; + const browser = query['file-browser-path'] || ''; // Remove the file browser path from the query string. delete query['file-browser-path']; diff --git a/packages/application/src/frontend.ts b/packages/application/src/frontend.ts index ee07b75f5efc..b615b0459a6c 100644 --- a/packages/application/src/frontend.ts +++ b/packages/application/src/frontend.ts @@ -334,7 +334,7 @@ export namespace JupyterFrontEnd { export namespace ITreeResolver { /** * The browser and file paths if the tree resolver encountered and handled - * a tree URL or `null` if not. + * a tree URL or `null` if not. Empty string paths should be ignored. */ export type Paths = { browser: string; file: string } | null; } diff --git a/packages/filebrowser-extension/src/index.ts b/packages/filebrowser-extension/src/index.ts index 198668e3b695..514c4317c12a 100644 --- a/packages/filebrowser-extension/src/index.ts +++ b/packages/filebrowser-extension/src/index.ts @@ -230,7 +230,8 @@ async function activateFactory( manager: docManager, driveName: options.driveName || '', refreshInterval: options.refreshInterval, - state: options.state === null ? undefined : options.state || state + state: + options.state === null ? undefined : options.state || state || undefined }); const restore = options.restore; const widget = new FileBrowser({ id, model, restore });