markers: apply problem marker sorting based on severity, line, and column number #7313

vince-fugnitto · 2020-03-11T01:03:27Z

What it does

Fixes #6572

The following commit updates the markers to be sorted based on the following rules:

markers for a given resource are first sorted by severity
if the severity are equal, sort by lowest line number
if the severity and line number are equal, sort by lowest column number
if all props are equal, do not perform a sort

How to test

perform changes that will result in diagnostics
verify that the rules hold true:

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

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

elaihau · 2020-03-11T02:35:22Z

do we do any sortings based on the file paths?

in the github issue you mentioned

markers should be sorted by resource
for a given resource, markers should be sorted by severity, line and column number

so can i assume that the path-based sorting should also be supported with your change?

vince-fugnitto · 2020-03-11T13:46:25Z

do we do any sortings based on the file paths?

I don't do any sorting at the moment for MarkerInfoNode (file nodes) based on their URI.
In order to align, I'll update the code to also handle the sorting of file nodes.

vince-fugnitto · 2020-03-11T15:15:10Z

@elaihau there does not seem to be any sorting of files at the moment for the problems view, and it looks like we add new problems to the bottom of the view as the files are opened (with markers). This pull-request was intended to fix the issue of for a given resource, to sort it's diagnostics with a ruleset. Can we include the improvement of actual resource sorting as a different pull-request as it was not the intention of this pull-request?

elaihau

I tested in gitpod.

Worked properly given that sorting by file paths is not in the scope of this pull request.

Thank you Vincent !

elaihau · 2020-03-12T03:28:06Z

I just noticed that this pr has "fixes 6572" in its description, maybe we could create a new GH issue to keep track of the file path sorting issue.

akosyakov · 2020-03-12T08:27:23Z

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

