Fix tree view reveal() behavior #8783
Conversation
|
@kittaakos @vince-fugnitto do one of you has the time to review this? 🙏 |
|
@amiramw, I'll look into the changes today. I tried it yesterday and it worked. 👍 Regarding the API; instead of the paren-chain string array we pass around, can we support the same API as VS Code does? $reveal(treeViewId: string, itemInfo: { item: ITreeItem, parentChain: ITreeItem[] } | undefined, options: IRevealOptions): Promise<void>;Do you know why cannot we have a reference to the |
vince-fugnitto
added
plug-in system
I don't think our tree handling is exact copy of vscode. I did see the parent chain and thought that we need it too. But currently there are a lot of fields in
Because |
| private getTreeItemLabel(treeItem: TreeItem2): string | undefined { | ||
| let label: string | undefined; | ||
| const treeItemLabel: string | TreeItemLabel | undefined = treeItem.label; | ||
| if (typeof treeItemLabel === 'object' && typeof treeItemLabel.label === 'string') { | ||
| label = treeItemLabel.label; | ||
| } else { | ||
| label = treeItem.label; | ||
| } | ||
| return label; | ||
| } |
There was a problem hiding this comment.
Please simplify:
private getTreeItemLabel(treeItem: TreeItem2): string | undefined {
const treeItemLabel: string | TreeItemLabel | undefined = treeItem.label;
if (typeof treeItemLabel === 'object' && typeof treeItemLabel.label === 'string') {
return treeItemLabel.label;
}
return treeItem.label;
}| return idLabel; | ||
| } | ||
|
|
||
| private buildTreeItemId(parentId: string, index: number, idLabel?: string) { |
There was a problem hiding this comment.
Fixed return type.
Changed idLabel to be string | undefined. By vscode API doc id or label must exist but by typescript typing and by developer mistake it means that it can be undefined. in this case I don't mind to keep the value "undefined" in the generated id.
| const parent = this.treeDataProvider.getParent && await this.treeDataProvider.getParent(element); | ||
| const chain = await this.calculateRevealParentChain(parent); | ||
| if (!chain || chain.length === 0) { | ||
| // parents are inconsistent | ||
| return; | ||
| } |
There was a problem hiding this comment.
Please help me to understand this flow, I must be overlooking something:
- the
treeDataProvidersupportsgetParent, - you have any invalid ID, so the parent will resolve to
undefined. - you call
calculateRevealParentChainrecursively withundefined, but that will return with an array with an empty string (chain.length !== 0). Is that expected? Why is it needed to have the empty string. I do not understand that part. Code returns with an empty array.
There was a problem hiding this comment.
Changed now so technical root doesn't get an entry in the array
| if (treeItem.id) { | ||
| return chain.concat(treeItem.id); | ||
| } | ||
| const idLabel = this.getTreeItemIdLabel(treeItem); |
There was a problem hiding this comment.
This should go under if (!children)
| } | ||
| } | ||
| // couldn't calculate consistent parent chain and id | ||
| return; |
There was a problem hiding this comment.
Let's be specific: return undefined. Please fix other places too.
| private async calculateRevealParentChain(element: T | undefined): Promise<string[] | undefined> { | ||
| if (!element) { | ||
| // root | ||
| return ['']; |
There was a problem hiding this comment.
As noted below; why is it [''] instead of []. I do not see where the empty string ID is used. Please explain.
There was a problem hiding this comment.
it is built with '' as root id here:
theia/packages/plugin-ext/src/main/browser/view/tree-views-main.ts
Lines 61 to 68 in 03a8cdb
Also in this file getChildren receives a string parent id and treats empty string as root.
But now that I think of it this technical root is always revealed and expanded so I removed it from the array.
| if (elementId) { | ||
| return this.proxy.$reveal(this.treeViewId, elementId, { | ||
| const elementParentChain = await this.calculateRevealParentChain(element); | ||
| if (elementParentChain) { |
There was a problem hiding this comment.
What should be revealed when the elementParentChain is ['']?
There was a problem hiding this comment.
The root. However i don't think you can trigger a call only with the technical root from vscode API as you get a compile error if you don't pass truthy argument to reveal.
Anyway I removed '' from the parent chain.
| @@ -98,14 +103,17 @@ export class TreeViewsMainImpl implements TreeViewsMain, Disposable { | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| async $reveal(treeViewId: string, treeItemId: string, options: TreeViewRevealOptions): Promise<any> { | |||
| const viewPanel = await this.viewRegistry.openView(treeViewId, { activate: options.focus }); | |||
| async $reveal(treeViewId: string, elementParentChain: string[], options: TreeViewRevealOptions): Promise<any> { | |||
There was a problem hiding this comment.
Please add a single line of comment on what elementParentChain is. The original treeItemId was obvious. Thanks!
There was a problem hiding this comment.
May I suggest a rename to "ancestorItemIds"
There was a problem hiding this comment.
the term parent chain is also used in code source: https://github.com/microsoft/vscode/blob/1aa76d792f6857a0444b71e0a169f34a8ef5488d/src/vs/workbench/api/common/extHostTreeViews.ts#L390
|
@amiramw, I added a few remarks. Please adjust. The behavior works, but I will verify once more. I do not know if we have to note the breaking API change in the changelog, but doing it should not hurt. Please do so. Thanks! |
@kittaakos I fixed all the code comments. Which API do you mean? |
Thank you! 👍
I meant this one. The second arg was a string. I leave the decision to you. |
|
@kittaakos I added the changelog as well |
Various fixes to support tree view reveal in several unsupported scenarios and align it with vscode. - reveal called without focus now opens the view container (left pane) and view (section), without focus - reveal of a node that its ancestors are collapsed now expand all parents and reveals it - reveal of a node that was not cached by backend or not fetched by client (for example when showing the tree for the first time) now works Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
Fixed. I'll merge once builds finish. |
The issue is a regression introduced in #8783 Calculation of parent chain (for id calculation) calls getChildren which dispose nodes. This means that reveal for a node that is already revealed dispose the node and so the frontend has an id that doesn't exist in the backend anymore so tree item command is not found. The fix is to check the cache before calling getChildren and making sure that the technical root is also in the cache. Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
The issue is a regression introduced in #8783 Calculation of parent chain (for id calculation) calls getChildren which dispose nodes. This means that reveal for a node that is already revealed dispose the node and so the frontend has an id that doesn't exist in the backend anymore so tree item command is not found. The fix is to check the cache before calling getChildren and making sure that the technical root is also in the cache. Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
The issue is a regression introduced in #8783 Calculation of parent chain (for id calculation) calls getChildren which dispose nodes. This means that reveal for a node that is already revealed dispose the node and so the frontend has an id that doesn't exist in the backend anymore so tree item command is not found. The fix is to check the cache before calling getChildren and making sure that the technical root is also in the cache. Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
Signed-off-by: Amiram Wingarten amiram.wingarten@sap.com
What it does
Various fixes to support tree view reveal in several unsupported scenarios and align it with vscode.
===) to cached node now worksHow to test
yarn && vsce packageto create the vsix fileScenarios
First time:
From other view container:
From other view container:
With non identical element:
Review checklist
Reminder for reviewers