-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
markers: update the problem-marker decoration #7466
Conversation
Fixes #3819 The following commit updates the `problem-marker` decoration from the `explorer` to display the total count of errors and warnings for a given resource and updates container nodes (parent directories) to display a generic symbol. The following changes are aligned with the behavior with VS Code. Signed-off-by: vince-fugnitto <[email protected]>
@@ -127,10 +127,14 @@ export class ProblemDecorator implements TreeDecorator { | |||
} | |||
|
|||
protected toDecorator(marker: Marker<Diagnostic>): TreeDecoration.Data { | |||
// Determine if the given marker is for a resource of a container. | |||
const isResource = Array.from(this.problemManager.getUris()).some(m => m === marker.uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there is a better way to do this.
It'd be easy if I had access to the FileStat
and could determine if the marker's URI is a directory or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, do only files have a marker? Folders wouldn't match anything?
You could write const isResource = new Set(this.problemManager.getUris()).has(marker.uri);
and document why this test filters file from folders.
I don't see a clean way to get information about the filestat synchronously. Best we could do is make a bunch of modification to get data about the current node being decorated, and see what field we could add to the node (folder etc...)
We will need to add a strategy when dealing with multiple
and not
|
We also need to merge the decoration for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played with this change in my local environment. And I confirm it works as described in both single and multi-root workspace, with both warning and errors in a single file.
I'd like to see if we can be able to enforce an order for |
Anything else should be done here? |
@akosyakov I believe #7466 (comment) is blocking the pull-request at the moment. |
I pushed the changes to my fork and will revisit whenever I have some time: vince-fugnitto#9 |
What it does
Fixes #3819
The following commit updates the
problem-marker
decoration from theexplorer
to display the total count of errors and warnings for a given resource and updates container nodes (parent directories) to display a generic symbol. The following changes are aligned with the behavior with VS Code.How to test
Review checklist
Reminder for reviewers
Signed-off-by: vince-fugnitto [email protected]