Show error and warning indicators in project panel items #18182

nilskch · 2024-09-21T20:00:39Z

Release Notes:

Add setting to display error and warning indicators in project panel items.

demo_errors.mov

nilskch

Hey, I love Zed and everything you do for the community! I was missing this feature a lot and many people asked for it and I wanted to give it a try. I hope this is something that you would want for Zed, let me know if I should make any changes.

nilskch · 2024-09-21T20:05:52Z

crates/project_panel/src/project_panel.rs

+                if diagnostic_summary.error_count > 0 {
+                    Some((path, Color::Error))
+                } else {
+                    None
+                }


I thought about doing this:

if diagnostic_summary.error_count > 0 { Some((path, Color::Error)) } else if diagnostic_summary.warning_count > 0 { Some((path, Color::Warning)) } else { None }

I didn't like that and removed it. We could make this configurable if you want?

nilskch · 2024-09-22T08:04:32Z

crates/project_panel/src/project_panel.rs

@@ -82,6 +82,7 @@ pub struct ProjectPanel {
    show_scrollbar: bool,
    scrollbar_drag_thumb_offset: Rc<Cell<Option<f32>>>,
    hide_scrollbar_task: Option<Task<()>>,
+    diagnostic_error_colors: HashMap<PathBuf, Color>,


if we do want to support showing warnings in the project panel, this should be a HashMap. If we only want to show errors we can make this a HashSet and not store the colors.

nilskch · 2024-09-22T16:35:56Z

Okay I played around a little bit and I have a concrete suggestion:
Let's add a "show_diagnostic_errors" bool property in the "project_panel" object in the settings.json. I would use a default value of true, but happy to make it false if you like it better.

I would not show warnings in the project panel. I think this causes too much unnecessary noise. The status GitFileStatus::Modified paints the items with the color Color::Modified which is very similar to Color::Warning (at least in the theme One Light) and this is very confusing.

If people want to display warnings as well, I am happy to make a follow-up PR but I honestly do not think someone wants this.

I will implement this and add the docs next week if you think this would be a good add.

nilskch · 2024-09-23T19:36:29Z

I implemented it the way I suggested in the last comment. Looking forward to your feedback on this!

demo_new.mov

nilskch · 2024-09-24T08:19:22Z

@mrnugget can you please take a look at this PR ? Tagging you because of the time zone^^

mrnugget

Thank you! Left a bunch of comments :)

mrnugget · 2024-09-24T12:49:16Z

crates/project_panel/src/project_panel.rs

+                    if new_settings.show_diagnostic_errors
+                        && !project_panel_settings.show_diagnostic_errors
+                    {
+                        this.update_diagnostics(cx);
+                    }


What if you disable it? Don't you then have to reset self.diagnostic_error_paths to be empty?

@mrnugget my thought was that self.diagnostic_error_paths is not used and updated once you enable the setting again and therefore it's not necessary to reset it. On the other hand this wastes memory, I will reset it.

crates/project_panel/src/project_panel_settings.rs

mrnugget · 2024-09-24T13:02:04Z

crates/project_panel/src/project_panel.rs

@@ -82,6 +82,7 @@ pub struct ProjectPanel {
    show_scrollbar: bool,
    scrollbar_drag_thumb_offset: Rc<Cell<Option<f32>>>,
    hide_scrollbar_task: Option<Task<()>>,
+    diagnostic_error_paths: HashSet<PathBuf>,


I think you need something like HashMap<WorktreeId, HashSet<PathBuf>, otherwise you'll have global diagnostic errors, even if they appear in a different worktree.

Example:

Here I have an error in the rust-proj, but the go-proj is also marked red.

(cc @bennetbo who found this bug)

I didn't know you can do this in Zed, thanks for flagging!

Takes a while to digest that and then you see it everywhere :)

mrnugget · 2024-09-24T13:03:00Z

cc @danilo-leal in case you have thoughts wrt to design

bennetbo · 2024-09-24T13:14:22Z

If people want to display warnings as well, I am happy to make a follow-up PR but I honestly do not think someone wants this.\

