From 6386336eee1ada9d86f790ed386298e96769a07e Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 29 Apr 2025 11:15:45 -0400 Subject: [PATCH] debugger: Fix bug where active debug line highlights weren't cleared (#29562) ## Context The bug occurred because we stopped propagating the `BreakpointStoreEvent::SetDebugLine` whenever a new debug line highlight had been set. This was done to prevent multiple panes from having editors focus on the debug line. However, it stopped the event from propagating to editors that needed to clear their debug line highlights. I fixed this by introducing two phases 1. Clear all debug line highlights 2. Set active debug line highlight in singular editor I also added a test to prevent regressions from occurring Release Notes: - N/A --- .../debugger_ui/src/tests/debugger_panel.rs | 260 +++++++++++++++++- .../debugger_ui/src/tests/stack_frame_list.rs | 4 +- crates/editor/src/editor.rs | 16 +- .../project/src/debugger/breakpoint_store.rs | 22 +- crates/project/src/debugger/session.rs | 3 +- crates/workspace/src/workspace.rs | 2 +- 6 files changed, 292 insertions(+), 15 deletions(-) diff --git a/crates/debugger_ui/src/tests/debugger_panel.rs b/crates/debugger_ui/src/tests/debugger_panel.rs index 18400991a7e57669729c390e1c63d86fc1d34c4e..0265abb0354874b086c25f5e2bc16055af4e39eb 100644 --- a/crates/debugger_ui/src/tests/debugger_panel.rs +++ b/crates/debugger_ui/src/tests/debugger_panel.rs @@ -14,7 +14,7 @@ use dap::{ }, }; use editor::{ - Editor, EditorMode, MultiBuffer, + ActiveDebugLine, Editor, EditorMode, MultiBuffer, actions::{self}, }; use gpui::{BackgroundExecutor, TestAppContext, VisualTestContext}; @@ -1460,3 +1460,261 @@ async fn test_we_send_arguments_from_user_config( "Launch request handler was not called" ); } + +#[gpui::test] +async fn test_active_debug_line_setting(executor: BackgroundExecutor, cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(executor.clone()); + + fs.insert_tree( + path!("/project"), + json!({ + "main.rs": "First line\nSecond line\nThird line\nFourth line", + "second.rs": "First line\nSecond line\nThird line\nFourth line", + }), + ) + .await; + + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + let workspace = init_test_workspace(&project, cx).await; + let cx = &mut VisualTestContext::from_window(*workspace, cx); + let project_path = Path::new(path!("/project")); + let worktree = project + .update(cx, |project, cx| project.find_worktree(project_path, cx)) + .expect("This worktree should exist in project") + .0; + + let worktree_id = workspace + .update(cx, |_, _, cx| worktree.read(cx).id()) + .unwrap(); + + let main_buffer = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "main.rs"), cx) + }) + .await + .unwrap(); + + let second_buffer = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "second.rs"), cx) + }) + .await + .unwrap(); + + let (main_editor, cx) = cx.add_window_view(|window, cx| { + Editor::new( + EditorMode::full(), + MultiBuffer::build_from_buffer(main_buffer, cx), + Some(project.clone()), + window, + cx, + ) + }); + + let (second_editor, cx) = cx.add_window_view(|window, cx| { + Editor::new( + EditorMode::full(), + MultiBuffer::build_from_buffer(second_buffer, cx), + Some(project.clone()), + window, + cx, + ) + }); + + let session = start_debug_session(&workspace, cx, |_| {}).unwrap(); + let client = session.update(cx, |session, _| session.adapter_client().unwrap()); + + client.on_request::(move |_, _| { + Ok(dap::ThreadsResponse { + threads: vec![dap::Thread { + id: 1, + name: "Thread 1".into(), + }], + }) + }); + + client.on_request::(move |_, _| { + Ok(dap::ScopesResponse { + scopes: Vec::default(), + }) + }); + + client.on_request::(move |_, args| { + assert_eq!(args.thread_id, 1); + + Ok(dap::StackTraceResponse { + stack_frames: vec![dap::StackFrame { + id: 1, + name: "frame 1".into(), + source: Some(dap::Source { + name: Some("main.rs".into()), + path: Some(path!("/project/main.rs").into()), + source_reference: None, + presentation_hint: None, + origin: None, + sources: None, + adapter_data: None, + checksums: None, + }), + line: 2, + column: 0, + end_line: None, + end_column: None, + can_restart: None, + instruction_pointer_reference: None, + module_id: None, + presentation_hint: None, + }], + total_frames: None, + }) + }); + + client + .fake_event(dap::messages::Events::Stopped(dap::StoppedEvent { + reason: dap::StoppedEventReason::Breakpoint, + description: None, + thread_id: Some(1), + preserve_focus_hint: None, + text: None, + all_threads_stopped: None, + hit_breakpoint_ids: None, + })) + .await; + + cx.run_until_parked(); + + main_editor.update_in(cx, |editor, window, cx| { + let active_debug_lines: Vec<_> = editor.highlighted_rows::().collect(); + + assert_eq!( + active_debug_lines.len(), + 1, + "There should be only one active debug line" + ); + + let point = editor + .snapshot(window, cx) + .buffer_snapshot + .summary_for_anchor::(&active_debug_lines.first().unwrap().0.start); + + assert_eq!(point.row, 1); + }); + + second_editor.update(cx, |editor, _| { + let active_debug_lines: Vec<_> = editor.highlighted_rows::().collect(); + + assert!( + active_debug_lines.is_empty(), + "There shouldn't be any active debug lines" + ); + }); + + let handled_second_stacktrace = Arc::new(AtomicBool::new(false)); + client.on_request::({ + let handled_second_stacktrace = handled_second_stacktrace.clone(); + move |_, args| { + handled_second_stacktrace.store(true, Ordering::SeqCst); + assert_eq!(args.thread_id, 1); + + Ok(dap::StackTraceResponse { + stack_frames: vec![dap::StackFrame { + id: 2, + name: "frame 2".into(), + source: Some(dap::Source { + name: Some("second.rs".into()), + path: Some(path!("/project/second.rs").into()), + source_reference: None, + presentation_hint: None, + origin: None, + sources: None, + adapter_data: None, + checksums: None, + }), + line: 3, + column: 0, + end_line: None, + end_column: None, + can_restart: None, + instruction_pointer_reference: None, + module_id: None, + presentation_hint: None, + }], + total_frames: None, + }) + } + }); + + client + .fake_event(dap::messages::Events::Stopped(dap::StoppedEvent { + reason: dap::StoppedEventReason::Breakpoint, + description: None, + thread_id: Some(1), + preserve_focus_hint: None, + text: None, + all_threads_stopped: None, + hit_breakpoint_ids: None, + })) + .await; + + cx.run_until_parked(); + + second_editor.update_in(cx, |editor, window, cx| { + let active_debug_lines: Vec<_> = editor.highlighted_rows::().collect(); + + assert_eq!( + active_debug_lines.len(), + 1, + "There should be only one active debug line" + ); + + let point = editor + .snapshot(window, cx) + .buffer_snapshot + .summary_for_anchor::(&active_debug_lines.first().unwrap().0.start); + + assert_eq!(point.row, 2); + }); + + main_editor.update(cx, |editor, _| { + let active_debug_lines: Vec<_> = editor.highlighted_rows::().collect(); + + assert!( + active_debug_lines.is_empty(), + "There shouldn't be any active debug lines" + ); + }); + + assert!( + handled_second_stacktrace.load(Ordering::SeqCst), + "Second stacktrace request handler was not called" + ); + + // Clean up + let shutdown_session = project.update(cx, |project, cx| { + project.dap_store().update(cx, |dap_store, cx| { + dap_store.shutdown_session(session.read(cx).session_id(), cx) + }) + }); + + shutdown_session.await.unwrap(); + + main_editor.update(cx, |editor, _| { + let active_debug_lines: Vec<_> = editor.highlighted_rows::().collect(); + + assert!( + active_debug_lines.is_empty(), + "There shouldn't be any active debug lines after session shutdown" + ); + }); + + second_editor.update(cx, |editor, _| { + let active_debug_lines: Vec<_> = editor.highlighted_rows::().collect(); + + assert!( + active_debug_lines.is_empty(), + "There shouldn't be any active debug lines after session shutdown" + ); + }); +} diff --git a/crates/debugger_ui/src/tests/stack_frame_list.rs b/crates/debugger_ui/src/tests/stack_frame_list.rs index 02b2e3ac63585554efedb4149a0cfed77e2bf082..e220df7976ea0c12536ee41376df481f9c5f3547 100644 --- a/crates/debugger_ui/src/tests/stack_frame_list.rs +++ b/crates/debugger_ui/src/tests/stack_frame_list.rs @@ -362,7 +362,7 @@ async fn test_select_stack_frame(executor: BackgroundExecutor, cx: &mut TestAppC let snapshot = editor.snapshot(window, cx); editor - .highlighted_rows::() + .highlighted_rows::() .map(|(range, _)| { let start = range.start.to_point(&snapshot.buffer_snapshot); let end = range.end.to_point(&snapshot.buffer_snapshot); @@ -432,7 +432,7 @@ async fn test_select_stack_frame(executor: BackgroundExecutor, cx: &mut TestAppC let snapshot = editor.snapshot(window, cx); editor - .highlighted_rows::() + .highlighted_rows::() .map(|(range, _)| { let start = range.start.to_point(&snapshot.buffer_snapshot); let end = range.end.to_point(&snapshot.buffer_snapshot); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6b253c1b747e1f68e281b2e677749eb732632661..9ef27f10fd3cc6ea91221ae52e7e9e3046000b60 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -286,7 +286,7 @@ impl InlayId { } } -pub enum DebugCurrentRowHighlight {} +pub enum ActiveDebugLine {} enum DocumentHighlightRead {} enum DocumentHighlightWrite {} enum InputComposition {} @@ -1581,7 +1581,11 @@ impl Editor { &project.read(cx).breakpoint_store(), window, |editor, _, event, window, cx| match event { - BreakpointStoreEvent::ActiveDebugLineChanged => { + BreakpointStoreEvent::ClearDebugLines => { + editor.clear_row_highlights::(); + editor.refresh_inline_values(cx); + } + BreakpointStoreEvent::SetDebugLine => { if editor.go_to_active_debug_line(window, cx) { cx.stop_propagation(); } @@ -16635,7 +16639,7 @@ impl Editor { let Some(active_stack_frame) = breakpoint_store.read(cx).active_position().cloned() else { - self.clear_row_highlights::(); + self.clear_row_highlights::(); return None; }; @@ -16662,8 +16666,8 @@ impl Editor { let multibuffer_anchor = snapshot.anchor_in_excerpt(id, position)?; handled = true; - self.clear_row_highlights::(); - self.go_to_line::( + self.clear_row_highlights::(); + self.go_to_line::( multibuffer_anchor, Some(cx.theme().colors().editor_debugger_active_line_background), window, @@ -17688,7 +17692,7 @@ impl Editor { let current_execution_position = self .highlighted_rows - .get(&TypeId::of::()) + .get(&TypeId::of::()) .and_then(|lines| lines.last().map(|line| line.range.start)); self.inline_value_cache.refresh_task = cx.spawn(async move |editor, cx| { diff --git a/crates/project/src/debugger/breakpoint_store.rs b/crates/project/src/debugger/breakpoint_store.rs index c47b3ea8fae30776d73f7a1ad603f5a1de22f512..cf1f8271f0ad96df974330f64dbff92e55599c24 100644 --- a/crates/project/src/debugger/breakpoint_store.rs +++ b/crates/project/src/debugger/breakpoint_store.rs @@ -111,7 +111,7 @@ enum BreakpointStoreMode { Remote(RemoteBreakpointStore), } -#[derive(Clone)] +#[derive(Clone, PartialEq)] pub struct ActiveStackFrame { pub session_id: SessionId, pub thread_id: ThreadId, @@ -521,13 +521,26 @@ impl BreakpointStore { self.active_stack_frame.take(); } - cx.emit(BreakpointStoreEvent::ActiveDebugLineChanged); + cx.emit(BreakpointStoreEvent::ClearDebugLines); cx.notify(); } pub fn set_active_position(&mut self, position: ActiveStackFrame, cx: &mut Context) { + if self + .active_stack_frame + .as_ref() + .is_some_and(|active_position| active_position == &position) + { + return; + } + + if self.active_stack_frame.is_some() { + cx.emit(BreakpointStoreEvent::ClearDebugLines); + } + self.active_stack_frame = Some(position); - cx.emit(BreakpointStoreEvent::ActiveDebugLineChanged); + + cx.emit(BreakpointStoreEvent::SetDebugLine); cx.notify(); } @@ -693,7 +706,8 @@ pub enum BreakpointUpdatedReason { } pub enum BreakpointStoreEvent { - ActiveDebugLineChanged, + SetDebugLine, + ClearDebugLines, BreakpointsUpdated(Arc, BreakpointUpdatedReason), BreakpointsCleared(Vec>), } diff --git a/crates/project/src/debugger/session.rs b/crates/project/src/debugger/session.rs index 3e457a8a6d66ae4e8ede93549c847b94aa5459e7..d831abc10ed24564e7342498b15d3ac818ae0241 100644 --- a/crates/project/src/debugger/session.rs +++ b/crates/project/src/debugger/session.rs @@ -652,7 +652,7 @@ impl Session { local.unset_breakpoints_from_paths(paths, cx).detach(); } } - BreakpointStoreEvent::ActiveDebugLineChanged => {} + BreakpointStoreEvent::SetDebugLine | BreakpointStoreEvent::ClearDebugLines => {} }) .detach(); cx.on_app_quit(Self::on_app_quit).detach(); @@ -1809,6 +1809,7 @@ impl Session { .insert(variables_reference, variables.clone()); cx.emit(SessionEvent::Variables); + cx.emit(SessionEvent::InvalidateInlineValue); Some(variables) }, cx, diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 50eeb366986a9397c98a40bcff98cf672c23f1af..3003c0535ed15467a35c579425e2c1b63c33f6d1 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -998,7 +998,7 @@ impl Workspace { | BreakpointStoreEvent::BreakpointsCleared(_) => { workspace.serialize_workspace(window, cx); } - BreakpointStoreEvent::ActiveDebugLineChanged => {} + BreakpointStoreEvent::SetDebugLine | BreakpointStoreEvent::ClearDebugLines => {} }, ) .detach();