Align view for background buffer opened with `alt-ret` (#7691)

* fix(picker): `alt-ret' changes cursor pos of current file, not new one

Closes #7673

* fix other pickers

* symbol pickers
* diagnostick pickers

This is done using the already patched `jump_to_location` method.

* fix global and jumplist pickers

* use `view` as old_id; make `align_view` method of `Action`

* test(picker): basic <alt-ret> functionality

* fix: picker integrational test

* fix nit

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

helix-term/src/commands.rs

@@ -2199,8 +2199,8 @@ fn global_search(cx: &mut Context) {
                     all_matches,
                     current_path,
                     move |cx, FileResult { path, line_num }, action| {
-                        match cx.editor.open(path, action) {
+                        let doc = match cx.editor.open(path, action) {
-                            Ok(_) => {}
+                            Ok(id) => doc_mut!(cx.editor, &id),
                             Err(e) => {
                                 cx.editor.set_error(format!(
                                     "Failed to open file '{}': {}",
@@ -2209,10 +2209,9 @@ fn global_search(cx: &mut Context) {
                                 ));
                                 return;
                             }
-                        }
+                        };
-
                         let line_num = *line_num;
-                        let (view, doc) = current!(cx.editor);
+                        let view = view_mut!(cx.editor);
                         let text = doc.text();
                         if line_num >= text.len_lines() {
                             cx.editor.set_error("The line you jumped to does not exist anymore because the file has changed.");
@@ -2222,7 +2221,9 @@ fn global_search(cx: &mut Context) {
                         let end = text.line_to_char((line_num + 1).min(text.len_lines()));
 
                         doc.set_selection(view.id, Selection::single(start, end));
-                        align_view(doc, view, Align::Center);
+                        if action.align_view(view, doc.id()){
+                            align_view(doc, view, Align::Center);
+                        }
                     }).with_preview(|_editor, FileResult { path, line_num }| {
                         Some((path.clone().into(), Some((*line_num, *line_num))))
                     });
@@ -2742,9 +2743,11 @@ fn jumplist_picker(cx: &mut Context) {
         |cx, meta, action| {
             cx.editor.switch(meta.id, action);
             let config = cx.editor.config();
-            let (view, doc) = current!(cx.editor);
+            let (view, doc) = (view_mut!(cx.editor), doc_mut!(cx.editor, &meta.id));
             doc.set_selection(view.id, meta.selection.clone());
-            view.ensure_cursor_in_view_center(doc, config.scrolloff);
+            if action.align_view(view, doc.id()) {
+                view.ensure_cursor_in_view_center(doc, config.scrolloff);
+            }
         },
     )
     .with_preview(|editor, meta| {

helix-term/src/commands/lsp.rs

@@ -195,7 +195,6 @@ fn location_to_file_location(location: &lsp::Location) -> FileLocation {
     (path.into(), line)
 }
 
-// TODO: share with symbol picker(symbol.location)
 fn jump_to_location(
     editor: &mut Editor,
     location: &lsp::Location,
@@ -213,15 +212,16 @@ fn jump_to_location(
             return;
         }
     };
-    match editor.open(&path, action) {
+
-        Ok(_) => (),
+    let doc = match editor.open(&path, action) {
+        Ok(id) => doc_mut!(editor, &id),
         Err(err) => {
             let err = format!("failed to open path: {:?}: {:?}", location.uri, err);
             editor.set_error(err);
             return;
         }
-    }
+    };
-    let (view, doc) = current!(editor);
+    let view = view_mut!(editor);
     // TODO: convert inside server
     let new_range =
         if let Some(new_range) = lsp_range_to_range(doc.text(), location.range, offset_encoding) {
@@ -233,45 +233,22 @@ fn jump_to_location(
     // we flip the range so that the cursor sits on the start of the symbol
     // (for example start of the function).
     doc.set_selection(view.id, Selection::single(new_range.head, new_range.anchor));
-    align_view(doc, view, Align::Center);
+    if action.align_view(view, doc.id()) {
+        align_view(doc, view, Align::Center);
+    }
 }
 
 type SymbolPicker = Picker<SymbolInformationItem>;
 
 fn sym_picker(symbols: Vec<SymbolInformationItem>, current_path: Option<lsp::Url>) -> SymbolPicker {
     // TODO: drop current_path comparison and instead use workspace: bool flag?
-    Picker::new(symbols, current_path.clone(), move |cx, item, action| {
+    Picker::new(symbols, current_path, move |cx, item, action| {
-        let (view, doc) = current!(cx.editor);
+        jump_to_location(
-        push_jump(view, doc);
+            cx.editor,
-
+            &item.symbol.location,
-        if current_path.as_ref() != Some(&item.symbol.location.uri) {
+            item.offset_encoding,
-            let uri = &item.symbol.location.uri;
+            action,
-            let path = match uri.to_file_path() {
+        );
-                Ok(path) => path,
-                Err(_) => {
-                    let err = format!("unable to convert URI to filepath: {}", uri);
-                    cx.editor.set_error(err);
-                    return;
-                }
-            };
-            if let Err(err) = cx.editor.open(&path, action) {
-                let err = format!("failed to open document: {}: {}", uri, err);
-                log::error!("{}", err);
-                cx.editor.set_error(err);
-                return;
-            }
-        }
-
-        let (view, doc) = current!(cx.editor);
-
-        if let Some(range) =
-            lsp_range_to_range(doc.text(), item.symbol.location.range, item.offset_encoding)
-        {
-            // we flip the range so that the cursor sits on the start of the symbol
-            // (for example start of the function).
-            doc.set_selection(view.id, Selection::single(range.head, range.anchor));
-            align_view(doc, view, Align::Center);
-        }
     })
     .with_preview(move |_editor, item| Some(location_to_file_location(&item.symbol.location)))
     .truncate_start(false)
@@ -286,7 +263,7 @@ enum DiagnosticsFormat {
 fn diag_picker(
     cx: &Context,
     diagnostics: BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
-    current_path: Option<lsp::Url>,
+    _current_path: Option<lsp::Url>,
     format: DiagnosticsFormat,
 ) -> Picker<PickerDiagnostic> {
     // TODO: drop current_path comparison and instead use workspace: bool flag?
@@ -324,22 +301,12 @@ fn diag_picker(
                   offset_encoding,
               },
               action| {
-            if current_path.as_ref() == Some(url) {
+            jump_to_location(
-                let (view, doc) = current!(cx.editor);
+                cx.editor,
-                push_jump(view, doc);
+                &lsp::Location::new(url.clone(), diag.range),
-            } else {
+                *offset_encoding,
-                let path = url.to_file_path().unwrap();
+                action,
-                cx.editor.open(&path, action).expect("editor.open failed");
+            )
-            }
-
-            let (view, doc) = current!(cx.editor);
-
-            if let Some(range) = lsp_range_to_range(doc.text(), diag.range, *offset_encoding) {
-                // we flip the range so that the cursor sits on the start of the symbol
-                // (for example start of the function).
-                doc.set_selection(view.id, Selection::single(range.head, range.anchor));
-                align_view(doc, view, Align::Center);
-            }
         },
     )
     .with_preview(move |_editor, PickerDiagnostic { url, diag, .. }| {

helix-term/tests/integration.rs

@@ -19,6 +19,7 @@ mod test {
     mod auto_pairs;
     mod commands;
     mod movement;
+    mod picker;
     mod prompt;
     mod splits;
 }

helix-term/tests/test/picker.rs

@@ -0,0 +1,80 @@
+use std::fs;
+
+use helix_core::{path::get_canonicalized_path, Range};
+use helix_loader::{current_working_dir, set_current_working_dir};
+use helix_view::{current_ref, editor::Action};
+use tempfile::{Builder, TempDir};
+
+use super::*;
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_picker_alt_ret() -> anyhow::Result<()> {
+    // Create two files, open the first and run a global search for a word
+    // from the second file. Press <alt-ret> to have helix open the second file in the
+    // new buffer, but not change focus. Then check whether the word is highlighted
+    // correctly and the view of the first file has not changed.
+    let tmp_dir = TempDir::new()?;
+    set_current_working_dir(tmp_dir.path().into())?;
+
+    let mut app = AppBuilder::new().build()?;
+
+    log::debug!(
+        "set current working directory to {:?}",
+        current_working_dir()
+    );
+
+    // Add prefix so helix doesn't hide these files in a picker
+    let files = [
+        Builder::new().prefix("1").tempfile_in(&tmp_dir)?,
+        Builder::new().prefix("2").tempfile_in(&tmp_dir)?,
+    ];
+    let paths = files
+        .iter()
+        .map(|f| get_canonicalized_path(f.path()).unwrap())
+        .collect::<Vec<_>>();
+
+    fs::write(&paths[0], "1\n2\n3\n4")?;
+    fs::write(&paths[1], "first\nsecond")?;
+
+    log::debug!(
+        "created and wrote two temporary files: {:?} & {:?}",
+        paths[0],
+        paths[1]
+    );
+
+    // Manually open to save the offset, otherwise we won't be able to change the state in the Fn trait
+    app.editor.open(files[0].path(), Action::Replace)?;
+    let view_offset = current_ref!(app.editor).0.offset;
+
+    test_key_sequences(
+        &mut app,
+        vec![
+            (Some("<space>/"), None),
+            (Some("second<ret>"), None),
+            (
+                Some("<A-ret><esc>"),
+                Some(&|app| {
+                    let (view, doc) = current_ref!(app.editor);
+                    assert_eq!(doc.path().unwrap(), &paths[0]);
+                    let select_ranges = doc.selection(view.id).ranges();
+                    assert_eq!(select_ranges[0], Range::new(0, 1));
+                    assert_eq!(view.offset, view_offset);
+                }),
+            ),
+            (
+                Some(":buffer<minus>next<ret>"),
+                Some(&|app| {
+                    let (view, doc) = current_ref!(app.editor);
+                    assert_eq!(doc.path().unwrap(), &paths[1]);
+                    let select_ranges = doc.selection(view.id).ranges();
+                    assert_eq!(select_ranges.len(), 1);
+                    assert_eq!(select_ranges[0], Range::new(6, 12));
+                }),
+            ),
+        ],
+        false,
+    )
+    .await?;
+
+    Ok(())
+}

helix-view/src/editor.rs

@@ -995,6 +995,13 @@ pub enum Action {
     VerticalSplit,
 }
 
+impl Action {
+    /// Whether to align the view to the cursor after executing this action
+    pub fn align_view(&self, view: &View, new_doc: DocumentId) -> bool {
+        !matches!((self, view.doc == new_doc), (Action::Load, false))
+    }
+}
+
 /// Error thrown on failed document closed
 pub enum CloseError {
     /// Document doesn't exist