zeta2: Try not to jump to collaborators as much (#49742)

Ben Kunkle created

Closes #ISSUE

Release Notes:

- N/A *or* Added/Fixed/Improved ...

Change summary

crates/edit_prediction/src/edit_prediction.rs       |  75 +++-
crates/edit_prediction/src/edit_prediction_tests.rs | 218 ++++++++++++++
2 files changed, 270 insertions(+), 23 deletions(-)

Detailed changes

crates/edit_prediction/src/edit_prediction.rs 🔗

@@ -1985,7 +1985,7 @@ impl EditPredictionStore {
         })
     }
 
-    async fn next_diagnostic_location(
+    pub(crate) async fn next_diagnostic_location(
         active_buffer: Entity<Buffer>,
         active_buffer_snapshot: &BufferSnapshot,
         active_buffer_diagnostic_search_range: Range<Point>,
@@ -1993,7 +1993,13 @@ impl EditPredictionStore {
         project: &Entity<Project>,
         cx: &mut AsyncApp,
     ) -> Result<Option<(Entity<Buffer>, language::Anchor)>> {
-        // find the closest diagnostic to the cursor that wasn't close enough to be included in the last request
+        let collaborator_cursor_rows: Vec<u32> = active_buffer_snapshot
+            .selections_in_range(Anchor::MIN..Anchor::MAX, false)
+            .flat_map(|(_, _, _, selections)| {
+                selections.map(|s| s.head().to_point(active_buffer_snapshot).row)
+            })
+            .collect();
+
         let mut jump_location = active_buffer_snapshot
             .diagnostic_groups(None)
             .into_iter()
@@ -2002,10 +2008,17 @@ impl EditPredictionStore {
                     .range
                     .to_point(&active_buffer_snapshot);
                 if range.overlaps(&active_buffer_diagnostic_search_range) {
-                    None
-                } else {
-                    Some(range.start)
+                    return None;
                 }
+                let near_collaborator = collaborator_cursor_rows.iter().any(|&collab_row| {
+                    range.start.row.abs_diff(collab_row) <= DIAGNOSTIC_LINES_RANGE
+                });
+                let near_local = active_buffer_cursor_point.row.abs_diff(range.start.row)
+                    <= DIAGNOSTIC_LINES_RANGE;
+                if near_collaborator && !near_local {
+                    return None;
+                }
+                Some(range.start)
             })
             .min_by_key(|probe| probe.row.abs_diff(active_buffer_cursor_point.row))
             .map(|position| {
@@ -2025,13 +2038,13 @@ impl EditPredictionStore {
                 })
             });
 
-            let buffer_task = project.update(cx, |project, cx| {
-                let (path, _, _) = project
+            let mut candidates: Vec<(ProjectPath, usize)> = project.read_with(cx, |project, cx| {
+                project
                     .diagnostic_summaries(false, cx)
                     .filter(|(path, _, _)| Some(path) != active_buffer_path.as_ref())
-                    .max_by_key(|(path, _, _)| {
-                        // find the buffer with errors that shares most parent directories
-                        path.path
+                    .map(|(path, _, _)| {
+                        let shared_prefix = path
+                            .path
                             .components()
                             .zip(
                                 active_buffer_path
@@ -2040,24 +2053,42 @@ impl EditPredictionStore {
                                     .unwrap_or_default(),
                             )
                             .take_while(|(a, b)| a == b)
-                            .count()
-                    })?;
-
-                Some(project.open_buffer(path, cx))
+                            .count();
+                        (path, shared_prefix)
+                    })
+                    .collect()
             });
 
-            if let Some(buffer_task) = buffer_task {
-                let closest_buffer = buffer_task.await?;
+            candidates.sort_by(|a, b| b.1.cmp(&a.1));
+
+            for (path, _) in candidates {
+                let candidate_buffer = project
+                    .update(cx, |project, cx| project.open_buffer(path, cx))
+                    .await?;
 
-                jump_location = closest_buffer
-                    .read_with(cx, |buffer, _cx| {
-                        buffer
+                let (has_collaborators, diagnostic_position) =
+                    candidate_buffer.read_with(cx, |buffer, _cx| {
+                        let snapshot = buffer.snapshot();
+                        let has_collaborators = snapshot
+                            .selections_in_range(Anchor::MIN..Anchor::MAX, false)
+                            .next()
+                            .is_some();
+                        let position = buffer
                             .buffer_diagnostics(None)
                             .into_iter()
                             .min_by_key(|entry| entry.diagnostic.severity)
-                            .map(|entry| entry.range.start)
-                    })
-                    .map(|position| (closest_buffer, position));
+                            .map(|entry| entry.range.start);
+                        (has_collaborators, position)
+                    });
+
+                if has_collaborators {
+                    continue;
+                }
+
+                if let Some(position) = diagnostic_position {
+                    jump_location = Some((candidate_buffer, position));
+                    break;
+                }
             }
         }
 

crates/edit_prediction/src/edit_prediction_tests.rs 🔗

@@ -17,7 +17,7 @@ use gpui::{
     http_client::{FakeHttpClient, Response},
 };
 use indoc::indoc;
-use language::{Buffer, Point};
+use language::{Anchor, Buffer, CursorShape, Operation, Point, Selection, SelectionGoal};
 use lsp::LanguageServerId;
 use parking_lot::Mutex;
 use pretty_assertions::{assert_eq, assert_matches};
@@ -2320,6 +2320,222 @@ fn test_compute_diff_between_snapshots(cx: &mut TestAppContext) {
     );
 }
 
+#[gpui::test]
+async fn test_diagnostic_jump_excludes_collaborator_regions(cx: &mut TestAppContext) {
+    fn set_collaborator_cursor(buffer: &Entity<Buffer>, row: u32, cx: &mut TestAppContext) {
+        let collab_replica = clock::ReplicaId::new(10);
+        let anchor = buffer.read_with(cx, |buffer, _| {
+            buffer.snapshot().anchor_before(Point::new(row, 0))
+        });
+        let selections: Arc<[Selection<Anchor>]> = Arc::new([Selection {
+            id: 1,
+            start: anchor,
+            end: anchor,
+            reversed: false,
+            goal: SelectionGoal::None,
+        }]);
+        buffer.update(cx, |buffer, cx| {
+            buffer.apply_ops(
+                [Operation::UpdateSelections {
+                    selections,
+                    lamport_timestamp: clock::Lamport {
+                        replica_id: collab_replica,
+                        value: 1,
+                    },
+                    line_mode: false,
+                    cursor_shape: CursorShape::Bar,
+                }],
+                cx,
+            );
+        });
+    }
+
+    fn publish_diagnostics(
+        uri_path: &'static str,
+        rows: &[u32],
+        project: &Entity<Project>,
+        cx: &mut TestAppContext,
+    ) {
+        let diagnostics: Vec<_> = rows
+            .iter()
+            .map(|&row| lsp::Diagnostic {
+                range: lsp::Range::new(lsp::Position::new(row, 0), lsp::Position::new(row, 5)),
+                severity: Some(lsp::DiagnosticSeverity::ERROR),
+                message: format!("error at row {row}"),
+                ..Default::default()
+            })
+            .collect();
+        project.update(cx, |project, cx| {
+            project.lsp_store().update(cx, |lsp_store, cx| {
+                lsp_store
+                    .update_diagnostics(
+                        LanguageServerId(0),
+                        lsp::PublishDiagnosticsParams {
+                            uri: lsp::Uri::from_file_path(uri_path).expect("invalid uri"),
+                            diagnostics,
+                            version: None,
+                        },
+                        None,
+                        language::DiagnosticSourceKind::Pushed,
+                        &[],
+                        cx,
+                    )
+                    .expect("failed to update diagnostics");
+            });
+        });
+    }
+
+    init_test(cx);
+
+    let mut lines = String::new();
+    for i in 0..60 {
+        lines.push_str(&format!("line {i}\n"));
+    }
+
+    let fs = FakeFs::new(cx.executor());
+    fs.insert_tree(
+        "/root",
+        json!({
+            "active.txt": lines,
+            "collab_file.txt": "error here\nsecond line\n",
+            "free_file.txt": "another error\nsecond line\n",
+        }),
+    )
+    .await;
+    let project = Project::test(fs, vec![path!("/root").as_ref()], cx).await;
+
+    let active_buffer = project
+        .update(cx, |project, cx| {
+            let path = project
+                .find_project_path(path!("/root/active.txt"), cx)
+                .expect("active.txt not found");
+            project.set_active_path(Some(path.clone()), cx);
+            project.open_buffer(path, cx)
+        })
+        .await
+        .expect("failed to open active buffer");
+
+    set_collaborator_cursor(&active_buffer, 5, cx);
+
+    publish_diagnostics(path!("/root/active.txt"), &[3, 25, 50], &project, cx);
+
+    cx.run_until_parked();
+
+    let cursor_point = Point::new(25, 0);
+    let empty_search_range: Range<Point> = Default::default();
+
+    let snapshot = active_buffer.read_with(cx, |buffer, _| buffer.snapshot());
+    let result = EditPredictionStore::next_diagnostic_location(
+        active_buffer.clone(),
+        &snapshot,
+        empty_search_range.clone(),
+        cursor_point,
+        &project,
+        &mut cx.to_async(),
+    )
+    .await
+    .expect("next_diagnostic_location failed");
+
+    let (result_buffer, result_anchor) = result.expect("expected a diagnostic location");
+    assert_eq!(result_buffer.entity_id(), active_buffer.entity_id());
+    let result_row = result_buffer.read_with(cx, |buffer, _| {
+        result_anchor.to_point(&buffer.snapshot()).row
+    });
+    assert_ne!(
+        result_row, 3,
+        "row 3 is near collaborator (row 5) but far from local cursor (row 25), should be excluded"
+    );
+    assert!(
+        result_row == 25 || result_row == 50,
+        "expected row 25 or 50, got {result_row}"
+    );
+
+    let snapshot_near = active_buffer.read_with(cx, |buffer, _| buffer.snapshot());
+    let near_cursor_point = Point::new(4, 0);
+    let result_near = EditPredictionStore::next_diagnostic_location(
+        active_buffer.clone(),
+        &snapshot_near,
+        empty_search_range.clone(),
+        near_cursor_point,
+        &project,
+        &mut cx.to_async(),
+    )
+    .await
+    .expect("next_diagnostic_location failed");
+
+    let (_, near_anchor) = result_near.expect("expected a diagnostic location when both are near");
+    let near_row =
+        active_buffer.read_with(cx, |buffer, _| near_anchor.to_point(&buffer.snapshot()).row);
+    assert_eq!(
+        near_row, 3,
+        "row 3 should be included when local cursor (row 4) is also near the collaborator"
+    );
+
+    let snapshot_far = active_buffer.read_with(cx, |buffer, _| buffer.snapshot());
+    let far_cursor_point = Point::new(50, 0);
+    let result_far = EditPredictionStore::next_diagnostic_location(
+        active_buffer.clone(),
+        &snapshot_far,
+        empty_search_range.clone(),
+        far_cursor_point,
+        &project,
+        &mut cx.to_async(),
+    )
+    .await
+    .expect("next_diagnostic_location failed");
+
+    let (_, far_anchor) = result_far.expect("expected a diagnostic location");
+    let far_row =
+        active_buffer.read_with(cx, |buffer, _| far_anchor.to_point(&buffer.snapshot()).row);
+    assert_eq!(
+        far_row, 50,
+        "row 50 is near local cursor (row 50) and far from collaborator, should be picked"
+    );
+
+    publish_diagnostics(path!("/root/collab_file.txt"), &[0], &project, cx);
+    publish_diagnostics(path!("/root/free_file.txt"), &[0], &project, cx);
+    cx.run_until_parked();
+
+    let collab_buffer = project
+        .update(cx, |project, cx| {
+            let path = project
+                .find_project_path(path!("/root/collab_file.txt"), cx)
+                .expect("collab_file.txt not found");
+            project.open_buffer(path, cx)
+        })
+        .await
+        .expect("failed to open collab buffer");
+
+    set_collaborator_cursor(&collab_buffer, 0, cx);
+    cx.run_until_parked();
+
+    let no_same_file_search_range = Point::new(0, 0)..Point::new(59, 0);
+    let snapshot_cross = active_buffer.read_with(cx, |buffer, _| buffer.snapshot());
+    let result_cross = EditPredictionStore::next_diagnostic_location(
+        active_buffer.clone(),
+        &snapshot_cross,
+        no_same_file_search_range,
+        Point::new(0, 0),
+        &project,
+        &mut cx.to_async(),
+    )
+    .await
+    .expect("cross-file next_diagnostic_location failed");
+
+    let (cross_buffer, _) = result_cross.expect("expected a cross-file diagnostic location");
+    let cross_path = cross_buffer.read_with(cx, |buffer, cx| {
+        buffer
+            .file()
+            .expect("buffer should have a file")
+            .full_path(cx)
+    });
+    assert_eq!(
+        cross_path,
+        Path::new(path!("root/free_file.txt")),
+        "should skip collab_file.txt (has collaborator) and pick free_file.txt"
+    );
+}
+
 #[ctor::ctor]
 fn init_logger() {
     zlog::init_test();