From 8075c2458fb93005c9cd9325bc6c1eb5df33c9dd Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 1 Apr 2025 01:40:05 -0400 Subject: [PATCH] Debugger: Fix breakpoint serialization (#27825) This PR fixes two bugs that cause unexpected behavior with breakpoints. The first bug made it impossible to delete the last breakpoint in a file in the workspace's database. This caused deleted breakpoints to remain in the database and added to new projects. The second bug was an edge case in the breakpoint context menu where disabling/enabling a breakpoint would sometimes set a new breakpoint on top of the old breakpoint. Release Notes: - N/A --- crates/editor/src/editor.rs | 27 +++--- .../project/src/debugger/breakpoint_store.rs | 13 +++ crates/workspace/src/persistence.rs | 96 ++++++++++++++++++- crates/workspace/src/workspace.rs | 6 +- 4 files changed, 124 insertions(+), 18 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 8ea913f9a1..24432c2131 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -6249,7 +6249,7 @@ impl Editor { /// It's also used to set the color of line numbers with breakpoints to the breakpoint color. /// TODO debugger: Use this function to color toggle symbols that house nested breakpoints fn active_breakpoints( - &mut self, + &self, range: Range, window: &mut Window, cx: &mut Context, @@ -6320,24 +6320,26 @@ impl Editor { let breakpoint = self .breakpoint_at_row(row, window, cx) - .map(|(_, bp)| Arc::from(bp)); + .map(|(anchor, bp)| (anchor, Arc::from(bp))); - let log_breakpoint_msg = if breakpoint.as_ref().is_some_and(|bp| bp.message.is_some()) { + let log_breakpoint_msg = if breakpoint.as_ref().is_some_and(|bp| bp.1.message.is_some()) { "Edit Log Breakpoint" } else { "Set Log Breakpoint" }; - let condition_breakpoint_msg = - if breakpoint.as_ref().is_some_and(|bp| bp.condition.is_some()) { - "Edit Condition Breakpoint" - } else { - "Set Condition Breakpoint" - }; + let condition_breakpoint_msg = if breakpoint + .as_ref() + .is_some_and(|bp| bp.1.condition.is_some()) + { + "Edit Condition Breakpoint" + } else { + "Set Condition Breakpoint" + }; let hit_condition_breakpoint_msg = if breakpoint .as_ref() - .is_some_and(|bp| bp.hit_condition.is_some()) + .is_some_and(|bp| bp.1.hit_condition.is_some()) { "Edit Hit Condition Breakpoint" } else { @@ -6350,12 +6352,13 @@ impl Editor { "Set Breakpoint" }; - let toggle_state_msg = breakpoint.as_ref().map_or(None, |bp| match bp.state { + let toggle_state_msg = breakpoint.as_ref().map_or(None, |bp| match bp.1.state { BreakpointState::Enabled => Some("Disable"), BreakpointState::Disabled => Some("Enable"), }); - let breakpoint = breakpoint.unwrap_or_else(|| Arc::new(Breakpoint::new_standard())); + let (anchor, breakpoint) = + breakpoint.unwrap_or_else(|| (anchor, Arc::new(Breakpoint::new_standard()))); ui::ContextMenu::build(window, cx, |menu, _, _cx| { menu.on_blur_subscription(Subscription::new(|| {})) diff --git a/crates/project/src/debugger/breakpoint_store.rs b/crates/project/src/debugger/breakpoint_store.rs index d3f8c220a0..524eed44a9 100644 --- a/crates/project/src/debugger/breakpoint_store.rs +++ b/crates/project/src/debugger/breakpoint_store.rs @@ -566,7 +566,20 @@ impl BreakpointStore { new_breakpoints.insert(path, breakpoints_for_file); } this.update(cx, |this, cx| { + log::info!("Finish deserializing breakpoints & initializing breakpoint store"); + for (path, count) in new_breakpoints.iter().map(|(path, bp_in_file)| { + (path.to_string_lossy(), bp_in_file.breakpoints.len()) + }) { + let breakpoint_str = if count > 1 { + "breakpoints" + } else { + "breakpoint" + }; + log::info!("Deserialized {count} {breakpoint_str} at path: {path}"); + } + this.breakpoints = new_breakpoints; + cx.notify(); })?; diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index 656aae9cdc..286a744569 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -720,8 +720,8 @@ impl WorkspaceDb { } for (path, bps) in map.iter() { - log::debug!( - "Got {} breakpoints from path: {}", + log::info!( + "Got {} breakpoints from database at path: {}", bps.len(), path.to_string_lossy() ); @@ -746,9 +746,10 @@ impl WorkspaceDb { DELETE FROM pane_groups WHERE workspace_id = ?1; DELETE FROM panes WHERE workspace_id = ?1;))?(workspace.id) .context("Clearing old panes")?; + + conn.exec_bound(sql!(DELETE FROM breakpoints WHERE workspace_id = ?1))?(workspace.id).context("Clearing old breakpoints")?; + for (path, breakpoints) in workspace.breakpoints { - conn.exec_bound(sql!(DELETE FROM breakpoints WHERE workspace_id = ?1 AND path = ?2))?((workspace.id, path.as_ref())) - .context("Clearing old breakpoints")?; for bp in breakpoints { let state = BreakpointStateWrapper::from(bp.state); match conn.exec_bound(sql!( @@ -1607,6 +1608,93 @@ mod tests { assert_eq!(loaded_breakpoints[4].path, Arc::from(path)); } + #[gpui::test] + async fn test_remove_last_breakpoint() { + env_logger::try_init().ok(); + + let db = WorkspaceDb(open_test_db("test_remove_last_breakpoint").await); + let id = db.next_id().await.unwrap(); + + let singular_path = Path::new("/tmp/test_remove_last_breakpoint.rs"); + + let breakpoint_to_remove = Breakpoint { + position: 100, + message: None, + state: BreakpointState::Enabled, + condition: None, + hit_condition: None, + }; + + let workspace = SerializedWorkspace { + id, + location: SerializedWorkspaceLocation::from_local_paths(["/tmp"]), + center_group: Default::default(), + window_bounds: Default::default(), + display: Default::default(), + docks: Default::default(), + centered_layout: false, + breakpoints: { + let mut map = collections::BTreeMap::default(); + map.insert( + Arc::from(singular_path), + vec![SourceBreakpoint { + row: breakpoint_to_remove.position, + path: Arc::from(singular_path), + message: None, + state: BreakpointState::Enabled, + condition: None, + hit_condition: None, + }], + ); + map + }, + session_id: None, + window_id: None, + }; + + db.save_workspace(workspace.clone()).await; + + let loaded = db.workspace_for_roots(&["/tmp"]).unwrap(); + let loaded_breakpoints = loaded.breakpoints.get(&Arc::from(singular_path)).unwrap(); + + assert_eq!(loaded_breakpoints.len(), 1); + assert_eq!(loaded_breakpoints[0].row, breakpoint_to_remove.position); + assert_eq!(loaded_breakpoints[0].message, breakpoint_to_remove.message); + assert_eq!( + loaded_breakpoints[0].condition, + breakpoint_to_remove.condition + ); + assert_eq!( + loaded_breakpoints[0].hit_condition, + breakpoint_to_remove.hit_condition + ); + assert_eq!(loaded_breakpoints[0].state, breakpoint_to_remove.state); + assert_eq!(loaded_breakpoints[0].path, Arc::from(singular_path)); + + let workspace_without_breakpoint = SerializedWorkspace { + id, + location: SerializedWorkspaceLocation::from_local_paths(["/tmp"]), + center_group: Default::default(), + window_bounds: Default::default(), + display: Default::default(), + docks: Default::default(), + centered_layout: false, + breakpoints: collections::BTreeMap::default(), + session_id: None, + window_id: None, + }; + + db.save_workspace(workspace_without_breakpoint.clone()) + .await; + + let loaded_after_remove = db.workspace_for_roots(&["/tmp"]).unwrap(); + let empty_breakpoints = loaded_after_remove + .breakpoints + .get(&Arc::from(singular_path)); + + assert!(empty_breakpoints.is_none()); + } + #[gpui::test] async fn test_next_id_stability() { env_logger::try_init().ok(); diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 2978e365f2..8c0c33808b 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -988,10 +988,12 @@ impl Workspace { cx.subscribe_in( &project.read(cx).breakpoint_store(), window, - |workspace, _, evt, window, cx| { - if let BreakpointStoreEvent::BreakpointsUpdated(_, _) = evt { + |workspace, _, event, window, cx| match event { + BreakpointStoreEvent::BreakpointsUpdated(_, _) + | BreakpointStoreEvent::BreakpointsCleared(_) => { workspace.serialize_workspace(window, cx); } + BreakpointStoreEvent::ActiveDebugLineChanged => {} }, ) .detach();