Refactore report tree#107
Conversation
- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.
csordasmarton
force-pushed
the
refactore_report_tree
branch
from
99c892c to
cb42347
Compare
Discookie
left a comment
Discookie
left a comment
There was a problem hiding this comment.
Found a pretty big bug in it:
With the repr. steps shown, going to a different file and then back to the original file causes this issue:
(No logs other than that to add here)
From that point, the Reports in current file view is empty, and the error shows for all other files opened.
This is probably why the tests failed as well.
Other than that, the code looks fine with minor nitpicks.
| ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter()); | ||
| window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions); | ||
| window.onDidChangeVisibleTextEditors((e: TextEditor[]) => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?
| window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions); | ||
| ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions); | ||
| window.onDidChangeActiveTextEditor(() => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Same question, why is setTimeout needed here?
| const selectedIds = new Set(this.selectedTreeItems.map(item => item.id)); | ||
| this.treeItems.forEach(root => root.traverse(item => { | ||
| if (selectedIds.has(item.id)) { | ||
| this.tree?.reveal(item, { select: true, focus: true }).then(() => {}, () => {}); |
There was a problem hiding this comment.
Why is that empty .then needed here? Use the void operator instead.
| // No children of sticky when there's no selected report | ||
| if (entry === undefined) { | ||
| return []; | ||
| const severityItems: { [key: string]: TreeDiagnosticReport[] } = {}; |
There was a problem hiding this comment.
I'd prefer to use a Map here, the flow shouldn't be much different.
|
Continued in #112. |
2 participants
Screenshot:
