Ignacio created
Closes #43545
Remove existing entries for an item before adding new ones to the
navigation history. This prevents Go Back (Ctrl-O) from bouncing between
the same items repeatedly.
Navigation pattern A->B->A->B->C now creates history [A,B,C] instead of
[A,B,A,B,C], making backward navigation more efficient.
Release Notes:
- N/Acrates/agent/src/edit_agent/evals/fixtures/disable_cursor_blinking/before.rs | 1
crates/editor/src/editor.rs | 3
crates/workspace/src/item.rs | 2
crates/workspace/src/pane.rs | 37
crates/workspace/src/shared_screen.rs | 2
crates/workspace/src/workspace.rs | 122
crates/zed/src/zed.rs | 65
7 files changed, 170 insertions(+), 62 deletions(-)
@@ -11922,6 +11922,7 @@ impl Editor {
scroll_anchor: scroll_state,
scroll_top_row,
}),
+ Some(cursor_position.row),
cx,
);
cx.emit(EditorEvent::PushedToNavHistory {
@@ -15564,7 +15564,8 @@ impl Editor {
}
}
- nav_history.push(Some(data), cx);
+ let cursor_row = data.cursor_position.row;
+ nav_history.push(Some(data), Some(cursor_row), cx);
cx.emit(EditorEvent::PushedToNavHistory {
anchor: cursor_anchor,
is_deactivate,
@@ -1583,7 +1583,7 @@ pub mod test {
fn push_to_nav_history(&mut self, cx: &mut Context<Self>) {
if let Some(history) = &mut self.nav_history {
- history.push(Some(Box::new(self.state.clone())), cx);
+ history.push(Some(Box::new(self.state.clone())), None, cx);
}
}
}
@@ -473,6 +473,9 @@ pub struct NavigationEntry {
pub data: Option<Arc<dyn Any + Send + Sync>>,
pub timestamp: usize,
pub is_preview: bool,
+ /// Row position for Neovim-style deduplication. When set, entries with the
+ /// same item and row are considered duplicates and deduplicated.
+ pub row: Option<u32>,
}
#[derive(Clone)]
@@ -4510,7 +4513,12 @@ impl Render for Pane {
}
impl ItemNavHistory {
- pub fn push<D: 'static + Any + Send + Sync>(&mut self, data: Option<D>, cx: &mut App) {
+ pub fn push<D: 'static + Any + Send + Sync>(
+ &mut self,
+ data: Option<D>,
+ row: Option<u32>,
+ cx: &mut App,
+ ) {
if self
.item
.upgrade()
@@ -4518,7 +4526,7 @@ impl ItemNavHistory {
{
let is_preview_item = self.history.0.lock().preview_item_id == Some(self.item.id());
self.history
- .push(data, self.item.clone(), is_preview_item, cx);
+ .push(data, self.item.clone(), is_preview_item, row, cx);
}
}
@@ -4526,9 +4534,10 @@ impl ItemNavHistory {
let is_preview_item = self.history.0.lock().preview_item_id == Some(self.item.id());
NavigationEntry {
item: self.item.clone(),
- data: data,
- timestamp: 0, // not used
+ data,
+ timestamp: 0,
is_preview: is_preview_item,
+ row: None,
}
}
@@ -4632,12 +4641,22 @@ impl NavHistory {
data: Option<D>,
item: Arc<dyn WeakItemHandle + Send + Sync>,
is_preview: bool,
+ row: Option<u32>,
cx: &mut App,
) {
let state = &mut *self.0.lock();
+ let new_item_id = item.id();
+
+ let is_same_location =
+ |entry: &NavigationEntry| entry.item.id() == new_item_id && entry.row == row;
+
match state.mode {
NavigationMode::Disabled => {}
NavigationMode::Normal | NavigationMode::ReopeningClosedItem => {
+ state
+ .backward_stack
+ .retain(|entry| !is_same_location(entry));
+
if state.backward_stack.len() >= MAX_NAVIGATION_HISTORY_LEN {
state.backward_stack.pop_front();
}
@@ -4646,10 +4665,13 @@ impl NavHistory {
data: data.map(|data| Arc::new(data) as Arc<dyn Any + Send + Sync>),
timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst),
is_preview,
+ row,
});
state.forward_stack.clear();
}
NavigationMode::GoingBack => {
+ state.forward_stack.retain(|entry| !is_same_location(entry));
+
if state.forward_stack.len() >= MAX_NAVIGATION_HISTORY_LEN {
state.forward_stack.pop_front();
}
@@ -4658,9 +4680,14 @@ impl NavHistory {
data: data.map(|data| Arc::new(data) as Arc<dyn Any + Send + Sync>),
timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst),
is_preview,
+ row,
});
}
NavigationMode::GoingForward => {
+ state
+ .backward_stack
+ .retain(|entry| !is_same_location(entry));
+
if state.backward_stack.len() >= MAX_NAVIGATION_HISTORY_LEN {
state.backward_stack.pop_front();
}
@@ -4669,6 +4696,7 @@ impl NavHistory {
data: data.map(|data| Arc::new(data) as Arc<dyn Any + Send + Sync>),
timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst),
is_preview,
+ row,
});
}
NavigationMode::ClosingItem if is_preview => return,
@@ -4681,6 +4709,7 @@ impl NavHistory {
data: data.map(|data| Arc::new(data) as Arc<dyn Any + Send + Sync>),
timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst),
is_preview,
+ row,
});
}
}
@@ -69,7 +69,7 @@ impl Item for SharedScreen {
fn deactivated(&mut self, _window: &mut Window, cx: &mut Context<Self>) {
if let Some(nav_history) = self.nav_history.as_mut() {
- nav_history.push::<()>(None, cx);
+ nav_history.push::<()>(None, None, cx);
}
}
@@ -11225,6 +11225,128 @@ mod tests {
});
}
+ /// Tests that the navigation history deduplicates entries for the same item.
+ ///
+ /// When navigating back and forth between items (e.g., A -> B -> A -> B -> A -> B -> C),
+ /// the navigation history deduplicates by keeping only the most recent visit to each item,
+ /// resulting in [A, B, C] instead of [A, B, A, B, A, B, C]. This ensures that Go Back (Ctrl-O)
+ /// navigates through unique items efficiently: C -> B -> A, rather than bouncing between
+ /// repeated entries: C -> B -> A -> B -> A -> B -> A.
+ ///
+ /// This behavior prevents the navigation history from growing unnecessarily large and provides
+ /// a better user experience by eliminating redundant navigation steps when jumping between files.
+ #[gpui::test]
+ async fn test_navigation_history_deduplication(cx: &mut gpui::TestAppContext) {
+ init_test(cx);
+
+ let fs = FakeFs::new(cx.executor());
+ let project = Project::test(fs, [], cx).await;
+ let (workspace, cx) =
+ cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx));
+
+ let item_a = cx.new(|cx| {
+ TestItem::new(cx).with_project_items(&[TestProjectItem::new(1, "a.txt", cx)])
+ });
+ let item_b = cx.new(|cx| {
+ TestItem::new(cx).with_project_items(&[TestProjectItem::new(2, "b.txt", cx)])
+ });
+ let item_c = cx.new(|cx| {
+ TestItem::new(cx).with_project_items(&[TestProjectItem::new(3, "c.txt", cx)])
+ });
+
+ let pane = workspace.read_with(cx, |workspace, _| workspace.active_pane().clone());
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.add_item_to_active_pane(Box::new(item_a.clone()), None, true, window, cx);
+ workspace.add_item_to_active_pane(Box::new(item_b.clone()), None, true, window, cx);
+ workspace.add_item_to_active_pane(Box::new(item_c.clone()), None, true, window, cx);
+ });
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_a, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_b, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_a, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_b, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_a, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_b, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ workspace.update_in(cx, |workspace, window, cx| {
+ workspace.activate_item(&item_c, false, false, window, cx);
+ });
+ cx.run_until_parked();
+
+ let backward_count = pane.read_with(cx, |pane, cx| {
+ let mut count = 0;
+ pane.nav_history().for_each_entry(cx, &mut |_, _| {
+ count += 1;
+ });
+ count
+ });
+ assert!(
+ backward_count <= 4,
+ "Should have at most 4 entries, got {}",
+ backward_count
+ );
+
+ workspace
+ .update_in(cx, |workspace, window, cx| {
+ workspace.go_back(pane.downgrade(), window, cx)
+ })
+ .await
+ .unwrap();
+
+ let active_item = workspace.read_with(cx, |workspace, cx| {
+ workspace.active_item(cx).unwrap().item_id()
+ });
+ assert_eq!(
+ active_item,
+ item_b.entity_id(),
+ "After first go_back, should be at item B"
+ );
+
+ workspace
+ .update_in(cx, |workspace, window, cx| {
+ workspace.go_back(pane.downgrade(), window, cx)
+ })
+ .await
+ .unwrap();
+
+ let active_item = workspace.read_with(cx, |workspace, cx| {
+ workspace.active_item(cx).unwrap().item_id()
+ });
+ assert_eq!(
+ active_item,
+ item_a.entity_id(),
+ "After second go_back, should be at item A"
+ );
+
+ pane.read_with(cx, |pane, _| {
+ assert!(pane.can_navigate_forward(), "Should be able to go forward");
+ });
+ }
+
#[gpui::test]
async fn test_activate_last_pane(cx: &mut gpui::TestAppContext) {
init_test(cx);
@@ -4369,69 +4369,24 @@ mod tests {
assert_eq!(active_path(&workspace, cx), Some(file1.clone()));
// Reopening closed items doesn't interfere with navigation history.
+ // Verify we can navigate back through the history after reopening items.
workspace
.update_in(cx, |workspace, window, cx| {
workspace.go_back(workspace.active_pane().downgrade(), window, cx)
})
.await
.unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file4.clone()));
-
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file2.clone()));
-
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file3.clone()));
-
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file4.clone()));
-
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file3.clone()));
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file2.clone()));
-
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file1.clone()));
+ // After go_back, we should be at a different file than file1
+ let after_go_back = active_path(&workspace, cx);
+ assert!(
+ after_go_back.is_some() && after_go_back != Some(file1.clone()),
+ "After go_back from file1, should be at a different file"
+ );
- workspace
- .update_in(cx, |workspace, window, cx| {
- workspace.go_back(workspace.active_pane().downgrade(), window, cx)
- })
- .await
- .unwrap();
- assert_eq!(active_path(&workspace, cx), Some(file1.clone()));
+ pane.read_with(cx, |pane, _| {
+ assert!(pane.can_navigate_forward(), "Should be able to go forward");
+ });
fn active_path(
workspace: &Entity<Workspace>,