Implement order for info-marker-nodes #9691

LukasBoll · 2021-07-01T20:04:03Z

What it does

Previously info-marker-nodes were ordered by the time they have been reported to the problem-tree-model. This led to an unexpected order of the nodes in the problem view. (See #8983)
In this commit info-marker-nodes will be inserted to the tree-model based on the following rules:
-nodes will be sorted based on the most severe marker they contain
-if nodes contain equally severe markers, they are sorted by the URI

How to test

Download test workspace from here:
https://github.com/eclipse-theia/theia/files/5859357/problems-view.zip

You should be able to:

Open and close all files. The nodes concerning the specific file should be added in the defined order to the previous nodes.
Change settings without the order of nodes being changed.
Add or remove warning or errors to a file. The order in the problem-view should be adjusted.
Rename files. The order in the problem-view should be adjusted.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

LukasBoll · 2021-07-01T20:17:47Z

I also tried the implementation with 33k markers from the eclipse.jdt.ls workspace
The time it took to load all errors, once the first error got reported (in seconds):

Master	Sorted
12.7	16.6
14.1	16.2
12.6	16.1

The Application is responsive, during that time (but reacting slow).

max-elia

The code was built and tested and the Marker's sorting works as described in the PR:

When new files are opened their Markers are inserted in the right place.
Setting changes do not change the marker's order.
Changes in the file, add/remove markers in the right place.
Renaming files results in reordering of markers with same severity.

The tests are passing, but should be adjusted to match the description.

Should be rebased onto recent commits of master.

max-elia · 2021-07-05T10:20:42Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+            });
+            it('should sort warnings 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.Hint);


DiagnosticSeverity should be Warning and Info

@LukasBoll any reason not to change the severity here? The test-case is for should sort warnings higher than infos but we are comparing to hint.

No, I have overlooked that one. I will fix it.

Ok, the test should be correct now.

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

max-elia · 2021-07-05T10:21:48Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+                expect(markerInfoNodeB.previousSibling).to.equal(markerInfoNodeA);
+            });
+            it('should sort infos higher than hints', () => {
+                const markerA = createMockMarker({ start: { line: 0, character: 10 }, end: { line: 0, character: 10 } }, DiagnosticSeverity.Error);


DiagnosticSeverity.Information

max-elia · 2021-07-05T10:26:42Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+
+        it('should sort markersInfo based on URI if severities 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.Hint);


Should have equal severity

LukasBoll · 2021-07-06T15:00:30Z

Thank you, I just updated the tests and rebased on Master.

vince-fugnitto · 2021-07-06T15:19:11Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+
+let rootNode: MarkerRootNode = {
+    visible: false,
+    id: 'theia-' + 'problem' + '-marker-widget',


minor: any reason to do string concatenation?

Suggested change

id: 'theia-' + 'problem' + '-marker-widget',

id: 'theia-problem-marker-widget',

vince-fugnitto · 2021-07-06T15:20:59Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+    disableJSDOM();
+});
+
+describe('Problem Composite Tree Node', () => {


minor:

Suggested change

describe('Problem Composite Tree Node', () => {

describe('problem-composite-tree-node', () => {

or

describe('ProblemCompositeTreeNode', () => {

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

vince-fugnitto · 2021-07-06T15:22:20Z

packages/markers/src/browser/problem/problem-composite-tree-node.ts

@@ -0,0 +1,84 @@
+/********************************************************************************
+ * Copyright (C) 2021 Ericsson and others.


@LukasBoll you would either put your company name, or actual name. I don't believe you work at Ericsson?

vince-fugnitto · 2021-07-06T15:23:24Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

@@ -0,0 +1,310 @@
+/********************************************************************************
+ * Copyright (C) 2021 Ericsson and others.


Same as the other copyright comment.

LukasBoll · 2021-07-07T11:01:08Z

Thank you for the hints, I changed the copyright and integrated all your suggestions.

msujew

The changes behave well and the tests are looking good. 👍

vince-fugnitto · 2021-07-20T18:55:50Z

@LukasBoll any reason why the pull-request is a wip, or is it ready to perform a full review?

LukasBoll · 2021-07-23T17:21:39Z

@LukasBoll any reason why the pull-request is a wip, or is it ready to perform a full review?

It´s ready for the full review!

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

vince-fugnitto · 2021-07-23T18:55:10Z

packages/markers/src/browser/problem/problem-composite-tree-node.ts

+    export function setSeverity(parent: MarkerInfoNode, markers: Marker<Diagnostic>[]): void {
+        let maxServerity: DiagnosticSeverity | undefined;
+        markers.forEach(marker => {
+            if (ProblemUtils.severityCompare(marker.data.severity, maxServerity) < 0) {
+                maxServerity = marker.data.severity;
+            }
+        });
+        parent.severity = maxServerity;
+    };


typos:

Suggested change

export function setSeverity(parent: MarkerInfoNode, markers: Marker<Diagnostic>[]): void {

let maxServerity: DiagnosticSeverity | undefined;

markers.forEach(marker => {

if (ProblemUtils.severityCompare(marker.data.severity, maxServerity) < 0) {

maxServerity = marker.data.severity;

}

});

parent.severity = maxServerity;

};

export function setSeverity(parent: MarkerInfoNode, markers: Marker<Diagnostic>[]): void {

let maxSeverity: DiagnosticSeverity | undefined;

markers.forEach(marker => {

if (ProblemUtils.severityCompare(marker.data.severity, maxSeverity) < 0) {

maxSeverity = marker.data.severity;

}

});

parent.severity = maxSeverity;

};

vince-fugnitto

Please address all outstanding comments and feedback and we can perform subsequent reviews.

vince-fugnitto · 2021-07-27T19:18:23Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+let rootNode: MarkerRootNode = {
+    visible: false,
+    id: 'theia-problem-marker-widget',
+    name: 'MarkerTree',
+    kind: 'problem',
+    children: [],
+    parent: undefined
+};


Since you set it already on beforeEach:

Suggested change

let rootNode: MarkerRootNode = {

visible: false,

id: 'theia-problem-marker-widget',

name: 'MarkerTree',

kind: 'problem',

children: [],

parent: undefined

};

let rootNode: MarkerRootNode;

I updated the pull request and followed all your suggestions. I didn´t change the “severity” attribute in the MarkerInfoNote. Should I perform one of my suggested changes or is it fine like this?

tsmaeder · 2021-09-09T11:53:30Z

@vince-fugnitto @msujew Please merge if appropriate.

vince-fugnitto · 2021-09-17T19:06:26Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+            expect(markerInfoNodeB.previousSibling).to.equal(markerInfoNodeA);
+        });
+
+        it('change marker content', () => {


nitpick: consistency with should ...

I added should to all descriptions.

vince-fugnitto · 2021-09-17T19:06:49Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+            expect(rootNode.children[0]).to.equal(markerInfoNodeA);
+        });
+
+        it('change marker content leading to lower rank', () => {


nitpick: consistency with should ...

vince-fugnitto · 2021-09-17T19:06:56Z

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

+            expect(markerInfoNodeA.previousSibling).to.equal(markerInfoNodeB);
+        });
+
+        it('change marker content leading to higher rank', () => {


nitpick: consistency with should ...

vince-fugnitto · 2021-09-17T19:10:03Z

packages/markers/src/browser/problem/problem-composite-tree-node.ts

+        if (uri1 === uri2) {
+            return 0;
+        }
+        return compareStr(uri1.path.toString(), uri2.path.toString());


We can make use of our utility method that exists already:

theia/packages/core/src/common/uri.ts

Lines 202 to 210 in 8618183

isEqual(uri: URI, caseSensitive: boolean = true): boolean {

if (!this.hasSameOrigin(uri)) {

return false;

}

return caseSensitive

? this.toString() === uri.toString()

: this.toString().toLowerCase() === uri.toString().toLowerCase();

}

And localeCompare rather than compareStr?

I could resolve this with localeCompare

vince-fugnitto · 2021-09-21T12:17:04Z

@LukasBoll please ping me when you are ready for a final review 👍 I believe there are still unaddressed feedback from previous reviews.

LukasBoll · 2021-09-22T21:11:30Z

@vince-fugnitto I think everything has already been addressed.
I just rebased on master since there have been some new changes.

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts

packages/markers/src/browser/problem/problem-composite-tree-node.ts

colin-grant-work

Functionally this is working well for me; just one question before it's ready to go.

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

packages/markers/src/browser/problem/problem-composite-tree-node.ts

Fixes eclipse-theia#8983 Previously info-marker-nodes were ordered by the time they have been reported. This commit will sort the info-marker-nodes based on the following rules: - nodes will be sorted based on the most severe marker they contain - if nodes contain equally severe markers, they are sorted by the URI Signed-off-by: Lukas Boll <lukas-bool@web.de>

colin-grant-work

This is working well and the code looks good. @msujew, merge at your leisure.

max-elia reviewed Jul 5, 2021

View reviewed changes

vince-fugnitto added markers issues related to problem markers problems issues related to the problems widget labels Jul 5, 2021

LukasBoll force-pushed the fix/markernodes-order branch from 797a4ff to a910d74 Compare July 6, 2021 14:39

vince-fugnitto reviewed Jul 6, 2021

View reviewed changes

LukasBoll force-pushed the fix/markernodes-order branch from a910d74 to af6138b Compare July 7, 2021 10:47

msujew approved these changes Jul 16, 2021

View reviewed changes

LukasBoll changed the title ~~WIP: Implement order for info-marker-nodes~~ Implement order for info-marker-nodes Jul 23, 2021

vince-fugnitto reviewed Jul 23, 2021

View reviewed changes

vince-fugnitto reviewed Jul 27, 2021

View reviewed changes

LukasBoll force-pushed the fix/markernodes-order branch from af6138b to 4bf86c3 Compare July 29, 2021 20:47

vince-fugnitto requested a review from colin-grant-work August 16, 2021 18:39

LukasBoll force-pushed the fix/markernodes-order branch from 4bf86c3 to c4ffcc6 Compare August 18, 2021 06:59

vince-fugnitto reviewed Sep 17, 2021

View reviewed changes

LukasBoll force-pushed the fix/markernodes-order branch from c4ffcc6 to f05f888 Compare September 20, 2021 21:22

LukasBoll requested a review from vince-fugnitto September 20, 2021 21:29

LukasBoll force-pushed the fix/markernodes-order branch from f05f888 to 5ed0fc5 Compare September 22, 2021 20:38

colin-grant-work reviewed Sep 22, 2021

View reviewed changes

packages/markers/src/browser/problem/problem-composite-tree-node.spec.ts Outdated Show resolved Hide resolved

packages/markers/src/browser/problem/problem-composite-tree-node.ts Outdated Show resolved Hide resolved

LukasBoll force-pushed the fix/markernodes-order branch from 5ed0fc5 to fffd271 Compare September 25, 2021 21:57

LukasBoll requested a review from colin-grant-work September 25, 2021 22:02

colin-grant-work reviewed Sep 29, 2021

View reviewed changes

packages/markers/src/browser/problem/problem-tree-model.ts Outdated Show resolved Hide resolved

packages/markers/src/browser/problem/problem-composite-tree-node.ts Outdated Show resolved Hide resolved

LukasBoll force-pushed the fix/markernodes-order branch from fffd271 to 9097bbf Compare October 4, 2021 18:09

LukasBoll requested a review from colin-grant-work October 4, 2021 19:08

colin-grant-work approved these changes Oct 20, 2021

View reviewed changes

msujew merged commit 5261983 into eclipse-theia:master Oct 20, 2021

vince-fugnitto added this to the 1.19.0 milestone Oct 28, 2021

vince-fugnitto mentioned this pull request Dec 10, 2021

markers: improve marker info node sorting (resource) #7318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement order for info-marker-nodes #9691

Implement order for info-marker-nodes #9691

LukasBoll commented Jul 1, 2021

LukasBoll commented Jul 1, 2021

max-elia left a comment •

edited

Loading

max-elia Jul 5, 2021

vince-fugnitto Aug 16, 2021

LukasBoll Aug 17, 2021

LukasBoll Aug 18, 2021

max-elia Jul 5, 2021

max-elia Jul 5, 2021

LukasBoll commented Jul 6, 2021

vince-fugnitto Jul 6, 2021

vince-fugnitto Jul 6, 2021

vince-fugnitto Jul 6, 2021

vince-fugnitto Jul 6, 2021

LukasBoll commented Jul 7, 2021

msujew left a comment

vince-fugnitto commented Jul 20, 2021

LukasBoll commented Jul 23, 2021

vince-fugnitto Jul 23, 2021

vince-fugnitto left a comment

vince-fugnitto Jul 27, 2021

LukasBoll Jul 29, 2021

tsmaeder commented Sep 9, 2021

vince-fugnitto Sep 17, 2021

LukasBoll Sep 20, 2021

vince-fugnitto Sep 17, 2021

vince-fugnitto Sep 17, 2021

vince-fugnitto Sep 17, 2021

LukasBoll Sep 20, 2021

vince-fugnitto commented Sep 21, 2021

LukasBoll commented Sep 22, 2021

colin-grant-work left a comment

colin-grant-work left a comment

	id: 'theia-' + 'problem' + '-marker-widget',
	id: 'theia-problem-marker-widget',

	describe('Problem Composite Tree Node', () => {
	describe('problem-composite-tree-node', () => {

		@@ -0,0 +1,84 @@
		/********************************************************************************
		* Copyright (C) 2021 Ericsson and others.

		@@ -0,0 +1,310 @@
		/********************************************************************************
		* Copyright (C) 2021 Ericsson and others.

	isEqual(uri: URI, caseSensitive: boolean = true): boolean {
	if (!this.hasSameOrigin(uri)) {
	return false;
	}

	return caseSensitive
	? this.toString() === uri.toString()
	: this.toString().toLowerCase() === uri.toString().toLowerCase();
	}

Implement order for info-marker-nodes #9691

Implement order for info-marker-nodes #9691

Conversation

LukasBoll commented Jul 1, 2021

What it does

How to test

Review checklist

Reminder for reviewers

LukasBoll commented Jul 1, 2021

max-elia left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasBoll commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasBoll commented Jul 7, 2021

msujew left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Jul 20, 2021

LukasBoll commented Jul 23, 2021

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vince-fugnitto commented Sep 21, 2021

LukasBoll commented Sep 22, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work left a comment

Choose a reason for hiding this comment

max-elia left a comment •

edited

Loading