From e3367714e5fb766427d951f5efba9007ec8cd5f0 Mon Sep 17 00:00:00 2001 From: Sandeep Somavarapu Date: Tue, 9 Jan 2018 18:47:11 +0100 Subject: [PATCH] Fix #40018 --- src/vs/workbench/api/node/extHostTreeViews.ts | 169 ++++++++---------- .../api/extHostTreeViews.test.ts | 20 +-- 2 files changed, 82 insertions(+), 107 deletions(-) diff --git a/src/vs/workbench/api/node/extHostTreeViews.ts b/src/vs/workbench/api/node/extHostTreeViews.ts index ad9610e5fb209..5ebf180db38c1 100644 --- a/src/vs/workbench/api/node/extHostTreeViews.ts +++ b/src/vs/workbench/api/node/extHostTreeViews.ts @@ -14,6 +14,7 @@ import { ExtHostTreeViewsShape, MainThreadTreeViewsShape } from './extHost.proto import { ITreeItem, TreeViewItemHandleArg } from 'vs/workbench/common/views'; import { ExtHostCommands, CommandsConverter } from 'vs/workbench/api/node/extHostCommands'; import { asWinJsPromise } from 'vs/base/common/async'; +import { coalesce } from 'vs/base/common/arrays'; type TreeItemHandle = string; @@ -51,7 +52,7 @@ export class ExtHostTreeViews implements ExtHostTreeViewsShape { if (!treeView) { return TPromise.wrapError(new Error(localize('treeView.notRegistered', 'No tree view with id \'{0}\' registered.', treeViewId))); } - return treeView.getTreeItems(); + return treeView.getChildren(); } $getChildren(treeViewId: string, treeItemHandle?: string): TPromise { @@ -69,10 +70,9 @@ export class ExtHostTreeViews implements ExtHostTreeViewsShape { } interface TreeNode { - index: number; handle: TreeItemHandle; - parent: TreeItemHandle; - children: TreeItemHandle[]; + parentHandle: TreeItemHandle; + childrenHandles: TreeItemHandle[]; } class ExtHostTreeView extends Disposable { @@ -89,25 +89,37 @@ class ExtHostTreeView extends Disposable { } } - getTreeItems(): TPromise { - this.clearAll(); - return asWinJsPromise(() => this.dataProvider.getChildren()) - .then(elements => this.resolveElements(elements)); - } - - getChildren(treeItemHandle: TreeItemHandle): TPromise { - let extElement = this.getExtensionElement(treeItemHandle); - if (extElement) { - this.clearChildren(extElement); - } else { - return TPromise.wrapError(new Error(localize('treeItem.notFound', 'No tree item with id \'{0}\' found.', treeItemHandle))); + getChildren(parentHandle?: TreeItemHandle): TPromise { + let parentElement; + if (parentHandle) { + parentElement = this.getExtensionElement(parentHandle); + if (!parentElement) { + return TPromise.wrapError(new Error(localize('treeItem.notFound', 'No tree item with id \'{0}\' found.', parentHandle))); + } } - return asWinJsPromise(() => this.dataProvider.getChildren(extElement)) - .then(childrenElements => this.resolveElements(childrenElements, treeItemHandle)) - .then(childrenItems => { - this.nodes.get(extElement).children = childrenItems.map(c => c.handle); - return childrenItems; + return asWinJsPromise(() => this.dataProvider.getChildren(parentElement)) + .then(elements => { + elements = coalesce(elements || []); + return TPromise.join(elements.map((element, index) => { + const node = this.nodes.get(element); + const currentHandle = node && node.parentHandle === parentHandle ? node.handle : void 0; + return this.resolveElement(element, currentHandle ? currentHandle : index, parentHandle) + .then(treeItem => { + if (treeItem) { + if (!currentHandle) { + // update the caches if current handle is not used + this.nodes.set(element, { + handle: treeItem.handle, + parentHandle, + childrenHandles: void 0 + }); + this.elements.set(treeItem.handle, element); + } + } + return treeItem; + }); + })).then(treeItems => this.updateChildren(coalesce(treeItems), parentElement)); }); } @@ -120,52 +132,29 @@ class ExtHostTreeView extends Disposable { if (hasRoot) { this.proxy.$refresh(this.viewId); } else { - const handlesToUpdate = this.getHandlesToUpdate(elements); - if (handlesToUpdate.length) { - this._refreshHandles(handlesToUpdate); + const handlesToRefresh = this.getHandlesToRefresh(elements); + if (handlesToRefresh.length) { + this.refreshHandles(handlesToRefresh); } } } - private resolveElements(elements: T[], parentHandle?: TreeItemHandle): TPromise { - if (elements && elements.length) { - return TPromise.join( - elements.filter(element => !!element) - .map((element, index) => { - return this.resolveElement(element, index, parentHandle) - .then(treeItem => { - if (treeItem) { - this.nodes.set(element, { - index, - handle: treeItem.handle, - parent: parentHandle, - children: void 0 - }); - if (this.elements.has(treeItem.handle)) { - return TPromise.wrapError(new Error(localize('treeView.duplicateElement', 'Element {0} is already registered', element))); - } - this.elements.set(treeItem.handle, element); - } - return treeItem; - }); - })) - .then(treeItems => treeItems.filter(treeItem => !!treeItem)); - } - return TPromise.as([]); - } - - private resolveElement(element: T, index: number, parentHandle?: TreeItemHandle): TPromise { + private resolveElement(element: T, handleOrIndex: TreeItemHandle | number, parentHandle: TreeItemHandle): TPromise { return asWinJsPromise(() => this.dataProvider.getTreeItem(element)) - .then(extTreeItem => this.massageTreeItem(element, extTreeItem, index, parentHandle)); + .then(extTreeItem => this.massageTreeItem(element, extTreeItem, handleOrIndex, parentHandle)); } - private massageTreeItem(element: T, extensionTreeItem: vscode.TreeItem, index: number, parentHandle: TreeItemHandle): ITreeItem { + private massageTreeItem(element: T, extensionTreeItem: vscode.TreeItem, handleOrIndex: TreeItemHandle | number, parentHandle: TreeItemHandle): ITreeItem { if (!extensionTreeItem) { return null; } + const icon = this.getLightIconPath(extensionTreeItem); const label = extensionTreeItem.label; - const handle = typeof element === 'string' ? element : this.generateHandle(label, index, parentHandle); + const handle = typeof handleOrIndex === 'number' ? + this.generateHandle(label, handleOrIndex, parentHandle) // create the handle + : handleOrIndex; // reuse the passed handle + return { handle, parentHandle, @@ -208,18 +197,18 @@ class ExtHostTreeView extends Disposable { return URI.file(iconPath).toString(); } - private getHandlesToUpdate(elements: T[]): TreeItemHandle[] { + private getHandlesToRefresh(elements: T[]): TreeItemHandle[] { const elementsToUpdate = new Set(); for (const element of elements) { let elementNode = this.nodes.get(element); if (elementNode && !elementsToUpdate.has(elementNode.handle)) { // check if an ancestor of extElement is already in the elements to update list let currentNode = elementNode; - while (currentNode && currentNode.parent && !elementsToUpdate.has(currentNode.parent)) { - const parentElement = this.elements.get(currentNode.parent); + while (currentNode && currentNode.parentHandle && !elementsToUpdate.has(currentNode.parentHandle)) { + const parentElement = this.elements.get(currentNode.parentHandle); currentNode = this.nodes.get(parentElement); } - if (!currentNode.parent) { + if (!currentNode.parentHandle) { elementsToUpdate.add(elementNode.handle); } } @@ -230,7 +219,7 @@ class ExtHostTreeView extends Disposable { elementsToUpdate.forEach((handle) => { const element = this.elements.get(handle); let node = this.nodes.get(element); - if (node && !elementsToUpdate.has(node.parent)) { + if (node && !elementsToUpdate.has(node.parentHandle)) { handlesToUpdate.push(handle); } }); @@ -238,21 +227,16 @@ class ExtHostTreeView extends Disposable { return handlesToUpdate; } - private _refreshHandles(itemHandles: TreeItemHandle[]): TPromise { + private refreshHandles(itemHandles: TreeItemHandle[]): TPromise { const itemsToRefresh: { [handle: string]: ITreeItem } = {}; const promises: TPromise[] = []; itemHandles.forEach(treeItemHandle => { const extElement = this.getExtensionElement(treeItemHandle); const node = this.nodes.get(extElement); - const promise = this.resolveElement(extElement, node.index, node.parent) + promises.push(this.resolveElement(extElement, treeItemHandle, node.parentHandle) .then(treeItem => { - if (treeItemHandle !== treeItem.handle) { - // Update caches if handle changes - this.updateCaches(node, treeItem, extElement); - } itemsToRefresh[treeItemHandle] = treeItem; - }); - promises.push(promise); + })); }); return TPromise.join(promises) .then(treeItems => { @@ -260,39 +244,34 @@ class ExtHostTreeView extends Disposable { }); } - private updateCaches(node: TreeNode, treeItem: ITreeItem, element: T): void { - if (node.parent) { - // Update parent's children handles - const parentElement = this.getExtensionElement(node.parent); + private updateChildren(newChildren: ITreeItem[], parentElement?: T): ITreeItem[] { + let existingChildrenHandles: TreeItemHandle[] = []; + if (parentElement) { const parentNode = this.nodes.get(parentElement); - parentNode.children[node.index] = treeItem.handle; + existingChildrenHandles = parentNode.childrenHandles || []; + parentNode.childrenHandles = newChildren.map(c => c.handle); + } else { + this.nodes.forEach(node => { + if (!node.parentHandle) { + existingChildrenHandles.push(node.handle); + } + }); } - // Update elements map - this.elements.delete(node.handle); - this.elements.set(treeItem.handle, element); - - // Update node - node.handle = treeItem.handle; - } - - private clearChildren(element: T): void { - let node = this.nodes.get(element); - if (node.children) { - for (const childHandle of node.children) { - const childEleement = this.elements.get(childHandle); - if (childEleement) { - this.clear(childEleement); - } + for (const existingChildHandle of existingChildrenHandles) { + const existingChildElement = this.elements.get(existingChildHandle); + if (existingChildElement && newChildren.every(c => c.handle !== existingChildHandle)) { + this.clear(existingChildElement); } } - node.children = void 0; + + return newChildren; } private clear(element: T): void { let node = this.nodes.get(element); - if (node.children) { - for (const childHandle of node.children) { + if (node.childrenHandles) { + for (const childHandle of node.childrenHandles) { const childEleement = this.elements.get(childHandle); if (childEleement) { this.clear(childEleement); @@ -303,12 +282,8 @@ class ExtHostTreeView extends Disposable { this.elements.delete(node.handle); } - private clearAll(): void { + dispose() { this.elements.clear(); this.nodes.clear(); } - - dispose() { - this.clearAll(); - } } \ No newline at end of file diff --git a/src/vs/workbench/test/electron-browser/api/extHostTreeViews.test.ts b/src/vs/workbench/test/electron-browser/api/extHostTreeViews.test.ts index b62cac7b7e000..1a692f0c257d5 100644 --- a/src/vs/workbench/test/electron-browser/api/extHostTreeViews.test.ts +++ b/src/vs/workbench/test/electron-browser/api/extHostTreeViews.test.ts @@ -111,24 +111,24 @@ suite('ExtHostTreeView', function () { return testObject.$getElements('testStringTreeProvider') .then(elements => { const actuals = elements.map(e => e.handle); - assert.deepEqual(actuals, ['a', 'b']); + assert.deepEqual(actuals, ['0/0:a', '0/1:b']); return TPromise.join([ - testObject.$getChildren('testStringTreeProvider', 'a') + testObject.$getChildren('testStringTreeProvider', '0/0:a') .then(children => { const actuals = children.map(e => e.handle); - assert.deepEqual(actuals, ['aa', 'ab']); + assert.deepEqual(actuals, ['0/0:a/0:aa', '0/0:a/1:ab']); return TPromise.join([ - testObject.$getChildren('testStringTreeProvider', 'aa').then(children => assert.equal(children.length, 0)), - testObject.$getChildren('testStringTreeProvider', 'ab').then(children => assert.equal(children.length, 0)) + testObject.$getChildren('testStringTreeProvider', '0/0:a/0:aa').then(children => assert.equal(children.length, 0)), + testObject.$getChildren('testStringTreeProvider', '0/0:a/1:ab').then(children => assert.equal(children.length, 0)) ]); }), - testObject.$getChildren('testStringTreeProvider', 'b') + testObject.$getChildren('testStringTreeProvider', '0/1:b') .then(children => { const actuals = children.map(e => e.handle); - assert.deepEqual(actuals, ['ba', 'bb']); + assert.deepEqual(actuals, ['0/1:b/0:ba', '0/1:b/1:bb']); return TPromise.join([ - testObject.$getChildren('testStringTreeProvider', 'ba').then(children => assert.equal(children.length, 0)), - testObject.$getChildren('testStringTreeProvider', 'bb').then(children => assert.equal(children.length, 0)) + testObject.$getChildren('testStringTreeProvider', '0/1:b/0:ba').then(children => assert.equal(children.length, 0)), + testObject.$getChildren('testStringTreeProvider', '0/1:b/1:bb').then(children => assert.equal(children.length, 0)) ]); }) ]); @@ -213,7 +213,7 @@ suite('ExtHostTreeView', function () { target.onRefresh.event(actuals => { assert.deepEqual(['0/0:a'], Object.keys(actuals)); assert.deepEqual(removeUnsetKeys(actuals['0/0:a']), { - handle: '0/0:aa', + handle: '0/0:a', label: 'aa', }); done();