Refactore report tree by csordasmarton · Pull Request #107 · Ericsson/CodecheckerVSCodePlugin

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton · 2022-04-19T12:08:55Z

Closes#59 and closes#40

Refactore the report tree's code.
Display depth of report steps.
Group reports by severity levels.

Screenshot:

- Refactore the report tree's code. - Display depth of report steps. - Group reports by severity levels.

Discookie

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.

Discookie · 2022-04-20T22:05:06Z

 ctx.subscriptions.push(this._diagnosticsUpdated = new EventEmitter());
- window.onDidChangeVisibleTextEditors(this.onDocumentsChanged, this, ctx.subscriptions);
+ window.onDidChangeVisibleTextEditors((e: TextEditor[]) => {
+ setTimeout(() => {


Why do we need to put this in a setTimeout, instead of into onDocumentsChanged directly?

Discookie · 2022-04-20T23:08:33Z

- window.onDidChangeActiveTextEditor(this.refreshBugList, this, ctx.subscriptions);
- ExtensionApi.diagnostics.diagnosticsUpdated(this.refreshBugList, this, ctx.subscriptions);
+ window.onDidChangeActiveTextEditor(() => {
+ setTimeout(() => {


Same question, why is setTimeout needed here?

Discookie · 2022-04-20T23:39:41Z

+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(() => {}, () => {});


Why is that empty .then needed here? Use the void operator instead.

Discookie · 2022-04-20T23:41:50Z

-// No children of sticky when there's no selected report
-if (entry === undefined) {
-return [];
+ const severityItems: { [key: string]: TreeDiagnosticReport[] } = {};


I'd prefer to use a Map here, the flow shouldn't be much different.

Discookie · 2022-07-08T09:30:16Z

Continued in #112.

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

csordasmarton added enhancement New feature or request refactoring labels Apr 19, 2022

csordasmarton added this to the 1.4.0 milestone Apr 19, 2022

csordasmarton requested a review from Discookie April 19, 2022 12:08

Refactore report tree
cb42347
- 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 cb42347Compare April 19, 2022 12:18

Discookie requested changes Apr 20, 2022
View reviewed changes

Discookie mentioned this pull request Jul 8, 2022
Refactor report tree #112
Merged

Discookie closed this Jul 8, 2022

Uh oh!

Conversation

csordasmarton commented Apr 19, 2022

Uh oh!

Discookie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Discookie commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

csordasmarton commented Apr 19, 2022

Uh oh!

Discookie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Discookie commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

csordasmarton commented Apr 19, 2022

Uh oh!

Discookie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Discookie commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

csordasmarton commented Apr 19, 2022

Uh oh!

Discookie left a comment