I'd like to have this and would prefer to add this in this PR as well, so that we do not have to break the settings right after this is released.
Maybe we could have a setting like (not sure about the names of the enum variants):
"show_diagnostics": "off/all/errors"

I would not show warnings in the project panel. I think this causes too much unnecessary noise. The status GitFileStatus::Modified paints the items with the color Color::Modified which is very similar to Color::Warning (at least in the theme One Light) and this is very confusing.

Let's add the warnings as an option and we will figure out how to make it "pretty" later.

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>

nilskch

@mrnugget @bennetbo thanks for the feedback! I think I addressed your comments now. Let me know how you want the settings to look like and what defaults we should use.

crates/project_panel/src/project_panel.rs

nilskch · 2024-09-24T19:17:35Z

crates/project_panel/src/project_panel.rs

+                self.update_highest_diagnostic_severity(
+                    &project_path,
+                    path_buffer.clone(),
+                    diagnostic_severity,
+                );


This here is necessary to also include the root directory of the worktree. We need the empty buffer in the HashMap as well.

nilskch · 2024-09-24T19:20:59Z

crates/project_panel/src/project_panel.rs

+                *max_diagnostic_severity =
+                    std::cmp::min(*max_diagnostic_severity, diagnostic_severity);


It's a little bit confusing, but DiagnosticSeverity::ERROR < DiagnosticSeverity::WARNING and I think errors should overshadow warnings here.

EDIT: renamed to strongest_diagnostic_severity

nilskch · 2024-09-24T19:28:56Z

crates/project_panel/src/project_panel.rs

+        self.diagnostics = Default::default();
+        self.project


is it better here to store everything in a local HashMap first and assign the local variable at the end to self.diagnostics?

Yeah, I think that's how I'd do it, would make the code nicer, IMHO

nilskch · 2024-09-25T07:05:45Z

Hey so this is how it looks like with warnings and the settings as @bennetbo suggested:

errors_warnings.mov

PS: I changed my GPG key last night and apparently you should not remove the old public key from your profile, otherwise old commits show up as unverified. It's still me, just wiser.

mrnugget · 2024-09-25T08:16:51Z

One thing that I'm now wondering about: how will a modified file look different from a file with warnings? Right now, modified files look like this:

That's also the has-a-warning style, no?

nilskch · 2024-09-25T08:41:07Z

@mrnugget in the last comment I posted a video where you can see the difference when I changed the settings. In the theme "One Light" the warning color is a lot brighter than the modified color. Tbh, I am not the biggest fan of having both, warnings and git modified.

Let's add the warnings as an option and we will figure out how to make it "pretty" later.

Not sure how much effort it is to make it "pretty", but maybe we should do that before we introduce this feature? The contrast of the warnings color is very bad for the selected file (see video).

danilo-leal · 2024-09-25T11:18:17Z

I'd avoid using only colors for these statuses—different themes will have different color qualities, and relying only on them can be very tricky. We can't guarantee that it will look great in all themes, and folks with any visual impairments can have an even harder time!

That said, I'd consider adding the same icons we display on the status bar for errors and warnings and simple dot indicators for the Git-related statuses (added & modified) to the project item. I'd also add tooltips to them.

zackangelo · 2024-09-25T14:15:21Z

Thank you for working on this! It's the only thing keeping me from using Zed as my main editor at this point. 🙂

nervenes · 2024-11-03T22:36:44Z

I really like the design, but as far as I understood, the Zed team is working on horizontal scrolling in the project panel at the moment and I do not think the A/M icons would work with that. I think it is best to wait for feedback from the Zed team before implementing more designs :)

Hey, i've just created a continuation PR for the git symbols in the project panel (and editor tabs), might want to check it out: #20134

My two cents on the design here, is adding the existing error/warning icons to the end slot, where error takes precedence over warning, these icons should be additional to the git symbols i'm adding i.e not to replace them, they could be on the left of U/M/C.

For coloring, red for error and yellow for warning could take precedence over the git coloring, as the entries will have their respective git symbols if my PR gets merged. Project diagnostics overall sounds to me should have higher priority, i think this is how VSCode and Xcode also does it.

