From ed3d3dc690890b654c88d7ca3809bb3fd90580f2 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 18 Jul 2024 17:22:38 +0300 Subject: [PATCH] Improve same line diagnostic rendering (#14741) Follow-up of https://github.com/zed-industries/zed/pull/14515 Fixed certain visual artifacts, related to multi line diagnostics and block toggle rendering. Also enabled diagnostics toolbar controls for the experimental view too. Release Notes: - N/A --- crates/diagnostics/src/diagnostics.rs | 2 +- crates/diagnostics/src/grouped_diagnostics.rs | 123 +++++++++--------- crates/diagnostics/src/toolbar_controls.rs | 98 ++++++++++---- crates/editor/src/editor.rs | 24 ++-- 4 files changed, 152 insertions(+), 95 deletions(-) diff --git a/crates/diagnostics/src/diagnostics.rs b/crates/diagnostics/src/diagnostics.rs index 377fa50700..95f7ea92ce 100644 --- a/crates/diagnostics/src/diagnostics.rs +++ b/crates/diagnostics/src/diagnostics.rs @@ -4,7 +4,7 @@ mod toolbar_controls; #[cfg(test)] mod diagnostics_tests; -mod grouped_diagnostics; +pub(crate) mod grouped_diagnostics; use anyhow::Result; use collections::{BTreeSet, HashSet}; diff --git a/crates/diagnostics/src/grouped_diagnostics.rs b/crates/diagnostics/src/grouped_diagnostics.rs index e38298312e..92c4a7be32 100644 --- a/crates/diagnostics/src/grouped_diagnostics.rs +++ b/crates/diagnostics/src/grouped_diagnostics.rs @@ -51,18 +51,18 @@ pub fn init(cx: &mut AppContext) { .detach(); } -struct GroupedDiagnosticsEditor { - project: Model, +pub struct GroupedDiagnosticsEditor { + pub project: Model, workspace: WeakView, focus_handle: FocusHandle, editor: View, summary: DiagnosticSummary, excerpts: Model, path_states: Vec, - paths_to_update: BTreeSet<(ProjectPath, LanguageServerId)>, - include_warnings: bool, + pub paths_to_update: BTreeSet<(ProjectPath, LanguageServerId)>, + pub include_warnings: bool, context: u32, - update_paths_tx: UnboundedSender<(ProjectPath, Option)>, + pub update_paths_tx: UnboundedSender<(ProjectPath, Option)>, _update_excerpts_task: Task>, _subscription: Subscription, } @@ -260,7 +260,7 @@ impl GroupedDiagnosticsEditor { } } - fn toggle_warnings(&mut self, _: &ToggleWarnings, cx: &mut ViewContext) { + pub fn toggle_warnings(&mut self, _: &ToggleWarnings, cx: &mut ViewContext) { self.include_warnings = !self.include_warnings; self.enqueue_update_all_excerpts(cx); cx.notify(); @@ -297,7 +297,7 @@ impl GroupedDiagnosticsEditor { /// to have changed. If a language server id is passed, then only the excerpts for /// that language server's diagnostics will be updated. Otherwise, all stale excerpts /// will be refreshed. - fn enqueue_update_stale_excerpts(&mut self, language_server_id: Option) { + pub fn enqueue_update_stale_excerpts(&mut self, language_server_id: Option) { for (path, server_id) in &self.paths_to_update { if language_server_id.map_or(true, |id| id == *server_id) { self.update_paths_tx @@ -320,7 +320,7 @@ impl GroupedDiagnosticsEditor { }); // TODO kb change selections as in the old panel, to the next primary diagnostics - // TODO kb make [shift-]f8 to work, jump to the next block group + // TODO make [shift-]f8 to work, jump to the next block group let _was_empty = self.path_states.is_empty(); let path_ix = match self.path_states.binary_search_by(|probe| { project::compare_paths((&probe.path.path, true), (&path_to_update.path, true)) @@ -340,7 +340,6 @@ impl GroupedDiagnosticsEditor { } }; - // TODO kb when warnings are turned off, there's a lot of refresh for many paths happening, why? let max_severity = if self.include_warnings { DiagnosticSeverity::WARNING } else { @@ -633,7 +632,7 @@ fn compare_diagnostic_ranges( }) } -// TODO kb wrong? What to do here instead? +// TODO wrong? What to do here instead? fn compare_diagnostic_range_edges( old: &Range, new: &Range, @@ -1316,59 +1315,67 @@ fn render_same_line_diagnostics( .map(|diagnostic| diagnostic_text_lines(diagnostic)) .sum::(); let editor_handle = editor_handle.clone(); - let mut parent = v_flex(); - let mut diagnostics_iter = diagnostics.iter().fuse(); - if let Some(first_diagnostic) = diagnostics_iter.next() { - let mut renderer = diagnostic_block_renderer( - first_diagnostic.clone(), - Some(folded_block_height), - false, - true, - ); - parent = parent.child( - h_flex() - .when_some(toggle_expand_label, |parent, label| { - parent.child(Button::new(cx.transform_block_id, label).on_click({ - let diagnostics = Arc::clone(&diagnostics); - move |_, cx| { - let new_expanded = !expanded; - button_expanded.store(new_expanded, atomic::Ordering::Release); - let new_size = if new_expanded { - expanded_block_height - } else { - folded_block_height - }; - editor_handle.update(cx, |editor, cx| { - editor.replace_blocks( - HashMap::from_iter(Some(( - block_id, - ( - Some(new_size), - render_same_line_diagnostics( - Arc::clone(&button_expanded), - Arc::clone(&diagnostics), - editor_handle.clone(), - folded_block_height, - ), + let parent = h_flex() + .items_start() + .child(v_flex().size_full().when_some_else( + toggle_expand_label, + |parent, label| { + parent.child(Button::new(cx.transform_block_id, label).on_click({ + let diagnostics = Arc::clone(&diagnostics); + move |_, cx| { + let new_expanded = !expanded; + button_expanded.store(new_expanded, atomic::Ordering::Release); + let new_size = if new_expanded { + expanded_block_height + } else { + folded_block_height + }; + editor_handle.update(cx, |editor, cx| { + editor.replace_blocks( + HashMap::from_iter(Some(( + block_id, + ( + Some(new_size), + render_same_line_diagnostics( + Arc::clone(&button_expanded), + Arc::clone(&diagnostics), + editor_handle.clone(), + folded_block_height, ), - ))), - None, - cx, - ) - }); - } - })) - }) - .child(renderer(cx)), - ); - } + ), + ))), + None, + cx, + ) + }); + } + })) + }, + |parent| { + parent.child( + h_flex() + .size(IconSize::default().rems()) + .invisible() + .flex_none(), + ) + }, + )); + let max_message_rows = if expanded { + None + } else { + Some(folded_block_height) + }; + let mut renderer = + diagnostic_block_renderer(first_diagnostic.clone(), max_message_rows, false, true); + let mut diagnostics_element = v_flex(); + diagnostics_element = diagnostics_element.child(renderer(cx)); if expanded { - for diagnostic in diagnostics_iter { + for diagnostic in diagnostics.iter().skip(1) { let mut renderer = diagnostic_block_renderer(diagnostic.clone(), None, false, true); - parent = parent.child(renderer(cx)); + diagnostics_element = diagnostics_element.child(renderer(cx)); } } - parent.into_any_element() + parent.child(diagnostics_element).into_any_element() }) } diff --git a/crates/diagnostics/src/toolbar_controls.rs b/crates/diagnostics/src/toolbar_controls.rs index 7f4deba73e..909eb77c22 100644 --- a/crates/diagnostics/src/toolbar_controls.rs +++ b/crates/diagnostics/src/toolbar_controls.rs @@ -1,11 +1,12 @@ -use crate::ProjectDiagnosticsEditor; -use gpui::{EventEmitter, ParentElement, Render, ViewContext, WeakView}; +use crate::{grouped_diagnostics::GroupedDiagnosticsEditor, ProjectDiagnosticsEditor}; +use futures::future::Either; +use gpui::{EventEmitter, ParentElement, Render, View, ViewContext, WeakView}; use ui::prelude::*; use ui::{IconButton, IconName, Tooltip}; use workspace::{item::ItemHandle, ToolbarItemEvent, ToolbarItemLocation, ToolbarItemView}; pub struct ToolbarControls { - editor: Option>, + editor: Option, WeakView>>, } impl Render for ToolbarControls { @@ -14,18 +15,33 @@ impl Render for ToolbarControls { let mut has_stale_excerpts = false; let mut is_updating = false; - if let Some(editor) = self.editor.as_ref().and_then(|editor| editor.upgrade()) { - let editor = editor.read(cx); - - include_warnings = editor.include_warnings; - has_stale_excerpts = !editor.paths_to_update.is_empty(); - is_updating = editor.update_paths_tx.len() > 0 - || editor - .project - .read(cx) - .language_servers_running_disk_based_diagnostics() - .next() - .is_some(); + if let Some(editor) = self.editor() { + match editor { + Either::Left(editor) => { + let editor = editor.read(cx); + include_warnings = editor.include_warnings; + has_stale_excerpts = !editor.paths_to_update.is_empty(); + is_updating = editor.update_paths_tx.len() > 0 + || editor + .project + .read(cx) + .language_servers_running_disk_based_diagnostics() + .next() + .is_some(); + } + Either::Right(editor) => { + let editor = editor.read(cx); + include_warnings = editor.include_warnings; + has_stale_excerpts = !editor.paths_to_update.is_empty(); + is_updating = editor.update_paths_tx.len() > 0 + || editor + .project + .read(cx) + .language_servers_running_disk_based_diagnostics() + .next() + .is_some(); + } + } } let tooltip = if include_warnings { @@ -42,12 +58,19 @@ impl Render for ToolbarControls { .disabled(is_updating) .tooltip(move |cx| Tooltip::text("Update excerpts", cx)) .on_click(cx.listener(|this, _, cx| { - if let Some(editor) = - this.editor.as_ref().and_then(|editor| editor.upgrade()) - { - editor.update(cx, |editor, _| { - editor.enqueue_update_stale_excerpts(None); - }); + if let Some(editor) = this.editor() { + match editor { + Either::Left(editor) => { + editor.update(cx, |editor, _| { + editor.enqueue_update_stale_excerpts(None); + }); + } + Either::Right(editor) => { + editor.update(cx, |editor, _| { + editor.enqueue_update_stale_excerpts(None); + }); + } + } } })), ) @@ -56,12 +79,19 @@ impl Render for ToolbarControls { IconButton::new("toggle-warnings", IconName::ExclamationTriangle) .tooltip(move |cx| Tooltip::text(tooltip, cx)) .on_click(cx.listener(|this, _, cx| { - if let Some(editor) = - this.editor.as_ref().and_then(|editor| editor.upgrade()) - { - editor.update(cx, |editor, cx| { - editor.toggle_warnings(&Default::default(), cx); - }); + if let Some(editor) = this.editor() { + match editor { + Either::Left(editor) => { + editor.update(cx, |editor, cx| { + editor.toggle_warnings(&Default::default(), cx); + }); + } + Either::Right(editor) => { + editor.update(cx, |editor, cx| { + editor.toggle_warnings(&Default::default(), cx); + }); + } + } } })), ) @@ -78,7 +108,10 @@ impl ToolbarItemView for ToolbarControls { ) -> ToolbarItemLocation { if let Some(pane_item) = active_pane_item.as_ref() { if let Some(editor) = pane_item.downcast::() { - self.editor = Some(editor.downgrade()); + self.editor = Some(Either::Left(editor.downgrade())); + ToolbarItemLocation::PrimaryRight + } else if let Some(editor) = pane_item.downcast::() { + self.editor = Some(Either::Right(editor.downgrade())); ToolbarItemLocation::PrimaryRight } else { ToolbarItemLocation::Hidden @@ -93,4 +126,13 @@ impl ToolbarControls { pub fn new() -> Self { ToolbarControls { editor: None } } + + fn editor( + &self, + ) -> Option, View>> { + Some(match self.editor.as_ref()? { + Either::Left(diagnostics) => Either::Left(diagnostics.upgrade()?), + Either::Right(grouped_diagnostics) => Either::Right(grouped_diagnostics.upgrade()?), + }) + } } diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 67cd0e3e6b..42f9ca5782 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -12827,23 +12827,31 @@ pub fn highlight_diagnostic_message( let mut prev_offset = 0; let mut in_code_block = false; + let has_row_limit = max_message_rows.is_some(); let mut newline_indices = diagnostic .message .match_indices('\n') + .filter(|_| has_row_limit) .map(|(ix, _)| ix) .fuse() .peekable(); - for (ix, _) in diagnostic + + for (quote_ix, _) in diagnostic .message .match_indices('`') .chain([(diagnostic.message.len(), "")]) { - let mut trimmed_ix = ix; - while let Some(newline_index) = newline_indices.peek() { - if *newline_index < ix { + let mut first_newline_ix = None; + let mut last_newline_ix = None; + while let Some(newline_ix) = newline_indices.peek() { + if *newline_ix < quote_ix { + if first_newline_ix.is_none() { + first_newline_ix = Some(*newline_ix); + } + last_newline_ix = Some(*newline_ix); + if let Some(rows_left) = &mut max_message_rows { if *rows_left == 0 { - trimmed_ix = newline_index.saturating_sub(1); break; } else { *rows_left -= 1; @@ -12855,14 +12863,14 @@ pub fn highlight_diagnostic_message( } } let prev_len = text_without_backticks.len(); - let new_text = &diagnostic.message[prev_offset..trimmed_ix]; + let new_text = &diagnostic.message[prev_offset..first_newline_ix.unwrap_or(quote_ix)]; text_without_backticks.push_str(new_text); if in_code_block { code_ranges.push(prev_len..text_without_backticks.len()); } - prev_offset = trimmed_ix + 1; + prev_offset = last_newline_ix.unwrap_or(quote_ix) + 1; in_code_block = !in_code_block; - if trimmed_ix != ix { + if first_newline_ix.map_or(false, |newline_ix| newline_ix < quote_ix) { text_without_backticks.push_str("..."); break; }