+import { Marker } from '../../common/marker';
+import { Diagnostic } from '@theia/languages/lib/browser';
+
+export namespace ProblemUtils {


(nit) I would better to add them as static method to ProblemTree. Utils always feels like it is useful to share but I don't know what is a good concept for it.

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

akosyakov · 2020-03-12T08:29:25Z

Did you test it on performance?

cc @marcdumais-work do you remember how we used to test #2316 against java? I remember we managed to create like over 40000 problem markers, too bad that we did not have a rule to write down test by then.

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>

vince-fugnitto · 2020-03-12T12:09:34Z

Did you test it on performance?

I could not find any noticeable degredation. I updated one of our eslint rules (single-quote to double-quote) and ran a task with problem-matchers. I generated 3k markers (noticeably a lot less than the mentioned 40k) and did not notice the sorting taking too long.

I will try to do more performance testing with numbers if helpful.

akosyakov · 2020-03-12T12:23:20Z

@vince-fugnitto it is probably fine, 3k is not enough though, i think we used to open eclipse java ls project. Initially it should report tons of error markers for different resources.

vince-fugnitto · 2020-03-12T13:59:18Z

@vince-fugnitto it is probably fine, 3k is not enough though, i think we used to open eclipse java ls project. Initially it should report tons of error markers for different resources.

I verified using the eclipse.jdt.ls project (which generates ~30k markers).
With 3 runs (master and this pull-request) I got the following results:

Trial	`master`	`sorting`
1	11.13	11.93
2	10.90	11.05
3	11.23	11.63
Avg (sec)	11.09	11.54

I timed the time it takes once reset workbench layout is called until the time 30k markers were identified.

lmcbout · 2020-03-16T15:21:36Z

Questions
1- Should we consider sorting the files in the problem view, According to the picture below, the files are not sorted. What about if we have a multi-root, could we sort the files starting with :
root -> filename -> severity ->line number !

2- What is the big number just before the (line,colums) It does not seems to give the end-user any information, I don't see the signification of that number,

If I just look at sorting inside a file Severity -line number, it seems to work fine

vince-fugnitto · 2020-03-16T15:26:37Z

@lmcbout

Questions
1- Should we consider sorting the files in the problem view, According to the picture below, the files are not sorted. What about if we have a multi-root, could we sort the files starting with :
root -> filename -> severity ->line number !

Please see past discussions:

markers: apply problem marker sorting based on severity, line, and column number #7313 (comment)
markers: apply problem marker sorting based on severity, line, and column number #7313 (comment)
markers: improve marker info node sorting (resource) #7318

2- What is the big number just before the (line,colums) It does not seems to give the end-user any information, I don't see the signification of that number,

It is the code.
For example: '536870973' is for 'value of local variable is not used'.

lmcbout · 2020-03-16T15:50:57Z

Please see past discussions:

From @elaihau commnet:

I just noticed that this pr has "fixes 6572" in its description, maybe we could create a new GH issue to keep track of the file path sorting issue.

I agree that we should have a new issue raised and have a link in this PR or in "6572"

For question #2
It is the code.
For example: '536870973' is for 'value of local variable is not used'.

Unless a user can determine what the number means, either we provide a link or when hover the number, it points to its description, I don't see the purpose to display that big number, it just clutter the display unless we can refer to it by some means

marcdumais-work · 2020-03-16T16:01:44Z

Unless a user can determine what the number means, either we provide a link or when hover the number, it points to its description, I don't see the purpose to display that big number, it just clutter the display unless we can refer to it by some means

There is not necessarily more info in the output that was parsed to generate the Marker. Anyway the error codes should be familiar to developers for a given language - generally compilers will output the same, without any more context

e.g. error 2515 generated by tsc:

packages/terminal/src/node/terminal-server.ts:27:14 - error TS2515: Non-abstract class 'TerminalServer' does not implement inherited abstract member 'create' from class 'BaseTerminalServer'.

vince-fugnitto · 2020-03-16T16:17:50Z

@lmcbout

Please see past discussions:

From @elaihau commnet:

I just noticed that this pr has "fixes 6572" in its description, maybe we could create a new GH issue to keep track of the file path sorting issue.

As mentioned, it is already tracked: #7318

I agree that we should have a new issue raised and have a link in this PR or in "6572"

For question #2
It is the code.
For example: '536870973' is for 'value of local variable is not used'.

Unless a user can determine what the number means, either we provide a link or when hover the number, it points to its description, I don't see the purpose to display that big number, it just clutter the display unless we can refer to it by some means

It displays the code and it is consistent with vscode.

A user can perform a search based on the code, but providing links would be difficult since where would we point to exactly? In any case, this is noise in the pull-request as the code was introduced a long time ago (#3141).

lmcbout

Fine

lmcbout

OK

vince-fugnitto · 2020-03-17T14:23:19Z

I'll merge tomorrow if there are no objections.

vince-fugnitto added the markers issues related to problem markers label Mar 11, 2020

vince-fugnitto requested a review from elaihau March 11, 2020 01:03

vince-fugnitto self-assigned this Mar 11, 2020

elaihau approved these changes Mar 11, 2020

View reviewed changes

akosyakov reviewed Mar 12, 2020

View reviewed changes

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

akosyakov reviewed Mar 12, 2020

View reviewed changes

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

vince-fugnitto force-pushed the vf/GH-6572 branch from 9190d48 to 3eafd72 Compare March 12, 2020 10:36

vince-fugnitto requested review from marcdumais-work and lmcbout March 13, 2020 13:36

lmcbout approved these changes Mar 16, 2020

View reviewed changes

vince-fugnitto merged commit 9b7476b into master Mar 18, 2020

vince-fugnitto deleted the vf/GH-6572 branch March 18, 2020 19:33

vince-fugnitto mentioned this pull request Mar 15, 2021

problems-view: problems order changes on preference changes #8983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

markers: apply problem marker sorting based on severity, line, and column number #7313

markers: apply problem marker sorting based on severity, line, and column number #7313

vince-fugnitto commented Mar 11, 2020

elaihau commented Mar 11, 2020 •

edited

Loading

vince-fugnitto commented Mar 11, 2020

vince-fugnitto commented Mar 11, 2020

elaihau left a comment

elaihau commented Mar 12, 2020

akosyakov Mar 12, 2020

akosyakov commented Mar 12, 2020 •

edited

Loading

vince-fugnitto commented Mar 12, 2020

akosyakov commented Mar 12, 2020 •

edited

Loading

vince-fugnitto commented Mar 12, 2020

lmcbout commented Mar 16, 2020

vince-fugnitto commented Mar 16, 2020 •

edited

Loading

lmcbout commented Mar 16, 2020

marcdumais-work commented Mar 16, 2020

vince-fugnitto commented Mar 16, 2020 •

edited

Loading

lmcbout left a comment

lmcbout left a comment

vince-fugnitto commented Mar 17, 2020

markers: apply problem marker sorting based on severity, line, and column number #7313

markers: apply problem marker sorting based on severity, line, and column number #7313

Conversation

vince-fugnitto commented Mar 11, 2020

What it does

How to test

Review checklist

Reminder for reviewers

elaihau commented Mar 11, 2020 • edited Loading

vince-fugnitto commented Mar 11, 2020

vince-fugnitto commented Mar 11, 2020

elaihau left a comment

Choose a reason for hiding this comment

elaihau commented Mar 12, 2020

akosyakov Mar 12, 2020

Choose a reason for hiding this comment

akosyakov commented Mar 12, 2020 • edited Loading

vince-fugnitto commented Mar 12, 2020

akosyakov commented Mar 12, 2020 • edited Loading

vince-fugnitto commented Mar 12, 2020

lmcbout commented Mar 16, 2020

vince-fugnitto commented Mar 16, 2020 • edited Loading

lmcbout commented Mar 16, 2020

marcdumais-work commented Mar 16, 2020

vince-fugnitto commented Mar 16, 2020 • edited Loading

lmcbout left a comment

Choose a reason for hiding this comment

lmcbout left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Mar 17, 2020

elaihau commented Mar 11, 2020 •

edited

Loading

akosyakov commented Mar 12, 2020 •

edited

Loading

akosyakov commented Mar 12, 2020 •

edited

Loading

vince-fugnitto commented Mar 16, 2020 •

edited

Loading

vince-fugnitto commented Mar 16, 2020 •

edited

Loading