This would go in hand with the changes i've done in my PR that i mentioned.

Co-authored by: Danilo Leal <danilo@zed.dev> Co-authored by: Peter Tripp <peter@zed.dev>

…ect-panel

crates/project_panel/src/project_panel.rs

danilo-leal

I'll merge this PR for now; feeling good about it! Thanks so much @nilskch and everyone else for the continued effort & contribution! We'll tackle any upcoming feedback iteratively on future PRs.

nervenes · 2024-11-13T11:35:24Z

this PR removed git coloring for icons in the project panel? 🤔

Angelk90 · 2024-11-13T15:32:56Z

@nilskch , @danilo-leal :

When set:

"project_panel": {
    "folder_icons": false
  }

How is it displayed?

danilo-leal · 2024-11-13T15:46:29Z

@Angelk90 like so. You'll be able to get these changes on the upcoming version this week!

@nervenes yeah, at least by default, we're aiming to have Git status only affect panel item labels for now. The icons also being affected should probably be a setting you'd turn on.

Angelk90 · 2024-11-13T15:52:50Z

@danilo-leal : You are always the best.

P.s.
How do you make zed screenshots without background?

nervenes · 2024-11-13T17:07:42Z

@danilo-leal i see, i know this is just an initial implementation but i think there are some things that can be cleared up, im trying to think from the average users perspective, but y'all know the best :>

if i were new, and the project panel showed me warning and error indicators on the file icon for diagnostics, and the file name was yellow (modified) or red (conflict) for git, and i solved the warning/error diagnostic problem but the filename is still colored yellow/red (for git) would confuse me, and vice versa if i solved a git problem and the yellow/red filename color went away while the warning/error indicator for diagnostics persisted. what's even more confusing now is that diagnostics coloring and git coloring for filenames take mixed priority, error diagnostics take highest priority, git takes second priority than warning diagnostics, and i know this because i've followed through the development of this but will the regular user know this?

if this is the path that is wanted, then maybe it would be better to either pick diagnostics or git (not both) for coloring? so i'd imagine with coloring mode to be set to diagnostics, it would color filenames either red or yellow, with no conflicting git colors, and git symbols (when/if merged) will be used to infer git information. or the opposite, that git coloring is enabled and you'd only infer diagnostics from the indicators (but the problem here is that the indicators are colored and small, so it still may not be clear to the user what it's actually for). i would personally recommend the former.

this is pushing me back on my git aware implementations for project panel entries and editor tabs, and i don't know what the vision/plan is here.

danilo-leal · 2024-11-13T17:41:05Z

@nervenes I just pushed a PR earlier today that I think goes along your latter suggestion #20600. I hear you about the icons being potentially not clear, though. I guess we could solve it via a tooltip. Like, if you hover the file with the X error icon, you could see something like "There's an error" or a better label.

nervenes · 2024-11-13T17:48:39Z

additionally, maybe clicking on the tooltip could take you to the particular error/warning in the diagnostic panel? also, is there any other place i could have a discussion with you, maybe any of the channels in the discord channel? i'd like to ask some questions about the git symbols, or should i open up an empty pr (that i'll later use for the implementation)?

hawkinsonb · 2024-11-14T16:44:01Z

@danilo-leal @nervenes Why not strike-through the filename for a git removal and use a darker red color if you need a color indicator? That way it very clearly separates it from diagnostics red, while adding a visual cue that a file was removed. This would accomplish the separation of functionality you are looking for while making the UX much nicer and explicit.

danilo-leal · 2024-11-15T13:53:54Z

@nervenes interesting idea! Hit me up on Zed's Discord. You can fine me there!

@hawkinsonb curious to explore that, yeah. Thanks for the suggestion!

show errors in project panel

9116e13

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Sep 21, 2024

nilskch commented Sep 21, 2024

View reviewed changes

nilskch added 2 commits September 21, 2024 22:19

color root directory as well

993c727

fix clippy

09bb386

nilskch commented Sep 22, 2024

View reviewed changes

add configuration for diagnostic errors

5dd0f94

nilskch added 4 commits September 23, 2024 21:17

some optimizations

bc64eb2

nit

18085fa

