Merge pull request #667 from zed-industries/fix-duplicate-nav-entries

Fix duplicate nav entries

crates/diagnostics/src/diagnostics.rs

@@ -454,9 +454,9 @@ impl workspace::Item for ProjectDiagnosticsEditor {
         None
     }
 
-    fn navigate(&mut self, data: Box<dyn Any>, cx: &mut ViewContext<Self>) {
+    fn navigate(&mut self, data: Box<dyn Any>, cx: &mut ViewContext<Self>) -> bool {
         self.editor
-            .update(cx, |editor, cx| editor.navigate(data, cx));
+            .update(cx, |editor, cx| editor.navigate(data, cx))
     }
 
     fn is_dirty(&self, cx: &AppContext) -> bool {

crates/editor/src/editor.rs

@@ -1393,8 +1393,6 @@ impl Editor {
             }
         }
 
-        self.push_to_nav_history(newest_selection.head(), Some(end.to_point(&buffer)), cx);
-
         let selection = Selection {
             id: post_inc(&mut self.next_selection_id),
             start,
@@ -5111,7 +5109,7 @@ impl Editor {
         cx.notify();
     }
 
-    fn transact(
+    pub fn transact(
         &mut self,
         cx: &mut ViewContext<Self>,
         update: impl FnOnce(&mut Self, &mut ViewContext<Self>),
@@ -6328,6 +6326,7 @@ mod tests {
                 editor.selected_display_ranges(cx),
                 &[DisplayPoint::new(3, 0)..DisplayPoint::new(3, 0)]
             );
+            assert!(nav_history.borrow_mut().pop_backward().is_none());
 
             // Move the cursor a small distance via the mouse.
             // Nothing is added to the navigation history.
@@ -6354,6 +6353,7 @@ mod tests {
                 editor.selected_display_ranges(cx),
                 &[DisplayPoint::new(5, 0)..DisplayPoint::new(5, 0)]
             );
+            assert!(nav_history.borrow_mut().pop_backward().is_none());
 
             editor
         });

crates/editor/src/items.rs

@@ -242,7 +242,7 @@ fn deserialize_selection(
 }
 
 impl Item for Editor {
-    fn navigate(&mut self, data: Box<dyn std::any::Any>, cx: &mut ViewContext<Self>) {
+    fn navigate(&mut self, data: Box<dyn std::any::Any>, cx: &mut ViewContext<Self>) -> bool {
         if let Some(data) = data.downcast_ref::<NavigationData>() {
             let buffer = self.buffer.read(cx).read(cx);
             let offset = if buffer.can_resolve(&data.anchor) {
@@ -250,11 +250,19 @@ impl Item for Editor {
             } else {
                 buffer.clip_offset(data.offset, Bias::Left)
             };
-
+            let newest_selection = self.newest_selection_with_snapshot::<usize>(&buffer);
             drop(buffer);
-            let nav_history = self.nav_history.take();
-            self.select_ranges([offset..offset], Some(Autoscroll::Fit), cx);
-            self.nav_history = nav_history;
+
+            if newest_selection.head() == offset {
+                false
+            } else {
+                let nav_history = self.nav_history.take();
+                self.select_ranges([offset..offset], Some(Autoscroll::Fit), cx);
+                self.nav_history = nav_history;
+                true
+            }
+        } else {
+            false
         }
     }
 

crates/search/src/project_search.rs

@@ -302,9 +302,9 @@ impl Item for ProjectSearchView {
         });
     }
 
-    fn navigate(&mut self, data: Box<dyn Any>, cx: &mut ViewContext<Self>) {
+    fn navigate(&mut self, data: Box<dyn Any>, cx: &mut ViewContext<Self>) -> bool {
         self.results_editor
-            .update(cx, |editor, cx| editor.navigate(data, cx));
+            .update(cx, |editor, cx| editor.navigate(data, cx))
     }
 
     fn should_update_tab_on_event(event: &ViewEvent) -> bool {

crates/workspace/src/pane.rs

@@ -212,40 +212,47 @@ impl Pane {
         workspace.activate_pane(pane.clone(), cx);
 
         let to_load = pane.update(cx, |pane, cx| {
-            // Retrieve the weak item handle from the history.
-            let entry = pane.nav_history.borrow_mut().pop(mode)?;
+            loop {
+                // Retrieve the weak item handle from the history.
+                let entry = pane.nav_history.borrow_mut().pop(mode)?;
 
-            // If the item is still present in this pane, then activate it.
-            if let Some(index) = entry
-                .item
-                .upgrade(cx)
-                .and_then(|v| pane.index_for_item(v.as_ref()))
-            {
-                if let Some(item) = pane.active_item() {
-                    pane.nav_history.borrow_mut().set_mode(mode);
-                    item.deactivated(cx);
-                    pane.nav_history
+                // If the item is still present in this pane, then activate it.
+                if let Some(index) = entry
+                    .item
+                    .upgrade(cx)
+                    .and_then(|v| pane.index_for_item(v.as_ref()))
+                {
+                    if let Some(item) = pane.active_item() {
+                        pane.nav_history.borrow_mut().set_mode(mode);
+                        item.deactivated(cx);
+                        pane.nav_history
+                            .borrow_mut()
+                            .set_mode(NavigationMode::Normal);
+                    }
+
+                    let prev_active_index = mem::replace(&mut pane.active_item_index, index);
+                    pane.focus_active_item(cx);
+                    let mut navigated = prev_active_index != pane.active_item_index;
+                    if let Some(data) = entry.data {
+                        navigated |= pane.active_item()?.navigate(data, cx);
+                    }
+
+                    if navigated {
+                        cx.notify();
+                        break None;
+                    }
+                }
+                // If the item is no longer present in this pane, then retrieve its
+                // project path in order to reopen it.
+                else {
+                    break pane
+                        .nav_history
                         .borrow_mut()
-                        .set_mode(NavigationMode::Normal);
+                        .paths_by_item
+                        .get(&entry.item.id())
+                        .cloned()
+                        .map(|project_path| (project_path, entry));
                 }
-
-                pane.active_item_index = index;
-                pane.focus_active_item(cx);
-                if let Some(data) = entry.data {
-                    pane.active_item()?.navigate(data, cx);
-                }
-                cx.notify();
-                None
-            }
-            // If the item is no longer present in this pane, then retrieve its
-            // project path in order to reopen it.
-            else {
-                pane.nav_history
-                    .borrow_mut()
-                    .paths_by_item
-                    .get(&entry.item.id())
-                    .cloned()
-                    .map(|project_path| (project_path, entry))
             }
         });
 

crates/workspace/src/workspace.rs

@@ -203,7 +203,9 @@ pub struct JoinProjectParams {
 
 pub trait Item: View {
     fn deactivated(&mut self, _: &mut ViewContext<Self>) {}
-    fn navigate(&mut self, _: Box<dyn Any>, _: &mut ViewContext<Self>) {}
+    fn navigate(&mut self, _: Box<dyn Any>, _: &mut ViewContext<Self>) -> bool {
+        false
+    }
     fn tab_content(&self, style: &theme::Tab, cx: &AppContext) -> ElementBox;
     fn project_path(&self, cx: &AppContext) -> Option<ProjectPath>;
     fn project_entry_id(&self, cx: &AppContext) -> Option<ProjectEntryId>;
@@ -362,7 +364,7 @@ pub trait ItemHandle: 'static + fmt::Debug {
         cx: &mut ViewContext<Workspace>,
     );
     fn deactivated(&self, cx: &mut MutableAppContext);
-    fn navigate(&self, data: Box<dyn Any>, cx: &mut MutableAppContext);
+    fn navigate(&self, data: Box<dyn Any>, cx: &mut MutableAppContext) -> bool;
     fn id(&self) -> usize;
     fn to_any(&self) -> AnyViewHandle;
     fn is_dirty(&self, cx: &AppContext) -> bool;
@@ -510,8 +512,8 @@ impl<T: Item> ItemHandle for ViewHandle<T> {
         self.update(cx, |this, cx| this.deactivated(cx));
     }
 
-    fn navigate(&self, data: Box<dyn Any>, cx: &mut MutableAppContext) {
-        self.update(cx, |this, cx| this.navigate(data, cx));
+    fn navigate(&self, data: Box<dyn Any>, cx: &mut MutableAppContext) -> bool {
+        self.update(cx, |this, cx| this.navigate(data, cx))
     }
 
     fn save(&self, project: ModelHandle<Project>, cx: &mut MutableAppContext) -> Task<Result<()>> {

crates/zed/src/zed.rs

@@ -893,6 +893,52 @@ mod tests {
             (file3.clone(), DisplayPoint::new(0, 0))
         );
 
+        // Modify file to remove nav history location, and ensure duplicates are skipped
+        editor1.update(cx, |editor, cx| {
+            editor.select_display_ranges(&[DisplayPoint::new(15, 0)..DisplayPoint::new(15, 0)], cx)
+        });
+
+        for _ in 0..5 {
+            editor1.update(cx, |editor, cx| {
+                editor
+                    .select_display_ranges(&[DisplayPoint::new(3, 0)..DisplayPoint::new(3, 0)], cx);
+            });
+            editor1.update(cx, |editor, cx| {
+                editor.select_display_ranges(
+                    &[DisplayPoint::new(13, 0)..DisplayPoint::new(13, 0)],
+                    cx,
+                )
+            });
+        }
+
+        editor1.update(cx, |editor, cx| {
+            editor.transact(cx, |editor, cx| {
+                editor.select_display_ranges(
+                    &[DisplayPoint::new(2, 0)..DisplayPoint::new(14, 0)],
+                    cx,
+                );
+                editor.insert("", cx);
+            })
+        });
+
+        editor1.update(cx, |editor, cx| {
+            editor.select_display_ranges(&[DisplayPoint::new(1, 0)..DisplayPoint::new(1, 0)], cx)
+        });
+        workspace
+            .update(cx, |w, cx| Pane::go_back(w, None, cx))
+            .await;
+        assert_eq!(
+            active_location(&workspace, cx),
+            (file1.clone(), DisplayPoint::new(2, 0))
+        );
+        workspace
+            .update(cx, |w, cx| Pane::go_back(w, None, cx))
+            .await;
+        assert_eq!(
+            active_location(&workspace, cx),
+            (file1.clone(), DisplayPoint::new(3, 0))
+        );
+
         fn active_location(
             workspace: &ViewHandle<Workspace>,
             cx: &mut TestAppContext,