markers: add sorting of problem markers

Fixes #6572

The following commit updates the `markers` to be sorted based on the following rules:
- markers are sorted by `severity`.
- if the first step is equal, markers are sorted by `line number`.
- if the second step is equal, markers are sorted by `column number`.
- else return no change.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>

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

@@ -27,6 +27,7 @@ import { Marker } from '../../common/marker';
 import { ProblemManager } from './problem-manager';
 import { ProblemPreferences, ProblemConfiguration } from './problem-preferences';
 import { PreferenceChangeEvent } from '@theia/core/lib/browser';
+import { ProblemUtils } from './problem-utils';
 
 @injectable()
 export class ProblemDecorator implements TreeDecorator {
@@ -182,7 +183,6 @@ export class ProblemDecorator implements TreeDecorator {
 export namespace ProblemDecorator {
 
     // Highest severities (errors) come first, then the others. Undefined severities treated as the last ones.
-    export const severityCompare = (left: Marker<Diagnostic>, right: Marker<Diagnostic>): number =>
-        (left.data.severity || Number.MAX_SAFE_INTEGER) - (right.data.severity || Number.MAX_SAFE_INTEGER);
+    export const severityCompare = ProblemUtils.severityCompare;
 
 }

packages/markers/src/browser/problem/problem-tree-model.spec.ts

@@ -0,0 +1,176 @@
+/********************************************************************************
+ * Copyright (C) 2020 Ericsson and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+import { enableJSDOM } from '@theia/core/lib/browser/test/jsdom';
+let disableJSDOM = enableJSDOM();
+
+import URI from '@theia/core/lib/common/uri';
+import { expect } from 'chai';
+import { Container } from 'inversify';
+import { Diagnostic, Range, DiagnosticSeverity } from 'vscode-languageserver-types';
+import { Event } from '@theia/core/lib/common/event';
+import { FileSystemWatcher } from '@theia/filesystem/lib/browser/filesystem-watcher';
+import { Marker } from '../../common/marker';
+import { MarkerManager } from '../marker-manager';
+import { MarkerNode, MarkerOptions } from '../marker-tree';
+import { PROBLEM_OPTIONS } from './problem-container';
+import { ProblemManager } from './problem-manager';
+import { ProblemTree } from './problem-tree-model';
+
+disableJSDOM();
+
+let problemTree: ProblemTree;
+
+before(() => {
+    disableJSDOM = enableJSDOM();
+    const testContainer = new Container();
+
+    testContainer.bind(MarkerManager).toSelf().inSingletonScope();
+    testContainer.bind(ProblemManager).toSelf();
+    testContainer.bind(MarkerOptions).toConstantValue(PROBLEM_OPTIONS);
+    testContainer.bind(FileSystemWatcher).toConstantValue({
+        onFilesChanged: Event.None
+    } as FileSystemWatcher);
+
+    testContainer.bind(ProblemTree).toSelf().inSingletonScope();
+    problemTree = testContainer.get<ProblemTree>(ProblemTree);
+});
+
+after(() => {
+    disableJSDOM();
+});
+
+describe('Problem Tree', () => {
+
+    describe('#sortMarkers', () => {
+        describe('should sort markers based on the highest severity', () => {
+            it('should sort errors higher than warnings', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Error);
+                const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Warning);
+                const nodeA = createMockMarkerNode(markerA);
+                const nodeB = createMockMarkerNode(markerB);
+                expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-1);
+                expect(problemTree.sortMarkers(nodeB, nodeA)).equals(1);
+            });
+            it('should sort errors higher than infos', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Error);
+                const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Information);
+                const nodeA = createMockMarkerNode(markerA);
+                const nodeB = createMockMarkerNode(markerB);
+                expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-2);
+                expect(problemTree.sortMarkers(nodeB, nodeA)).equals(2);
+            });
+            it('should sort errors higher than hints', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Error);
+                const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Hint);
+                const nodeA = createMockMarkerNode(markerA);
+                const nodeB = createMockMarkerNode(markerB);
+                expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-3);
+                expect(problemTree.sortMarkers(nodeB, nodeA)).equals(3);
+            });
+            it('should sort warnings higher than infos', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Warning);
+                const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Information);
+                const nodeA = createMockMarkerNode(markerA);
+                const nodeB = createMockMarkerNode(markerB);
+                expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-1);
+                expect(problemTree.sortMarkers(nodeB, nodeA)).equals(1);
+            });
+            it('should sort warnings higher than hints', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Warning);
+                const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Hint);
+                const nodeA = createMockMarkerNode(markerA);
+                const nodeB = createMockMarkerNode(markerB);
+                expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-2);
+                expect(problemTree.sortMarkers(nodeB, nodeA)).equals(2);
+            });
+            it('should sort infos higher than hints', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Information);
+                const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Hint);
+                const nodeA = createMockMarkerNode(markerA);
+                const nodeB = createMockMarkerNode(markerB);
+                expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-1);
+                expect(problemTree.sortMarkers(nodeB, nodeA)).equals(1);
+            });
+        });
+
+        it('should sort markers based on lowest line number if their severities are equal', () => {
+            const markerA = createMockMarker({ start: { line: 1, character: 10 }, end: { line: 1, character: 20 } }, DiagnosticSeverity.Error);
+            const markerB = createMockMarker({ start: { line: 5, character: 10 }, end: { line: 5, character: 20 } }, DiagnosticSeverity.Error);
+            const nodeA = createMockMarkerNode(markerA);
+            const nodeB = createMockMarkerNode(markerB);
+            expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-4);
+            expect(problemTree.sortMarkers(nodeB, nodeA)).equals(4);
+        });
+
+        it('should sort markers based on lowest column number if their severities and line numbers are equal', () => {
+            const markerA = createMockMarker({ start: { line: 1, character: 10 }, end: { line: 1, character: 10 } }, DiagnosticSeverity.Error);
+            const markerB = createMockMarker({ start: { line: 1, character: 20 }, end: { line: 1, character: 20 } }, DiagnosticSeverity.Error);
+            const nodeA = createMockMarkerNode(markerA);
+            const nodeB = createMockMarkerNode(markerB);
+            expect(problemTree.sortMarkers(nodeA, nodeB)).equals(-10);
+            expect(problemTree.sortMarkers(nodeB, nodeA)).equals(10);
+        });
+
+        it('should not sort if markers are equal', () => {
+            const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Error);
+            const markerB = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Error);
+            const nodeA = createMockMarkerNode(markerA);
+            const nodeB = createMockMarkerNode(markerB);
+            expect(problemTree.sortMarkers(nodeA, nodeB)).equals(0);
+            expect(problemTree.sortMarkers(nodeB, nodeA)).equals(0);
+        });
+    });
+
+});
+
+/**
+ * Create a mock marker node with the given diagnostic marker.
+ * @param marker the diagnostic marker.
+ *
+ * @returns a mock marker node.
+ */
+function createMockMarkerNode(marker: Marker<Diagnostic>): MarkerNode {
+    return {
+        id: 'id',
+        name: 'marker',
+        parent: undefined,
+        selected: false,
+        uri: new URI(''),
+        marker
+    };
+}
+
+/**
+ * Create a mock diagnostic marker.
+ * @param range the diagnostic range.
+ * @param severity the diagnostic severity.
+ *
+ * @returns a mock diagnostic marker.
+ */
+function createMockMarker(range: Range, severity: DiagnosticSeverity): Readonly<Marker<Diagnostic>> {
+    const data: Diagnostic = {
+        range: range,
+        severity: severity,
+        message: 'message'
+    };
+    return Object.freeze({
+        uri: name,
+        kind: 'marker',
+        owner: 'owner',
+        data
+    });
+}

packages/markers/src/browser/problem/problem-tree-model.ts

@@ -20,15 +20,53 @@ import { MarkerNode, MarkerTree, MarkerOptions, MarkerInfoNode } from '../marker
 import { MarkerTreeModel } from '../marker-tree-model';
 import { injectable, inject } from 'inversify';
 import { OpenerOptions, TreeNode } from '@theia/core/lib/browser';
+import { Marker } from '../../common/marker';
 import { Diagnostic } from 'vscode-languageserver-types';
+import { ProblemUtils } from './problem-utils';
 
 @injectable()
 export class ProblemTree extends MarkerTree<Diagnostic> {
+
     constructor(
         @inject(ProblemManager) protected readonly problemManager: ProblemManager,
         @inject(MarkerOptions) protected readonly markerOptions: MarkerOptions) {
         super(problemManager, markerOptions);
     }
+
+    protected getMarkerNodes(parent: MarkerInfoNode, markers: Marker<Diagnostic>[]): MarkerNode[] {
+        return markers.map((marker, index) => this.createMarkerNode(marker, index, parent)).sort((a, b) => this.sortMarkers(a, b));
+    }
+
+    /**
+     * Sort markers based on the following rules:
+     * - Markers are fist sorted by `severity`.
+     * - Markers are sorted by `line number` if applicable.
+     * - Markers are sorted by `column number` if
+     * @param a the first marker for comparison.
+     * @param b the second marker for comparison.
+     */
+    sortMarkers(a: MarkerNode, b: MarkerNode): number {
+        const markerA = a.marker as Marker<Diagnostic>;
+        const markerB = b.marker as Marker<Diagnostic>;
+
+        // Determine the marker with the highest severity.
+        const severity = ProblemUtils.severityCompare(markerA, markerB);
+        if (severity !== 0) {
+            return severity;
+        }
+        // Determine the marker with the lower line number.
+        const lineNumber = ProblemUtils.lineNumberCompare(markerA, markerB);
+        if (lineNumber !== 0) {
+            return lineNumber;
+        }
+        // Determine the marker with the lower column number.
+        const columnNumber = ProblemUtils.columnNumberCompare(markerA, markerB);
+        if (columnNumber !== 0) {
+            return columnNumber;
+        }
+        return 0;
+    }
+
 }
 
 @injectable()

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

@@ -0,0 +1,48 @@
+/********************************************************************************
+ * Copyright (C) 2020 Ericsson and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+import { Marker } from '../../common/marker';
+import { Diagnostic } from '@theia/languages/lib/browser';
+
+export namespace ProblemUtils {
+
+    /**
+     * Comparator for severity.
+     * - The highest severity (`error`) come first followed by others.
+     * - `undefined` severities are treated as the last ones.
+     * @param a the first marker for comparison.
+     * @param b the second marker for comparison.
+     */
+    export const severityCompare = (a: Marker<Diagnostic>, b: Marker<Diagnostic>): number =>
+        (a.data.severity || Number.MAX_SAFE_INTEGER) - (b.data.severity || Number.MAX_SAFE_INTEGER);
+
+    /**
+     * Comparator for line numbers.
+     * - The lowest line number comes first.
+     * @param a the first marker for comparison.
+     * @param b the second marker for comparison.
+     */
+    export const lineNumberCompare = (a: Marker<Diagnostic>, b: Marker<Diagnostic>): number => a.data.range.start.line - b.data.range.start.line;
+
+    /**
+     * Comparator for column numbers.
+     * - The lowest column number comes first.
+     * @param a the first marker for comparison.
+     * @param b the second marker for comparison.
+     */
+    export const columnNumberCompare = (a: Marker<Diagnostic>, b: Marker<Diagnostic>): number => a.data.range.start.character - b.data.range.start.character;
+
+}