-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
[VSCode] #9642 Enable problem decorator in TreeView #9643
[VSCode] #9642 Enable problem decorator in TreeView #9643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contributions. Your changes look really promising already. I have a few suggestions/nitpicks though.
Additionally, would you mind adding the issue reproduction/testing steps to the 'How to test' section of your PR?
import { TreeViewDecorator, TreeViewDecoratorService } from './view/treeview-decorator-service'; | ||
import { TreeViewProblemDecorator } from './view/treeview-problem-decorator'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep everything more in line with other tree-view
files and we usually use kebap-case for our file names, please add the dash to the treeview
file names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed, thanks
} | ||
|
||
protected collectDecorators(tree: Tree): Map<string, TreeDecoration.Data> { | ||
const result = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This map should be typed to <string, Marker<Diagnostics>
. As it stands, this is a Map<any, any>
. This could lead to potential typing errors in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed here and in other cases in this file
} | ||
}); | ||
|
||
return new Map(Array.from(result.entries()).map(m => [m[0], this.toDecorator(m[1])] as [string, TreeDecoration.Data])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can omit the as [string, TreeDecoration.Data]
here (especially if you type the result
map correctly).
const result: Map<string, Marker<Diagnostic>> = new Map(); | ||
// We traverse up and assign the diagnostic to the container element. | ||
// Just like file based traverse, but use element parent instead. | ||
for (const [uri, marker] of new Map(markers.map(m => [new URI(m.uri), m] as [URI, Marker<Diagnostic>])).entries()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Instead of as [URI, Marker<Diagnostic>]
you should type the map constructor to <URI, Marker<Diagnostic>
.
5996552
to
4c7dc0a
Compare
Added reproduction steps from issue, is it okay? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xcariba Thanks work your work. Yes, the reproduction steps are enough.
I can confirm that the changes work as expected. The contributed File Explorer
view correctly displays problems like the default Theia file explorer:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe in which case the side-effect occurs?
It has a side effect - it will load whole tree at once in order to do that, but now it is consistent with VSCode implementation.
@@ -0,0 +1,35 @@ | |||
/******************************************************************************** | |||
* Copyright (C) 2018 TypeFox and others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xcariba was this file copied from somewhere, else you should put 2021
and you company if it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thanks, fixed
@@ -0,0 +1,96 @@ | |||
/******************************************************************************** | |||
* Copyright (C) 2018 TypeFox and others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous copyright header feedback.
import { TreeDecorator, AbstractTreeDecoratorService } from '@theia/core/lib/browser/tree/tree-decorator'; | ||
|
||
/** | ||
* Symbol for all decorators that would like to contribute into the vscode tree views. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Symbol for all decorators that would like to contribute into the vscode tree views. | |
* Symbol for all decorators that would like to contribute into plugin tree views. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe in which case the side-effect occurs?
In all plugin (vscode?) treeviews. Since decorators are applied on browser side and I had to iterate from diagnostic marker item to root (in order to set diagnostic markers on all parent elements) I had to load whole tree on browser side.
export const TreeViewDecorator = Symbol('TreeViewDecorator'); | ||
|
||
/** | ||
* Decorator service for the vscode tree views. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Decorator service for the vscode tree views. | |
* Decorator service for plugin tree views. |
4c7dc0a
to
427c047
Compare
427c047
to
36e59e1
Compare
Is there anything I can do to improve this PR? This issue is critical to me and I'm willing to fix or improve it to make it work properly =) |
import { TreeDecorator, AbstractTreeDecoratorService } from '@theia/core/lib/browser/tree/tree-decorator'; | ||
|
||
/** | ||
* Symbol for all decorators that would like to contribute into plugin tree views. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Symbol for all decorators that would like to contribute into plugin tree views. | |
* Symbol for all decorators that would like to contribute to plugin tree views. |
protected collectDecorators(tree: Tree): Map<string, TreeDecoration.Data> { | ||
const result = new Map<string, Marker<Diagnostic>>(); | ||
|
||
// If the tree root is undefined or the preference for the decorations is disabled, return an empty result map. | ||
if (tree.root === undefined || !this.problemPreferences['problems.decorations.enabled']) { | ||
return new Map<string, TreeDecoration.Data>(); | ||
} | ||
|
||
const pathToIdMap = new Map<string, string>(); | ||
|
||
for (const node of new DepthFirstTreeIterator(tree.root)) { | ||
if (this.isTreeItem(node) && node.resourceUri) { | ||
pathToIdMap.set(node.resourceUri.toString(), node.id); | ||
} | ||
} | ||
|
||
const markers = this.appendContainerItemMarkers(tree, this.collectMarkers(tree), pathToIdMap); | ||
|
||
markers.forEach((marker: Marker<Diagnostic>, uri: string) => { | ||
const nodeId = pathToIdMap.get(uri); | ||
if (nodeId) { | ||
result.set(nodeId, marker); | ||
} | ||
}); | ||
|
||
return new Map(Array.from(result.entries()).map(m => [m[0], this.toDecorator(m[1])])); | ||
} | ||
|
||
protected appendContainerItemMarkers(tree: Tree, markers: Marker<Diagnostic>[], pathToIdMap: Map<string, string>): Map<string, Marker<Diagnostic>> { | ||
const result: Map<string, Marker<Diagnostic>> = new Map(); | ||
// We traverse up and assign the diagnostic to the container element. | ||
// Just like file based traverse, but use element parent instead. | ||
for (const [uri, marker] of new Map<URI, Marker<Diagnostic>>(markers.map(m => [new URI(m.uri), m])).entries()) { | ||
const uriString = uri.toString(); | ||
result.set(uriString, marker); | ||
const parentNode = tree.getNode(pathToIdMap.get(uriString))?.parent; | ||
if (this.isTreeItem(parentNode) && parentNode.resourceUri) { | ||
let parentUri: URI | undefined = new URI(parentNode.resourceUri); | ||
while (parentUri && !parentUri.path.isRoot) { | ||
const parentUriString = parentUri.toString(); | ||
const existing = result.get(parentUriString); | ||
// Make sure the highest diagnostic severity (smaller number) will be propagated to the container directory. | ||
if (existing === undefined || this.compare(marker, existing) < 0) { | ||
result.set(parentUriString, { | ||
data: marker.data, | ||
uri: parentUriString, | ||
owner: marker.owner, | ||
kind: marker.kind | ||
}); | ||
parentUri = parentUri.parent; | ||
} else { | ||
parentUri = undefined; | ||
} | ||
} | ||
} | ||
} | ||
|
||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two methods contain a lot of redundant iteration and can be simplified considerably. I suggest something like this:
protected collectDecorators(tree: Tree): Map<string, TreeDecoration.Data> {
if (tree.root === undefined || !this.problemPreferences['problems.decorations.enabled']) {
return new Map();
}
const uriToMarker = new Map<string, Marker<Diagnostic>>();
const markers = this.collectMarkers(tree);
const idToDecoration = new Map<string, TreeDecoration.Data>();
const propagateToParents = (uri: URI, childMarkerToPropagate: Marker<Diagnostic>): void => {
let currentUri = uri.parent;
while (!currentUri.path.isRoot) {
const currentUriString = currentUri.toString();
const existingParentMarker = uriToMarker.get(currentUriString);
if (!existingParentMarker || this.compare(childMarkerToPropagate, existingParentMarker) < 0) {
const parentMarker = {
...childMarkerToPropagate,
uri: currentUriString,
};
uriToMarker.set(currentUriString, parentMarker);
} else {
break; // Higher-priority marker already set and propagated.
}
currentUri = currentUri.parent;
}
};
for (const marker of markers) {
const uri = new URI(marker.uri);
const uriString = uri.toString();
uriToMarker.set(uriString, marker);
propagateToParents(uri, marker);
}
for (const node of new DepthFirstTreeIterator(tree.root)) {
if (this.isTreeItem(node) && node.resourceUri) {
const marker = uriToMarker.get(node.resourceUri.toString());
if (marker) {
idToDecoration.set(node.id, this.toDecorator(marker));
}
}
}
return idToDecoration;
}
There's a little bit of confusion in the existing code:
// We traverse up and assign the diagnostic to the container element.
// Just like file based traverse, but use element parent instead.
First, the code retrieves the parent element (const parentNode = tree.getNode(pathToIdMap.get(uriString))?.parent;
), but later it uses the URI's parent (parentUri = parentUri.parent;
). As I've rewritten the code, I follow the latter approach (URI's parent). If the intent is to propagate up through the tree, regardless of whether the tree is structured like a file tree, the code would need to be adjusted accordingly.
try { | ||
result = parent; | ||
// Resolve all tree items to enable problem decorator | ||
const children = await this.resolveAllChildren(parent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make this a standard feature for all plugin-contributed tree views? From the behavior of some tree views in VSCode (e.g. in the GitHub plugin, the Reference View), it certainly seems as though resolving children and grandchildren is delayed until requested, and automatically resolving all children could come with substantial performance consequences for trees that rely on network calls to retrieve information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I tried to fetch all children to apply decorations for parent elements. In my case (in my extension) I send whole tree in one request and this change won't affect performance, but for other cases it won't be effective. I'll try to figure out how to fix it, thanks!
@colin-grant-work @msujew can we move this one forward? |
Unfortunately I don't have time to fix this PR right now, I will be able to fix it in a month. I also already implemented git decorations based on this branch, but the solution is far from ideal in common case (it works for me now). If tree data provider has slow operations (like networking or some heavy processing) it will be unusable. |
@xcariba : Would you be able to rebase, so that @colin-grant-work can have another look? Would be great to get your contributio n in! |
Signed-off-by: Alexander Kozinko <[email protected]>
36e59e1
to
e2028b8
Compare
Yes, I did rebase on current master. Examples:Child file for fileIf I have some companion file for other file, for example header file and C file and I want to show header file node with C file as child node. So I have some folder structure, like in explorer, but I in folders I only show header files and they have C files as child nodes.
Group elementIf I want to group header and C file in one virtual group diagnostic will not show up on that group.
ProblemCurrent stateIn current (master) solution for decorators I would go from file diagnostic up to workspace root by its parent folders and set diagnostics to nodes, that represent files and folder of URI segments, but in this case C file diagnostic will not show up neither on header file (it will show on C file and its folder and up) nor on common group (myEntity1 in this example) since it is. My (current, not ideal) solutionIn my solution I get node that corresponds to diagnostic URI and go up to root by node parent elements, so parent element always gets child element diagnostic. But if element tree is not fully loaded diagnostic will not show up until I expand or load whole tree to all elements. In my vscode plugin it is not a big problem since I can load whole tree in one request, but it is not great solution in general as it may affect performance and memory consumption if tree is really huge or if it is loaded asynchronously and load operation is expensive (like when node children are loaded from remote resource or require some calculations). ConclusionI'm not sure how it is implemented in vscode, but it works fine in this case. It will be better if we could determine if tree item child request returns promise or items and if it is an array of items - load it immediately, but I'm not sure if it is possible here. |
@xcariba, I'm happy to take a look at how we can make this work for your use case and comply more generally with the VSCode tree view behavior. I have two questions for you:
|
Unfortunately I cannot share plugin, maybe I'll find time to create example plugin that shows this issue, but I don't know when I'll be able to.
It is my implementation of TreeDataProvider. If element is undefined (root element) it loads whole tree from my service. |
@msujew : You added a screenshot before, what did you use for testing? |
@xcariba, I have been working on this a bit, but I've run into some difficulties, so I'd like to ask you a little bit more about your use case and the behavior you observe in VSCode. From your description, your first use case is a grouping of files under files. That involves three entities: real parent folder, virtual parent file (.h, in your case), and virtual child file (.c, in your case). I have created a sample extension that does something like this for the Theia repo by grouping In that screenshot, the child file ( Your second use case is virtual groupings where pairs of related files are grouped under a virtual parent. When I try a naive version of that in my sample extension, I get the following in VSCode: Here, the test and the source file are grouped under the virtual parent The basic difficulty is that VSCode's decoration is strongly oriented towards and highly optimized for URI's. It's possible in that context to implement something more sophisticated than what I've linked to spoof VSCode into showing markers on virtual parents, but by default, it isn't propagating the markers to parents in the tree structure, it's using properties of URI's to identify child decorations when it decorates the parent. |
@colin-grant-work I checked current version and I can confirm that I probably misinformed you or maybe something has changed. VSCode indeed works only with URI. |
Signed-off-by: Alexander Kozinko [email protected]
What it does
Enables problem decorations in VSCode extensions that contribute TreeView.
It adds decorator service for TreeView api and adapts existing problem decorator to work with it. It also supports decorator for tree models with structure that does not represent real file system tree, for example tree item may have
resourceUri
that point to one file and this item has child that points to another file in the same folder. It will add proper diagnostics for both these files. It has a side effect - it will load whole tree at once in order to do that, but now it is consistent with VSCode implementation.How to test
Review checklist
Reminder for reviewers