docs nit

4bebe91

final cleanup

0761d36

mrnugget reviewed Sep 24, 2024

View reviewed changes

mrnugget assigned mrnugget and bennetbo Sep 24, 2024

nilskch and others added 2 commits September 24, 2024 17:31

Update crates/project_panel/src/project_panel_settings.rs

d1b3217

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>

add warnings to project panel

eaa0937

nilskch commented Sep 24, 2024

View reviewed changes

nilskch added 3 commits September 24, 2024 20:35

rename var

cddc98e

cleanup

53f07c5

remove log dependency again

108ccf2

mrnugget mentioned this pull request Nov 4, 2024

Improve editor tabs and project panel items #20134

Closed

mrnugget and others added 11 commits November 7, 2024 18:56

project panel: Make diagnostic error status overwrite git status

009db72

Co-authored by: Danilo Leal <danilo@zed.dev> Co-authored by: Peter Tripp <peter@zed.dev>

Merge branch 'main' into pr/18182

49cb5d1

Checkpoint: Create a dedicated icon for panel diagnostic display

fc15825

Merge branch 'main' into pr/18182

52bb56e

Merge branch 'zed-industries:main' into feat/highlight-errors-in-proj…

7b6f663

…ect-panel

Merge branch 'main' into pr/18182

37b1f49

Use the new DecoratedIcon component

a2b51d3

Organize colors used for each state and scenario

73d1334

Remove unused SVGs

88573c5

Iterate on the setting's writing

06cb366

Iterate on the triangle knockout icon

2f3b8f3

nilskch commented Nov 12, 2024

View reviewed changes

crates/project_panel/src/project_panel.rs Show resolved Hide resolved

danilo-leal approved these changes Nov 12, 2024

View reviewed changes

danilo-leal merged commit 0a9c78a into zed-industries:main Nov 12, 2024
11 checks passed

WeetHet mentioned this pull request Nov 13, 2024

Error indicators in project panel should use different border for selected files #20572

Closed

1 task

nilskch deleted the feat/highlight-errors-in-project-panel branch November 13, 2024 12:22

hrm1810884 mentioned this pull request Nov 20, 2024

:zenn:zed導入ログ hrm1810884/Writing#3

Open

This was referenced Nov 25, 2024

Show error and warning indicators in tabs #21179

Closed

Show error and warning indicators in tabs #21383

Merged

		*max_diagnostic_severity =
		std::cmp::min(*max_diagnostic_severity, diagnostic_severity);

Show error and warning indicators in project panel items #18182

Show error and warning indicators in project panel items #18182

Conversation

nilskch commented Sep 21, 2024 • edited by danilo-leal Loading

nilskch left a comment

Choose a reason for hiding this comment

nilskch Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

nilskch Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

nilskch commented Sep 22, 2024

nilskch commented Sep 23, 2024

nilskch commented Sep 24, 2024

mrnugget left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnugget commented Sep 24, 2024

bennetbo commented Sep 24, 2024

nilskch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilskch commented Sep 25, 2024

mrnugget commented Sep 25, 2024

nilskch commented Sep 25, 2024

danilo-leal commented Sep 25, 2024

zackangelo commented Sep 25, 2024

nervenes commented Nov 3, 2024 • edited Loading

danilo-leal left a comment

Choose a reason for hiding this comment

nervenes commented Nov 13, 2024

Angelk90 commented Nov 13, 2024

danilo-leal commented Nov 13, 2024

Angelk90 commented Nov 13, 2024

nervenes commented Nov 13, 2024 • edited Loading

danilo-leal commented Nov 13, 2024

nervenes commented Nov 13, 2024 • edited Loading

hawkinsonb commented Nov 14, 2024

danilo-leal commented Nov 15, 2024

nilskch commented Sep 21, 2024 •

edited by danilo-leal

Loading

nilskch Sep 21, 2024 •

edited

Loading

nilskch Sep 22, 2024 •

edited

Loading

nervenes commented Nov 3, 2024 •

edited

Loading

nervenes commented Nov 13, 2024 •

edited

Loading

nervenes commented Nov 13, 2024 •

edited

Loading