From 13ad4c94979f91091aa854155ef39c3350bbe7c9 Mon Sep 17 00:00:00 2001 From: Maddy <12754347+MaddyDev@users.noreply.github.com> Date: Fri, 2 Apr 2021 19:33:41 -0700 Subject: [PATCH] Select notebook in viewlet when opened in editor (#14854) * test changes * code clean up * remove extra line * remove commented code * remove unnecessary import * update expand logic * implement parent,children on bookitem * merge conflicts * refactor code * fix trustBook test * typo on condition * feedback * indexOf to include * fix undefined error on expand * remove set parent * revert getTreeItem logic * Fix duplicated nodes * remove debug * Clean up logic * Fix tests * Fix logic * cleanup * Cleanup findAndExpandParentNode (#14960) Co-authored-by: chgagnon --- extensions/notebook/src/book/bookModel.ts | 64 +++++++--- extensions/notebook/src/book/bookTreeItem.ts | 20 ++- extensions/notebook/src/book/bookTreeView.ts | 114 +++++++++++------- extensions/notebook/src/extension.ts | 4 +- .../notebook/src/test/book/book.test.ts | 15 +-- 5 files changed, 150 insertions(+), 67 deletions(-) diff --git a/extensions/notebook/src/book/bookModel.ts b/extensions/notebook/src/book/bookModel.ts index 4c9b9ca8c4..407c2a4b7d 100644 --- a/extensions/notebook/src/book/bookModel.ts +++ b/extensions/notebook/src/book/bookModel.ts @@ -19,7 +19,7 @@ const fsPromises = fileServices.promises; const content = 'content'; export class BookModel { - private _bookItems: BookTreeItem[]; + private _bookItems: BookTreeItem[] = []; private _allNotebooks = new Map(); private _tableOfContentsPath: string; private _contentFolderPath: string; @@ -28,6 +28,10 @@ export class BookModel { private _errorMessage: string; private _activePromise: Deferred | undefined = undefined; private _queuedPromises: Deferred[] = []; + /** + * The root tree item for this model + */ + private _rootNode: BookTreeItem; constructor( public readonly bookPath: string, @@ -35,9 +39,7 @@ export class BookModel { public readonly isNotebook: boolean, private _extensionContext: vscode.ExtensionContext, private _onDidChangeTreeData: vscode.EventEmitter, - public readonly notebookRootPath?: string) { - this._bookItems = []; - } + public readonly notebookRootPath?: string) { } public unwatchTOC(): void { fs.unwatchFile(this.tableOfContentsPath); @@ -154,6 +156,7 @@ export class BookModel { } ); this._bookItems.push(notebookItem); + this._rootNode = notebookItem; if (this.openAsUntitled && !this._allNotebooks.get(pathDetails.base)) { this._allNotebooks.set(pathDetails.base, notebookItem); } else { @@ -199,6 +202,7 @@ export class BookModel { dark: this._extensionContext.asAbsolutePath('resources/dark/book_inverse.svg') } ); + this._rootNode = book; this._bookItems.push(book); } catch (e) { this._errorMessage = loc.readBookError(this.bookPath, e instanceof Error ? e.message : e); @@ -212,8 +216,21 @@ export class BookModel { return this._bookItems; } - public async getSections(tableOfContents: IJupyterBookToc, sections: JupyterBookSection[], root: string, book: BookTreeItemFormat): Promise { - let notebooks: BookTreeItem[] = []; + public set bookItems(bookItems: BookTreeItem[]) { + bookItems.forEach(b => { + // only add unique notebooks + if (!this._bookItems.includes(b)) { + this._bookItems.push(b); + } + }); + } + + public async getSections(element: BookTreeItem): Promise { + let tableOfContents: IJupyterBookToc = element.tableOfContents; + let sections: JupyterBookSection[] = element.sections; + let root: string = element.root; + let book: BookTreeItemFormat = element.book; + let treeItems: BookTreeItem[] = []; for (let i = 0; i < sections.length; i++) { if (sections[i].url) { let externalLink: BookTreeItem = new BookTreeItem({ @@ -225,7 +242,8 @@ export class BookModel { type: BookTreeItemType.ExternalLink, treeItemCollapsibleState: vscode.TreeItemCollapsibleState.Collapsed, isUntitled: this.openAsUntitled, - version: book.version + version: book.version, + parent: element }, { light: this._extensionContext.asAbsolutePath('resources/light/link.svg'), @@ -233,7 +251,7 @@ export class BookModel { } ); - notebooks.push(externalLink); + treeItems.push(externalLink); } else if (sections[i].file) { const pathToNotebook: string = getContentPath(book.version, book.root, sections[i].file.concat('.ipynb')); const pathToMarkdown: string = getContentPath(book.version, book.root, sections[i].file.concat('.md')); @@ -250,7 +268,8 @@ export class BookModel { type: BookTreeItemType.Notebook, treeItemCollapsibleState: vscode.TreeItemCollapsibleState.Collapsed, isUntitled: this.openAsUntitled, - version: book.version + version: book.version, + parent: element }, { light: this._extensionContext.asAbsolutePath('resources/light/notebook.svg'), @@ -262,14 +281,14 @@ export class BookModel { if (!this._allNotebooks.get(path.basename(pathToNotebook))) { this._allNotebooks.set(path.basename(pathToNotebook), notebook); } - notebooks.push(notebook); + treeItems.push(notebook); } else { // convert to URI to avoid causing issue with drive letters when getting navigation links let uriToNotebook: vscode.Uri = vscode.Uri.file(pathToNotebook); if (!this._allNotebooks.get(uriToNotebook.fsPath)) { this._allNotebooks.set(uriToNotebook.fsPath, notebook); } - notebooks.push(notebook); + treeItems.push(notebook); } } else if (await fs.pathExists(pathToMarkdown)) { let markdown: BookTreeItem = new BookTreeItem({ @@ -281,21 +300,35 @@ export class BookModel { type: BookTreeItemType.Markdown, treeItemCollapsibleState: vscode.TreeItemCollapsibleState.Collapsed, isUntitled: this.openAsUntitled, - version: book.version + version: book.version, + parent: element }, { light: this._extensionContext.asAbsolutePath('resources/light/markdown.svg'), dark: this._extensionContext.asAbsolutePath('resources/dark/markdown_inverse.svg') } ); - notebooks.push(markdown); + if (this.openAsUntitled) { + if (!this._allNotebooks.get(path.basename(pathToMarkdown))) { + this._allNotebooks.set(path.basename(pathToMarkdown), markdown); + } + } else { + // convert to URI to avoid causing issue with drive letters when getting navigation links + let uriToNotebook: vscode.Uri = vscode.Uri.file(pathToMarkdown); + if (!this._allNotebooks.get(uriToNotebook.fsPath)) { + this._allNotebooks.set(uriToNotebook.fsPath, markdown); + } + } + treeItems.push(markdown); } else { this._errorMessage = loc.missingFileError(sections[i].title, book.title); vscode.window.showErrorMessage(this._errorMessage); } } } - return notebooks; + element.children = treeItems; + this.bookItems = treeItems; + return treeItems; } /** @@ -334,4 +367,7 @@ export class BookModel { public get version(): BookVersion { return this._bookVersion; } + public get rootNode(): BookTreeItem { + return this._rootNode; + } } diff --git a/extensions/notebook/src/book/bookTreeItem.ts b/extensions/notebook/src/book/bookTreeItem.ts index a2ff98d17d..d96516730d 100644 --- a/extensions/notebook/src/book/bookTreeItem.ts +++ b/extensions/notebook/src/book/bookTreeItem.ts @@ -34,10 +34,12 @@ export interface BookTreeItemFormat { treeItemCollapsibleState: number; isUntitled: boolean; version?: BookVersion; + parent?: BookTreeItem; + children?: BookTreeItem[]; } export class BookTreeItem extends vscode.TreeItem { - private _sections: JupyterBookSection[]; + private _sections: JupyterBookSection[] | undefined; private _uri: string | undefined; private _previousUri: string; private _nextUri: string; @@ -204,6 +206,22 @@ export class BookTreeItem extends vscode.TreeItem { this._tableOfContentsPath = tocPath; } + public get children(): BookTreeItem[] | undefined { + return this.book.children; + } + + public set children(children: BookTreeItem[] | undefined) { + this.book.children = children; + } + + public get parent(): BookTreeItem { + return this.book.parent; + } + + public set parent(parent: BookTreeItem) { + this.book.parent = parent; + } + /** * Helper method to find a child section with a specified URL * @param url The url of the section we're searching for diff --git a/extensions/notebook/src/book/bookTreeView.ts b/extensions/notebook/src/book/bookTreeView.ts index b555ad0c13..6684711888 100644 --- a/extensions/notebook/src/book/bookTreeView.ts +++ b/extensions/notebook/src/book/bookTreeView.ts @@ -42,20 +42,34 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider; public viewId: string; - public books: BookModel[]; + public books: BookModel[] = []; public currentBook: BookModel; constructor(workspaceFolders: vscode.WorkspaceFolder[], extensionContext: vscode.ExtensionContext, openAsUntitled: boolean, view: string, public providerId: string) { this._openAsUntitled = openAsUntitled; this._extensionContext = extensionContext; - this.books = []; this.bookPinManager = new BookPinManager(); this.viewId = view; this.initialize(workspaceFolders).catch(e => console.error(e)); this.prompter = new CodeAdapter(); this._bookTrustManager = new BookTrustManager(this.books); this.bookTocManager = new BookTocManager(); - + this._bookViewer = vscode.window.createTreeView(this.viewId, { showCollapseAll: true, treeDataProvider: this }); + this._bookViewer.onDidChangeVisibility(async e => { + // Whenever the viewer changes visibility then try and reveal the currently active document + // in the tree view + let openDocument = azdata.nb.activeNotebookEditor; + let notebookPath = openDocument?.document.uri; + // Find the book that this notebook belongs to + const book = this.books.find(book => notebookPath?.fsPath.replace(/\\/g, '/').indexOf(book.bookPath) >= -1); + // Only reveal if... + if (e.visible && // If the view is currently visible - if not then we'll just wait until this is called when the view is made visible + book && // The notebook is part of a book in the viewlet (otherwise nothing to reveal) + (this._openAsUntitled ? notebookPath?.scheme === 'untitled' : notebookPath?.scheme !== 'untitled')) // The notebook is of the correct type for this tree view + { + await this.revealDocumentInTreeView(notebookPath); + } + }); this._extensionContext.subscriptions.push(azdata.nb.registerNavigationProvider(this)); } @@ -216,10 +230,10 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider book.bookPath === bookPath); } if (showPreview) { + this.currentBook = this.books.find(book => book.bookPath === bookPath); this._bookViewer.reveal(this.currentBook.bookItems[0], { expand: vscode.TreeItemCollapsibleState.Expanded, focus: true, select: true }); await this.showPreviewFile(urlToOpen); } @@ -310,15 +324,7 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { - let openDocument = azdata.nb.activeNotebookEditor; - let notebookPath = openDocument?.document.uri; - // call reveal only once on the correct view - if (e.visible && ((!this._openAsUntitled && notebookPath?.scheme !== 'untitled') || (this._openAsUntitled && notebookPath?.scheme === 'untitled'))) { - this.revealActiveDocumentInViewlet(); - } - }); + this._onDidChangeTreeData.fire(undefined); } } @@ -383,17 +389,17 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { + async revealDocumentInTreeView(uri?: vscode.Uri, shouldReveal: boolean = true): Promise { let bookItem: BookTreeItem; let notebookPath: string; // If no uri is passed in, try to use the current active notebook editor if (!uri) { let openDocument = azdata.nb.activeNotebookEditor; if (openDocument) { - notebookPath = openDocument.document.uri.fsPath; + notebookPath = openDocument.document.uri.fsPath.replace(/\\/g, '/'); } } else if (uri.fsPath) { - notebookPath = uri.fsPath; + notebookPath = uri.fsPath.replace(/\\/g, '/'); } if (shouldReveal || this._bookViewer?.visible) { @@ -407,24 +413,51 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { - let bookItem: BookTreeItem = this.currentBook?.getNotebook(notebookPath); - // if the node is not expanded getNotebook returns undefined, try to expand the parent node or getChildren of - // the root node. - if (!bookItem) { - // get the parent node and expand it if it's not already - let allNodes = this.currentBook?.getAllNotebooks(); - let book = allNodes ? Array.from(allNodes?.keys())?.filter(x => x.indexOf(notebookPath.substring(0, notebookPath.lastIndexOf(path.sep))) > -1) : undefined; - let bookNode = book?.length > 0 ? this.currentBook?.getNotebook(book.find(x => x.substring(0, x.lastIndexOf(path.sep)) === notebookPath.substring(0, notebookPath.lastIndexOf(path.sep)))) : undefined; - if (bookNode) { - if (this._bookViewer?.visible) { - await this._bookViewer.reveal(bookNode, { select: true, focus: false, expand: 3 }); - } else { - await this.getChildren(bookNode); - } - - bookItem = this.currentBook?.getNotebook(notebookPath); + async findAndExpandParentNode(notebookPath: string): Promise { + const parentBook = this.books.find(b => notebookPath.indexOf(b.bookPath) > -1); + if (!parentBook) { + // No parent book, likely because the Notebook is at the top level and not under a Notebook. + // Nothing to expand in that case so just return immediately + return undefined; + } + this.currentBook = parentBook; + let bookItem: BookTreeItem = parentBook.getNotebook(notebookPath); + if (bookItem) { + // We already have the Notebook loaded so just return it immediately + return bookItem; + } + // We couldn't find the Notebook which may mean that we don't have it loaded yet, starting from + // the top we'll expand nodes until we find the parent of the Notebook we're looking for + // get the children of root node and expand the nodes to the notebook level. + await this.getChildren(parentBook.rootNode); + // The path to the parent of the Notebook we're looking for (this is the node we're looking to expand) + const parentPath = notebookPath.substring(0, notebookPath.lastIndexOf(path.posix.sep)); + // Find number of directories between the Notebook path and the root of the book it's contained in + // so we know how many parent nodes to expand + let depthOfNotebookInBook: number = path.relative(notebookPath, parentBook.bookPath).split(path.sep).length; + // Walk the tree, expanding parent nodes as needed to load the child nodes until + // we find the one for our Notebook + while (depthOfNotebookInBook > 0) { + // check if the notebook is available in already expanded levels. + bookItem = parentBook.bookItems.find(b => b.tooltip === notebookPath); + if (bookItem) { + return bookItem; } + // Search for the parent item + // notebook can be inside the same folder as parent and can be in a different folder as well + // so check for both scenarios. + let bookItemToExpand = parentBook.bookItems.find(b => b.tooltip.indexOf(parentPath) > -1) ?? + parentBook.bookItems.find(b => path.relative(notebookPath, b.tooltip)?.split(path.sep)?.length === depthOfNotebookInBook); + if (!bookItemToExpand) { + break; + } + if (!bookItemToExpand.children) { + // We haven't loaded children of this node yet so do that now so we can + // continue expanding and search its children + await this.getChildren(bookItemToExpand); + } + await this._bookViewer.reveal(bookItemToExpand, { select: false, focus: true, expand: 3 }); + depthOfNotebookInBook--; } return bookItem; } @@ -548,7 +581,7 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { let bookCollection = await this.getNotebooksInTree(folderPath); for (let i = 0; i < bookCollection.bookPaths.length; i++) { await this.openBook(bookCollection.bookPaths[i], urlToOpen, showPreview); @@ -604,23 +637,19 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { return element; } getChildren(element?: BookTreeItem): Thenable { if (element) { if (element.sections) { - return Promise.resolve(this.currentBook.getSections(element.tableOfContents, element.sections, element.root, element.book).then(sections => { return sections; })); + return Promise.resolve(this.currentBook.getSections(element)); } else { return Promise.resolve([]); } } else { - let bookItems: BookTreeItem[] = []; - this.books.map(book => { - bookItems = bookItems.concat(book.bookItems); - }); - return Promise.resolve(bookItems); + return Promise.resolve(this.books.map(book => book.rootNode)); } } @@ -630,8 +659,7 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider { - // Remove it for perf issues. - return undefined; + return element?.parent; } getUntitledNotebookUri(resource: string): vscode.Uri { diff --git a/extensions/notebook/src/extension.ts b/extensions/notebook/src/extension.ts index 15fa3f567d..96a6e92cc0 100644 --- a/extensions/notebook/src/extension.ts +++ b/extensions/notebook/src/extension.ts @@ -150,9 +150,9 @@ export async function activate(extensionContext: vscode.ExtensionContext): Promi azdata.nb.onDidChangeActiveNotebookEditor(e => { if (e.document.uri.scheme === 'untitled') { - providedBookTreeViewProvider.revealActiveDocumentInViewlet(e.document.uri, false); + providedBookTreeViewProvider.revealDocumentInTreeView(e.document.uri, false); } else { - bookTreeViewProvider.revealActiveDocumentInViewlet(e.document.uri, false); + bookTreeViewProvider.revealDocumentInTreeView(e.document.uri, false); } }); diff --git a/extensions/notebook/src/test/book/book.test.ts b/extensions/notebook/src/test/book/book.test.ts index 4a5271048d..3e6ae6e2e8 100644 --- a/extensions/notebook/src/test/book/book.test.ts +++ b/extensions/notebook/src/test/book/book.test.ts @@ -184,6 +184,7 @@ describe('BooksTreeViewTests', function () { should(children).be.Array(); should(children.length).equal(1); book = children[0]; + should(book).equal(bookTreeViewProvider.currentBook.rootNode); should(book.title).equal(expectedBook.title); }); @@ -210,13 +211,13 @@ describe('BooksTreeViewTests', function () { }); it('should set notebooks trusted to true on trustBook', async () => { - let notebook1Path = path.join(rootFolderPath, 'Book', 'content', 'notebook1.ipynb'); + let notebook1Path = notebook1.tooltip; let bookTrustManager: BookTrustManager = new BookTrustManager(bookTreeViewProvider.books); - let isTrusted = bookTrustManager.isNotebookTrustedByDefault(vscode.Uri.file(notebook1Path).fsPath); + let isTrusted = bookTrustManager.isNotebookTrustedByDefault(notebook1Path); should(isTrusted).equal(false, 'Notebook should not be trusted by default'); - bookTreeViewProvider.trustBook(bookTreeViewProvider.currentBook.bookItems[0]); - isTrusted = bookTrustManager.isNotebookTrustedByDefault(vscode.Uri.file(notebook1Path).fsPath); + bookTreeViewProvider.trustBook(notebook1); + isTrusted = bookTrustManager.isNotebookTrustedByDefault(notebook1Path); should(isTrusted).equal(true, 'Failed to set trust on trustBook'); }); @@ -252,7 +253,7 @@ describe('BooksTreeViewTests', function () { let notebook2Path = path.join(rootFolderPath, 'Book', 'content', 'notebook2.ipynb'); let notebookUri = vscode.Uri.file(notebook2Path); - let revealActiveDocumentInViewletSpy = sinon.spy(bookTreeViewProvider, 'revealActiveDocumentInViewlet'); + let revealActiveDocumentInViewletSpy = sinon.spy(bookTreeViewProvider, 'revealDocumentInTreeView'); await azdata.nb.showNotebookDocument(notebookUri); should(azdata.nb.notebookDocuments.find(doc => doc.fileName === notebookUri.fsPath)).not.be.undefined(); should(revealActiveDocumentInViewletSpy.calledOnce).be.true('revealActiveDocumentInViewlet should have been called'); @@ -335,7 +336,7 @@ describe('BooksTreeViewTests', function () { const untitledNotebook1Uri = vscode.Uri.parse(`untitled:notebook1.ipynb`); const untitledNotebook2Uri = vscode.Uri.parse(`untitled:notebook2.ipynb`); - let revealActiveDocumentInViewletSpy = sinon.spy(providedbookTreeViewProvider, 'revealActiveDocumentInViewlet'); + let revealActiveDocumentInViewletSpy = sinon.spy(providedbookTreeViewProvider, 'revealDocumentInTreeView'); await azdata.nb.showNotebookDocument(untitledNotebook1Uri); should(azdata.nb.notebookDocuments.find(doc => doc.fileName === untitledNotebook1Uri.fsPath)).not.be.undefined(); should(revealActiveDocumentInViewletSpy.calledOnce).be.true('revealActiveDocumentInViewlet should have been called'); @@ -652,7 +653,7 @@ describe('BooksTreeViewTests', function () { it('should show error if notebook or markdown file is missing', async function (): Promise { let books: BookTreeItem[] = bookTreeViewProvider.currentBook.bookItems; - let children = await bookTreeViewProvider.currentBook.getSections({ sections: [] }, books[0].sections, rootFolderPath, books[0].book); + let children = await bookTreeViewProvider.currentBook.getSections(books[0]); should(bookTreeViewProvider.currentBook.errorMessage).equal('Missing file : Notebook1 from '.concat(bookTreeViewProvider.currentBook.bookItems[0].title)); // rest of book should be detected correctly even with a missing file equalBookItems(children[0], expectedNotebook2);