From aa4d84a0b30fbe93882508c6d9dc8ca4c95341d0 Mon Sep 17 00:00:00 2001 From: woojiq <122799969+woojiq@users.noreply.github.com> Date: Tue, 8 Aug 2023 16:17:29 +0300 Subject: [PATCH] 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 functionality * fix: picker integrational test * fix nit Co-authored-by: Michael Davis --------- Co-authored-by: Michael Davis --- helix-term/src/commands.rs | 19 ++++---- helix-term/src/commands/lsp.rs | 77 +++++++++---------------------- helix-term/tests/integration.rs | 1 + helix-term/tests/test/picker.rs | 80 +++++++++++++++++++++++++++++++++ helix-view/src/editor.rs | 7 +++ 5 files changed, 121 insertions(+), 63 deletions(-) create mode 100644 helix-term/tests/test/picker.rs diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index e06225593..8be0f83a8 100644 --- a/helix-term/src/commands.rs +++ b/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) { - Ok(_) => {} + let doc = match cx.editor.open(path, action) { + 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| { diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 17f936cd6..57be1267c 100644 --- a/helix-term/src/commands/lsp.rs +++ b/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; fn sym_picker(symbols: Vec, current_path: Option) -> SymbolPicker { // TODO: drop current_path comparison and instead use workspace: bool flag? - Picker::new(symbols, current_path.clone(), move |cx, item, action| { - let (view, doc) = current!(cx.editor); - push_jump(view, doc); - - if current_path.as_ref() != Some(&item.symbol.location.uri) { - let uri = &item.symbol.location.uri; - 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); - } + Picker::new(symbols, current_path, move |cx, item, action| { + jump_to_location( + cx.editor, + &item.symbol.location, + item.offset_encoding, + action, + ); }) .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>, - current_path: Option, + _current_path: Option, format: DiagnosticsFormat, ) -> Picker { // 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) { - let (view, doc) = current!(cx.editor); - push_jump(view, doc); - } else { - let path = url.to_file_path().unwrap(); - 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); - } + jump_to_location( + cx.editor, + &lsp::Location::new(url.clone(), diag.range), + *offset_encoding, + action, + ) }, ) .with_preview(move |_editor, PickerDiagnostic { url, diag, .. }| { diff --git a/helix-term/tests/integration.rs b/helix-term/tests/integration.rs index d77eefed0..a62f00660 100644 --- a/helix-term/tests/integration.rs +++ b/helix-term/tests/integration.rs @@ -19,6 +19,7 @@ mod test { mod auto_pairs; mod commands; mod movement; + mod picker; mod prompt; mod splits; } diff --git a/helix-term/tests/test/picker.rs b/helix-term/tests/test/picker.rs new file mode 100644 index 000000000..f6d1aa258 --- /dev/null +++ b/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 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::>(); + + 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("/"), None), + (Some("second"), None), + ( + Some(""), + 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(":buffernext"), + 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(()) +} diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 113102b6b..af00864a7 100644 --- a/helix-view/src/editor.rs +++ b/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