Problems View does not display all Problems when they are reported to fast

meisenbarth-work commented 1 month ago

Bug Description:

The Problems View does not display all problems in the tree when problems are reported too fast. The counter at the panel is correct, but problems are missing,

The problems are added to an array, the adding of the problems is delayed, probably for performance reasons. https://github.com/eclipse-theia/theia/blob/63585530b2e32af0ed4bac4edb5ff443bfc248b7/packages/markers/src/browser/problem/problem-tree-model.ts#L82

The array is sorted, changing the order within the array. https://github.com/eclipse-theia/theia/blob/63585530b2e32af0ed4bac4edb5ff443bfc248b7/packages/markers/src/browser/problem/problem-tree-model.ts#L87 https://github.com/eclipse-theia/theia/blob/63585530b2e32af0ed4bac4edb5ff443bfc248b7/packages/markers/src/browser/problem/problem-composite-tree-node.ts#L41

The nodes are added, since the order changed, the node without errors and only warnings replaces the one with errors. https://github.com/eclipse-theia/theia/blob/63585530b2e32af0ed4bac4edb5ff443bfc248b7/packages/markers/src/browser/problem/problem-tree-model.ts#L89 https://github.com/eclipse-theia/theia/blob/63585530b2e32af0ed4bac4edb5ff443bfc248b7/packages/markers/src/browser/problem/problem-tree-model.ts#L92 https://github.com/eclipse-theia/theia/blob/63585530b2e32af0ed4bac4edb5ff443bfc248b7/packages/core/src/browser/tree/tree.ts#L349

It looks like the problem manager should provide all current problems for a specific file. Thus I assume it would be okay to remove other occurrences of the node from the markers array before adding a new entry? Otherwise a copy of the markers array could be sorted, but it looks like a waste to add nodes just to remove them in the next step again.

Steps to Reproduce:

Have a Problem Watcher that handles output
Have first warnings and than an error in the output, the error must be less than 50ms away from the last warning
The error is not displayed

Additional Information

Operating System: Windows 10
Theia Version: 1.45

tsmaeder commented 1 month ago

If I understand correctly, the problem already exists once we do

 this.markers.push({ node, markers });

If the line is invoked twice before the debounced doInsertNodesWithMarkers() method is invoked, we end up with two entries in this.markers for the same node. Generally, I wonder why this model is so complicated: can't we just refresh the nodes and let the virtualized tree take care of performance issues?

@meisenbarth-work since you seem to have analyzed this problem thoroughly, would you fancy doing a PR to fix it?

meisenbarth-work commented 1 month ago

The debounce and array are from https://github.com/eclipse-theia/theia/pull/12408/files#diff-62bf371138c21435be92f9a1aedb041297aa9623f9218736eb930aff29dcd3f1 to fix a performance issue. Removing this (again) is probably not desired.

Regarding the PR, i would rather not, since I don't know how (technically and legally).

msujew commented 3 weeks ago

This will be fixed in https://github.com/eclipse-theia/theia/pull/13841. I initially only wanted to fix https://github.com/eclipse-theia/theia/issues/13835, but the root problem is the same - multiple nodes for the same file in the problem widget mess with the tree widget implementation. By replacing the array with a map, we circumvent the issue completely.

tsmaeder commented 3 weeks ago

@meisenbarth-work could you check your use case once the linked PR is merged, please?

meisenbarth-work commented 2 weeks ago

Sorry for the late reply, i can confirm the bug could not be reproduced anymore after applying the fix. Thanks a lot.

eclipse-theia / theia