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

woojiq · 2023-07-20T20:03:47Z

Now if alt-ret is pressed, it opens the selected file and places the cursor on the selected line (if path contained a line number), but in the background (i. e the focused document remains unchanged).

All lsp pickers
Global search picker
Jump list picker

Closes helix-editor#7673

* symbol pickers * diagnostick pickers This is done using the already patched `jump_to_location` method.

woojiq · 2023-07-22T13:06:58Z

Here we go again. Now all the lsp pickers work correctly because under the hood I added a jump_to_location method instead of their own almost identical implementation. I wanted to add something similar like this method to the non-lsp pickers, but they use slightly different logic, so I just changed the way they get doc_id.

cc @pascalkuthe pr is ready for viewing when you have free time 🤓

helix-view/src/view.rs

pascalkuthe · 2023-07-31T10:59:18Z

helix-term/src/commands/lsp.rs

-            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,
+            )


this is not quite the same thing. The diagnostic picker only seems to create a jumplist entry if not opening a new file. Admittetly this is a bit strange I would expect the various Lsp pickers to behavhe consistently. Do you know the history here @the-mikedavis?

I think it's just an oversight: pushing a jump all of the time makes sense to me.

woojiq · 2023-08-01T09:01:43Z

Added basic integrational test for global search picker. I couldn't test lsp pickers because I need to run an lsp which will be hard to do.

pascalkuthe

LGTM, except the change to the diagnostic picker I am not sure about. Let's wait for @the-mikedavis review

the-mikedavis · 2023-08-08T00:56:51Z

helix-term/src/commands/lsp.rs

-            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,
+            )


I think it's just an oversight: pushing a jump all of the time makes sense to me.

helix-term/tests/test/picker.rs

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

…7691) * fix(picker): `alt-ret' changes cursor pos of current file, not new one Closes helix-editor#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>

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

cb5c019

Closes helix-editor#7673

This comment was marked as resolved.

Sign in to view

woojiq marked this pull request as draft July 21, 2023 12:36

fix other pickers

1fdf84e

* symbol pickers * diagnostick pickers This is done using the already patched `jump_to_location` method.

This comment was marked as resolved.

Sign in to view

woojiq marked this pull request as ready for review July 22, 2023 10:06

woojiq marked this pull request as draft July 22, 2023 10:20

fix global and jumplist pickers

47e5e98

woojiq marked this pull request as ready for review July 22, 2023 13:00

pascalkuthe requested changes Jul 31, 2023

View reviewed changes

pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2023

woojiq added 2 commits July 31, 2023 21:02

use view as old_id; make align_view method of Action

2bced89

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

01af29a

woojiq force-pushed the alt-enter-jump-fix branch from dcbd4bb to 01af29a Compare August 1, 2023 08:54

fix: picker integrational test

ba7f312

pascalkuthe previously approved these changes Aug 1, 2023

View reviewed changes

pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2023

the-mikedavis reviewed Aug 8, 2023

View reviewed changes

fix nit

e853290

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

woojiq dismissed pascalkuthe’s stale review via e853290 August 8, 2023 08:37

the-mikedavis approved these changes Aug 8, 2023

View reviewed changes

the-mikedavis changed the title ~~fix(picker): `alt-ret' changes cursor pos of current file, not new one~~ Align view for background buffer opened with alt-ret Aug 8, 2023

pascalkuthe approved these changes Aug 8, 2023

View reviewed changes

pascalkuthe merged commit aa4d84a into helix-editor:master Aug 8, 2023

bjorn-ove mentioned this pull request Aug 9, 2023

Document Alt-Enter for the picker #7884

Merged

woojiq deleted the alt-enter-jump-fix branch August 10, 2023 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

woojiq commented Jul 20, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

woojiq commented Jul 22, 2023 •

edited

Loading

pascalkuthe Jul 31, 2023 •

edited

Loading

the-mikedavis Aug 8, 2023

woojiq commented Aug 1, 2023 •

edited

Loading

pascalkuthe left a comment

the-mikedavis Aug 8, 2023

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

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

Conversation

woojiq commented Jul 20, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

woojiq commented Jul 22, 2023 • edited Loading

pascalkuthe Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

the-mikedavis Aug 8, 2023

Choose a reason for hiding this comment

woojiq commented Aug 1, 2023 • edited Loading

pascalkuthe left a comment

Choose a reason for hiding this comment

the-mikedavis Aug 8, 2023

Choose a reason for hiding this comment

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

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

woojiq commented Jul 20, 2023 •

edited

Loading

woojiq commented Jul 22, 2023 •

edited

Loading

pascalkuthe Jul 31, 2023 •

edited

Loading

woojiq commented Aug 1, 2023 •

edited

Loading