Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use tree item path for tree node id. Fixes #12111 #12120

Merged
merged 6 commits into from
Feb 27, 2023
Merged

Use tree item path for tree node id. Fixes #12111 #12120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e871339
Use tree item path for tree node id. Fixes #12111
tsmaeder Jan 27, 2023
326a643
Make the linter happy
tsmaeder Jan 30, 2023
58c3218
remove unnecessary loop
paul-marechal Feb 3, 2023
dc9a715
add helper comment
paul-marechal Feb 3, 2023
7dcc283
Add breaking change log
tsmaeder Feb 24, 2023
b07dec7
Undo method rename
tsmaeder Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@

- [Previous Changelogs](https://github.com/eclipse-theia/theia/tree/master/doc/changelogs/)

## v1.36.0

<a name="breaking_changes_1.36.0">[Breaking Changes:](#breaking_changes_1.36.0)</a>

- [plugin] renamed `TreeViewExtImpl#toTreeItem()` to `TreeViewExtImpl#toTreeElement()`

## v1.35.0 - 02/23/2023

- [application-package] updated default supported VS Code API to `1.70.1` [#12200](https://github.com/eclipse-theia/theia/pull/12200)
Expand Down
59 changes: 33 additions & 26 deletions packages/plugin-ext/src/plugin/tree/tree-views.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import { URI } from '@theia/core/shared/vscode-uri';
import { UriComponents } from '@theia/core/lib/common/uri';

export class TreeViewsExtImpl implements TreeViewsExt {

private proxy: TreeViewsMain;

private readonly treeViews = new Map<string, TreeViewExtImpl<any>>();
Expand All @@ -45,9 +44,9 @@ export class TreeViewsExtImpl implements TreeViewsExt {
commandRegistry.registerArgumentProcessor({
processArgument: arg => {
if (TreeViewItemReference.is(arg)) {
return this.toTreeItem(arg);
return this.toTreeElement(arg);
} else if (Array.isArray(arg)) {
return arg.map(param => TreeViewItemReference.is(param) ? this.toTreeItem(param) : param);
return arg.map(param => TreeViewItemReference.is(param) ? this.toTreeElement(param) : param);
} else {
return arg;
}
Expand All @@ -62,8 +61,8 @@ export class TreeViewsExtImpl implements TreeViewsExt {
return this.getTreeView(treeViewId).handleDrop!(treeItemId, dataTransferItems, token);
}

protected toTreeItem(treeViewItemRef: TreeViewItemReference): any {
return this.treeViews.get(treeViewItemRef.viewId)?.getTreeItem(treeViewItemRef.itemId);
protected toTreeElement(treeViewItemRef: TreeViewItemReference): any {
colin-grant-work marked this conversation as resolved.
Show resolved Hide resolved
return this.treeViews.get(treeViewItemRef.viewId)?.getElement(treeViewItemRef.itemId);
}

registerTreeDataProvider<T>(plugin: Plugin, treeViewId: string, treeDataProvider: TreeDataProvider<T>): PluginDisposable {
Expand Down Expand Up @@ -186,6 +185,10 @@ interface TreeExtNode<T> extends Disposable {
}

class TreeViewExtImpl<T> implements Disposable {
private static readonly ID_COMPUTED = 'c';
private static readonly ID_ITEM = 'i';

private nextItemId: 0;

private readonly onDidExpandElementEmitter = new Emitter<TreeViewExpansionEvent<T>>();
readonly onDidExpandElement = this.onDidExpandElementEmitter.event;
Expand Down Expand Up @@ -274,7 +277,7 @@ class TreeViewExtImpl<T> implements Disposable {
this.proxy.$setDescription(this.treeViewId, this._description);
}

getTreeItem(treeItemId: string): T | undefined {
getElement(treeItemId: string): T | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here re: naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And same answer ;-)

return this.nodes.get(treeItemId)?.value;
}

Expand Down Expand Up @@ -312,14 +315,9 @@ class TreeViewExtImpl<T> implements Disposable {
// parent is inconsistent
return undefined;
}
const idLabel = this.getTreeItemIdLabel(treeItem);
let possibleIndex = children.length;
// find the right element id by searching all possible id names in the cache
while (possibleIndex-- > 0) {
const candidateId = this.buildTreeItemId(parentId, possibleIndex, idLabel);
if (this.nodes.has(candidateId)) {
return chain.concat(candidateId);
}
const candidateId = this.buildTreeItemId(parentId, treeItem);
if (this.nodes.has(candidateId)) {
return chain.concat(candidateId);
Comment on lines +318 to +320
Copy link
Member

@paul-marechal paul-marechal Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this new code mean we'll never hit the cache for tree items for which we compute an ID?

The previous logic seemed to assume we could find items from some possibleIndex, but now there's a case where buildTreeItemId generates a new unique ID for each call, which means that those items will never be found here.

Is this an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if the tree item does not have an id set. This is correct, in my opinion, because if it doesn't have an id, there's no way to tell it's the same tree item, so any state we might have for the tree item at index n might not apply to the new item at index n.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the documentation of TreeItem says:

If not provided, an id is generated using the tree item's label. Note that when labels change, ids will change and that selection and expansion state cannot be kept stable anymore.

If you look at the way id's are constructed, the id's will be the same for items that have the same label.

}
// couldn't calculate consistent parent chain and id
return undefined;
Expand All @@ -335,7 +333,7 @@ class TreeViewExtImpl<T> implements Disposable {
return typeof treeItemLabel === 'object' ? treeItemLabel.highlights : undefined;
}

private getTreeItemIdLabel(treeItem: TreeItem): string | undefined {
private getItemLabel(treeItem: TreeItem): string | undefined {
let idLabel = this.getTreeItemLabel(treeItem);
// Use resource URI if label is not set
if (idLabel === undefined && treeItem.resourceUri) {
Expand All @@ -348,8 +346,18 @@ class TreeViewExtImpl<T> implements Disposable {
return idLabel;
}

private buildTreeItemId(parentId: string, index: number, idLabel: string | undefined): string {
return `${parentId}/${index}:${idLabel}`;
private buildTreeItemId(parentId: string, item: TreeItem): string {
// build tree id according to https://code.visualstudio.com/api/references/vscode-api#TreeItem
colin-grant-work marked this conversation as resolved.
Show resolved Hide resolved
// note: the front end tree implementation cannot handle reparenting items, hence the id is set to the "path" of individual ids
let id = typeof item.id === 'string' ? item.id : this.getItemLabel(item);
if (id) {
// we use '' as the id of the root, we don't consider that a valid id
// since '' is falsy, we'll never get '' in this branch
id = TreeViewExtImpl.ID_ITEM + id;
} else {
id = TreeViewExtImpl.ID_COMPUTED + this.nextItemId++;
}
return `${parentId}/${id}`;
colin-grant-work marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +349 to +360
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsmaeder, this turns out to be a dangerous way to build the ID. If getItemLabel returns the same thing for several children of the same item - we've seen this downstream in the references view when the lines where something is referred to are identical - then this produces children with the exact same ID's, which isn't permitted in our core Tree implementation. Should be able to reproduce in Theia in a moment.

}

async getChildren(parentId: string): Promise<TreeViewItem[] | undefined> {
Expand All @@ -369,20 +377,19 @@ class TreeViewExtImpl<T> implements Disposable {
// ask data provider for children for cached element
const result = await this.options.treeDataProvider.getChildren(parent);
if (result) {
const treeItemPromises = result.map(async (value, index) => {
const treeItemPromises = result.map(async value => {

// Ask data provider for a tree item for the value
// Data provider must return theia.TreeItem
const treeItem = await this.options.treeDataProvider.getTreeItem(value);
// Convert theia.TreeItem to the TreeViewItem

const label = this.getTreeItemLabel(treeItem);
const label = this.getItemLabel(treeItem);
const highlights = this.getTreeItemLabelHighlights(treeItem);
const idLabel = this.getTreeItemIdLabel(treeItem);

// Generate the ID
// ID is used for caching the element
const id = treeItem.id || this.buildTreeItemId(parentId, index, idLabel);
const id = this.buildTreeItemId(parentId, treeItem);

const toDisposeElement = new DisposableCollection();
const node: TreeExtNode<T> = {
Expand Down Expand Up @@ -467,7 +474,7 @@ class TreeViewExtImpl<T> implements Disposable {

async onExpanded(treeItemId: string): Promise<any> {
// get element from a cache
const cachedElement = this.getTreeItem(treeItemId);
const cachedElement = this.getElement(treeItemId);

// fire an event
if (cachedElement) {
Expand All @@ -479,7 +486,7 @@ class TreeViewExtImpl<T> implements Disposable {

async onCollapsed(treeItemId: string): Promise<any> {
// get element from a cache
const cachedElement = this.getTreeItem(treeItemId);
const cachedElement = this.getElement(treeItemId);

// fire an event
if (cachedElement) {
Expand Down Expand Up @@ -513,7 +520,7 @@ class TreeViewExtImpl<T> implements Disposable {
get selectedElements(): T[] {
const items: T[] = [];
for (const id of this.selectedItemIds) {
const item = this.getTreeItem(id);
const item = this.getElement(id);
if (item) {
items.push(item);
}
Expand Down Expand Up @@ -554,7 +561,7 @@ class TreeViewExtImpl<T> implements Disposable {
async onDragStarted(treeItemIds: string[], token: CancellationToken): Promise<UriComponents[] | undefined> {
const treeItems: T[] = [];
for (const id of treeItemIds) {
const item = this.getTreeItem(id);
const item = this.getElement(id);
if (item) {
treeItems.push(item);
}
Expand All @@ -571,7 +578,7 @@ class TreeViewExtImpl<T> implements Disposable {
}

async handleDrop(treeItemId: string | undefined, dataTransferItems: [string, string | DataTransferFileDTO][], token: CancellationToken): Promise<void> {
const treeItem = treeItemId ? this.getTreeItem(treeItemId) : undefined;
const treeItem = treeItemId ? this.getElement(treeItemId) : undefined;
const dropTransfer = new DataTransfer();
if (this.options.dragAndDropController?.handleDrop) {
this.localDataTransfer.forEach((item, type) => {
Expand Down