Use tail decorations for problems in TreeWidget

CHANGELOG.md

@@ -10,6 +10,14 @@
 
 - [plugin] added support for `DocumentSymbolProviderMetadata` [#10811](https://github.com/eclipse-theia/theia/pull/10811) - Contributed on behalf of STMicroelectronics
 
+## v1.24.0 - unreleased
+
+[1.24.0 Milestone](https://github.com/eclipse-theia/theia/milestone/32)
+
+<a name="breaking_changes_1.24.0">[Breaking Changes:](#breaking_changes_1.24.0)</a>
+
+ - [markers] `ProblemDecorator` reimplemented to reduce redundancy and align more closely with VSCode. `collectMarkers` now returns `Map<string, TreeDecoration.Data>`, `getOverlayIconColor` renamed to `getColor`, `getOverlayIcon` removed, `appendContainerMarkers` returns `void`.
+
 ## v1.23.0 - 2/24/2022
 
 [1.23.0 Milestone](https://github.com/eclipse-theia/theia/milestone/31)

packages/core/src/browser/style/tree.css

@@ -135,6 +135,10 @@
     text-align: center;
 }
 
+.theia-TreeNodeTail:not(:last-of-type)::after {
+    content: ',';
+}
+
 .theia-TreeNodeSegment mark {
     background-color: var(--theia-list-filterMatchBackground);
     color: var(--theia-list-inactiveSelectionForeground);

packages/core/src/browser/tree/tree-widget.tsx

@@ -799,7 +799,7 @@ export class TreeWidget extends ReactWidget implements StatefulWidget {
      * @param props the node properties.
      */
     protected renderTailDecorations(node: TreeNode, props: NodeProps): React.ReactNode {
-        const tailDecorations = this.getDecorationData(node, 'tailDecorations').filter(notEmpty).reduce((acc, current) => acc.concat(current), []);
+        const tailDecorations = this.getDecorationData(node, 'tailDecorations').reduce((acc, current) => acc.concat(current), []);
         if (tailDecorations.length === 0) {
             return;
         }
@@ -809,7 +809,7 @@ export class TreeWidget extends ReactWidget implements StatefulWidget {
     protected renderTailDecorationsForNode(node: TreeNode, props: NodeProps, tailDecorations:
         (TreeDecoration.TailDecoration | TreeDecoration.TailDecorationIcon | TreeDecoration.TailDecorationIconClass)[]): React.ReactNode {
         return <React.Fragment>
-            {tailDecorations.map((decoration, index) => {
+            {tailDecorations.reverse().map((decoration, index) => {
                 const { tooltip } = decoration;
                 const { data, fontData } = decoration as TreeDecoration.TailDecoration;
                 const color = (decoration as TreeDecoration.TailDecorationIcon).color;
@@ -1037,8 +1037,8 @@ export class TreeWidget extends ReactWidget implements StatefulWidget {
      *
      * @returns the tree decoration data at the given key.
      */
-    protected getDecorationData<K extends keyof TreeDecoration.Data>(node: TreeNode, key: K): TreeDecoration.Data[K][] {
-        return this.getDecorations(node).filter(data => data[key] !== undefined).map(data => data[key]).filter(notEmpty);
+    protected getDecorationData<K extends keyof TreeDecoration.Data>(node: TreeNode, key: K): Required<Pick<TreeDecoration.Data, K>>[K][] {
+        return this.getDecorations(node).filter(data => data[key] !== undefined).map(data => data[key]);
     }
 
     /**

packages/core/src/browser/widget-decoration.ts

@@ -337,7 +337,7 @@ export namespace WidgetDecoration {
     }
     export namespace Data {
         /**
-         * Compares the decoration data based on the priority. Lowest priorities come first.
+         * Compares the decoration data based on the priority. Lowest priorities come first (i.e. left.priority - right.priority).
          */
         export const comparePriority = (left: Data, right: Data): number => (left.priority || 0) - (right.priority || 0);
     }

packages/markers/src/browser/marker-manager.ts

@@ -176,6 +176,10 @@ export abstract class MarkerManager<D extends object> {
         return result;
     }
 
+    getMarkersByUri(): IterableIterator<[string, MarkerCollection<D>]> {
+        return this.uri2MarkerCollection.entries();
+    }
+
     getUris(): IterableIterator<string> {
         return this.uri2MarkerCollection.keys();
     }

packages/markers/src/browser/problem/problem-decorator.ts

@@ -17,9 +17,8 @@
 import { inject, injectable, postConstruct } from '@theia/core/shared/inversify';
 import { Diagnostic, DiagnosticSeverity } from '@theia/core/shared/vscode-languageserver-protocol';
 import URI from '@theia/core/lib/common/uri';
-import { notEmpty } from '@theia/core/lib/common/objects';
 import { Event, Emitter } from '@theia/core/lib/common/event';
-import { Tree } from '@theia/core/lib/browser/tree/tree';
+import { Tree, TreeNode } from '@theia/core/lib/browser/tree/tree';
 import { DepthFirstTreeIterator } from '@theia/core/lib/browser/tree/tree-iterator';
 import { TreeDecorator, TreeDecoration } from '@theia/core/lib/browser/tree/tree-decorator';
 import { FileStatNode } from '@theia/filesystem/lib/browser';
@@ -79,20 +78,18 @@ export class ProblemDecorator implements TreeDecorator {
     protected collectDecorators(tree: Tree): Map<string, TreeDecoration.Data> {
         const decorations = new Map<string, TreeDecoration.Data>();
         // 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']) {
+        if (!tree.root || !this.problemPreferences['problems.decorations.enabled']) {
             return decorations;
         }
-        const markers = this.appendContainerMarkers(tree, this.collectMarkers(tree));
+        const baseDecorations = this.collectMarkers(tree);
         for (const node of new DepthFirstTreeIterator(tree.root)) {
-            const nodeUri = FileStatNode.getUri(node);
+            const nodeUri = this.getUriFromNode(node);
             if (nodeUri) {
-                const marker = markers.get(nodeUri);
-                let decorator: TreeDecoration.Data | undefined;
-                if (marker) {
-                    decorator = this.toDecorator(marker);
-                }
+                let decorator = baseDecorations.get(nodeUri);
                 if (OpenEditorNode.is(node)) {
                     decorator = this.appendSuffixDecoration(node.uri, decorator);
+                } else if (decorator) {
+                    this.appendContainerMarkers(node, decorator, decorations);
                 }
                 if (decorator) {
                     decorations.set(node.id, decorator);
@@ -134,78 +131,56 @@ export class ProblemDecorator implements TreeDecorator {
         return `${workspacePrefixString}${separator}${filePathString}`;
     }
 
-    protected appendContainerMarkers(tree: Tree, markers: Marker<Diagnostic>[]): Map<string, Marker<Diagnostic>> {
-        const result: Map<string, Marker<Diagnostic>> = new Map();
-        // We traverse up and assign the diagnostic to the container directory.
-        // Note, instead of stopping at the WS root, we traverse up the driver root.
-        // We will filter them later based on the expansion state of the tree.
-        for (const [uri, marker] of new Map(markers.map(m => [new URI(m.uri), m] as [URI, Marker<Diagnostic>])).entries()) {
-            const uriString = uri.toString();
-            result.set(uriString, marker);
-            let parentUri: URI | undefined = uri.parent;
-            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;
-                }
+    /**
+     * Traverses up the tree from the given node and attaches decorations to any parents.
+     */
+    protected appendContainerMarkers(node: TreeNode, decoration: TreeDecoration.Data, decorations: Map<string, TreeDecoration.Data>): void {
+        let parent = node?.parent;
+        while (parent) {
+            const existing = decorations.get(parent.id);
+            // Make sure the highest diagnostic severity (smaller number) will be propagated to the container directory.
+            if (existing === undefined || this.compareDecorators(existing, decoration) < 0) {
+                decorations.set(parent.id, decoration);
+                parent = parent.parent;
+            } else {
+                break;
             }
         }
-        return result;
     }
 
-    protected collectMarkers(tree: Tree): Marker<Diagnostic>[] {
-        return Array.from(this.problemManager.getUris())
-            .map(uri => new URI(uri))
-            .map(uri => this.problemManager.findMarkers({ uri }))
-            .map(markers => markers.sort(this.compare.bind(this)))
-            .map(markers => markers.shift())
-            .filter(notEmpty)
-            .filter(this.filterMarker.bind(this));
+    /**
+     * @returns a map matching stringified URI's to a decoration whose features reflect the highest-severity problem found
+     * and the number of problems found (based on {@link ProblemDecorator.toDecorator })
+     */
+    protected collectMarkers(tree: Tree): Map<string, TreeDecoration.Data> {
+        const decorationsForUri = new Map();
+        const compare = this.compare.bind(this);
+        const filter = this.filterMarker.bind(this);
+        for (const [, markers] of this.problemManager.getMarkersByUri()) {
+            const relevant = markers.findMarkers({}).filter(filter).sort(compare);
+            if (relevant.length) {
+                decorationsForUri.set(relevant[0].uri, this.toDecorator(relevant));
+            }
+        }
+        return decorationsForUri;
     }
 
-    protected toDecorator(marker: Marker<Diagnostic>): TreeDecoration.Data {
-        const position = TreeDecoration.IconOverlayPosition.BOTTOM_RIGHT;
-        const icon = this.getOverlayIcon(marker);
-        const color = this.getOverlayIconColor(marker);
-        const priority = this.getPriority(marker);
+    protected toDecorator(markers: Marker<Diagnostic>[]): TreeDecoration.Data {
+        const color = this.getColor(markers[0]);
+        const priority = this.getPriority(markers[0]);
         return {
             priority,
             fontData: {
                 color,
             },
-            iconOverlay: {
-                position,
-                icon,
+            tailDecorations: [{
                 color,
-                background: {
-                    shape: 'circle',
-                    color: 'transparent'
-                }
-            },
+                data: markers.length.toString(),
+            }],
         };
     }
 
-    protected getOverlayIcon(marker: Marker<Diagnostic>): string {
-        const { severity } = marker.data;
-        switch (severity) {
-            case 1: return 'times-circle';
-            case 2: return 'exclamation-circle';
-            case 3: return 'info-circle';
-            default: return 'hand-o-up';
-        }
-    }
-
-    protected getOverlayIconColor(marker: Marker<Diagnostic>): TreeDecoration.Color {
+    protected getColor(marker: Marker<Diagnostic>): TreeDecoration.Color {
         const { severity } = marker.data;
         switch (severity) {
             case 1: return 'var(--theia-editorError-foreground)';
@@ -241,10 +216,17 @@ export class ProblemDecorator implements TreeDecorator {
             || severity === DiagnosticSeverity.Information;
     }
 
+    protected getUriFromNode(node: TreeNode): string | undefined {
+        return FileStatNode.getUri(node);
+    }
+
     protected compare(left: Marker<Diagnostic>, right: Marker<Diagnostic>): number {
         return ProblemDecorator.severityCompare(left, right);
     }
 
+    protected compareDecorators(left: TreeDecoration.Data, right: TreeDecoration.Data): number {
+        return TreeDecoration.Data.comparePriority(left, right);
+    }
 }
 
 export namespace ProblemDecorator {

packages/navigator/src/browser/navigator-widget.tsx

@@ -17,7 +17,7 @@
 import { injectable, inject, postConstruct } from '@theia/core/shared/inversify';
 import { Message } from '@theia/core/shared/@phosphor/messaging';
 import URI from '@theia/core/lib/common/uri';
-import { CommandService, notEmpty } from '@theia/core/lib/common';
+import { CommandService } from '@theia/core/lib/common';
 import {
     Key, TreeModel, SelectableTreeNode, TREE_NODE_SEGMENT_CLASS, TREE_NODE_TAIL_CLASS,
     TreeDecoration, NodeProps, OpenerService, ContextMenuRenderer, ExpandableTreeNode, TreeProps, TreeNode
@@ -148,7 +148,7 @@ export class FileNavigatorWidget extends FileTreeWidget {
     }
 
     protected override renderTailDecorations(node: TreeNode, props: NodeProps): React.ReactNode {
-        const tailDecorations = this.getDecorationData(node, 'tailDecorations').filter(notEmpty).reduce((acc, current) => acc.concat(current), []);
+        const tailDecorations = this.getDecorationData(node, 'tailDecorations').reduce((acc, current) => acc.concat(current), []);
 
         if (tailDecorations.length === 0) {
             return;