add select_next_sibling and select_prev_sibling commands #1495

the-mikedavis · 2022-01-12T15:15:19Z

builds on expand_selection and shrink_selection to move to next and previous sibling nodes in the syntax tree

mostly I'm interested in all four of those together an alternate means of motion, like so:

select-movement.mp4

Where I have

A-j shrink selection
A-k expand selection
A-h select prev sibling
A-l select next sibling

in my personal config. I don't know if these commands are worthy of a default keybind 🤔

What do you think?

pickfire · 2022-01-12T15:27:36Z

Seemed interesting, do you find you using it frequently and needs to keep holding down alt? If that's the case something like our extend state would be nice, otherwise the keys you show are quite good already.

Or maybe { and }? Last time I use that for paragraph but I navigate in a similar manner, I think would be nice if the language support tree-sitter than we use this, and if not we fallback to paragraph.

pickfire · 2022-01-12T15:31:27Z

helix-core/src/object.rs

+    selection.clone().transform(|range| {
+        let from = text.char_to_byte(range.from());
+        let to = text.char_to_byte(range.to());
+
+        let sibling = match tree
+            .root_node()
+            .descendant_for_byte_range(from, to)
+            .and_then(find_prev_sibling)
+        {
+            Some(sibling) => sibling,
+            None => return range,
+        };
+
+        let from = text.byte_to_char(sibling.start_byte());
+        let to = text.byte_to_char(sibling.end_byte());
+
+        if range.head < range.anchor {
+            Range::new(to, from)
+        } else {
+            Range::new(from, to)
+        }
+    })


This whole thing seemed like it is duplicated, probably can reuse.

Yep it's all duplicated. I tried out refactoring it to take the find_prev_sibling/find_next_sibling function as an argument but I couldn't figure out the borrowing rules. I'll keep looking into it 🤔

(ofc if you'd like to push a commit to cut down on that boilerplate, I'd be grateful 🙂)

I figured out how to do higher order functions but now I think I may have overdone it 😄

the-mikedavis · 2022-01-12T15:33:26Z

Ah that's a good idea, an extended state would be great

That's not something we can setup in the config toml currently, right? I'd love a built-in keybinding for that but idk how many others would find this way of doing motion useful

pickfire · 2022-01-12T15:35:00Z

That's not something we can setup in the config toml currently, right? I'd love a built-in keybinding for that but idk how many others would find this way of doing motion useful

Yeah, extended state is not possible with current config mechanism. Not sure if we want this also, I think as of now integrating to the editor is easier since cases like this is rare. cc @sudormrfbin

sudormrfbin · 2022-01-13T14:54:33Z

(By extended state I assume you mean "sticky" mode, like in Z) - It's not currently configurable primarily because I couldn't find a good syntax for it in the toml config file. How should a sub-keymap be marked as sticky ? Maybe a special properties field for keybinds ? I'm not sure about the design.

the-mikedavis · 2022-01-13T15:35:38Z

err yep, "sticky"

I haven't really thought this through, but one way I'm thinking that could work: things like keys.insert and keys.normal but with a custom mode name like keys.foo and then you can bind something to switch into that mode, like

[keys.normal]
"T" = "enter_mode_once treejump" # not sticky
"A-t" = "enter_mode treejump" # sticky

[keys.treejump]
"j" = "shrink_selection"
"k" = "expand_selection"
"h" = "select_prev_sibling"
"l" = "select_next_sibling"

but for this to work you'd need to be able to bind keys to commands that take arguments which I think is not currently possible, right?

pickfire · 2022-01-14T14:26:26Z

Using [s like @dead10ck did in #1506 seemed like a good idea too, but I think it may be more preferable be { since it seemed more useful.

dead10ck

Looks great to me! As for bindings, I just went with what I thought was consistent with the current expand/shrink bindings, but I personally plan to bind them to C-{h,j,k,l} in my own config. I find these extremely useful, so I want them almost as easily accessible as moving the cursor itself.

helix-core/src/object.rs

dead10ck · 2022-01-14T15:00:08Z

helix-core/src/object.rs

+    F: Fn(Node) -> Option<Node>,
+{
+    sibling_fn(node).or_else(|| {
+        node.parent()


Nice idea to recurse up the tree until you find a sibling. 👍

^ this, was admiring exactly the same thing

dead10ck · 2022-01-14T15:02:51Z

helix-term/src/commands.rs

+}
+
+fn select_prev_sibling(cx: &mut Context) {
+    let motion = |editor: &mut Editor| {


I tried to factor these too so it's not duplicated everywhere except the sibling call, but I couldn't figure out how to make the borrow checker happy. I tried making a fn(&Syntax, RopeSlice, &Selection) argument, but it didn't like that the function argument gets captured by the motion closure.

I'm a little afraid of the borrow checker 😅, I'll give this a shot but I'm not optimistic

edit: it didn't go well 🙃

Could do something like this:

helix/helix-term/src/commands.rs

Lines 488 to 535 in 5549acd

fn move_impl<F>(cx: &mut Context, move_fn: F, dir: Direction, behaviour: Movement)

where

F: Fn(RopeSlice, Range, Direction, usize, Movement) -> Range,

{

let count = cx.count();

let (view, doc) = current!(cx.editor);

let text = doc.text().slice(..);

let selection = doc

.selection(view.id)

.clone()

.transform(|range| move_fn(text, range, dir, count, behaviour));

doc.set_selection(view.id, selection);

}

use helix_core::movement::{move_horizontally, move_vertically};

fn move_char_left(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Backward, Movement::Move)

}

fn move_char_right(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Forward, Movement::Move)

}

fn move_line_up(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Backward, Movement::Move)

}

fn move_line_down(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Forward, Movement::Move)

}

fn extend_char_left(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Backward, Movement::Extend)

}

fn extend_char_right(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Forward, Movement::Extend)

}

fn extend_line_up(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Backward, Movement::Extend)

}

fn extend_line_down(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Forward, Movement::Extend)

}

pickfire

Looks good to me, tested and it works.

the-mikedavis · 2022-01-14T15:45:16Z

I think it may be more preferable be { since it seemed more useful.

yeah looks like [ is otherwise lsp diagnostic jumping, maybe {/} for these selection commands would fit better? There isn't anything bound for those keys yet

pickfire · 2022-01-14T16:01:42Z

Hmm, I wonder if we should not use ]s since it is used for next spell in vim and kak, should we pick a key that is not used in vim and at the same time be clearer? Maybe ]t which means next tree item?

pickfire

Looks good to me. @dead10ck what do you think about the key? If it's good then I guess I can merge this.

dead10ck · 2022-01-15T16:03:38Z

That seems fine to me. It seems consistent with the default bindings. I'm going to rebind it anyway, so I don't have big concerns about the default.

archseer

I'd be okay with adding the Alt- mappings and we can experiment with them in master. It makes it easy to repeat without a sticky mode since you can just hold down Alt

archseer · 2022-01-16T01:37:28Z

helix-term/src/commands.rs

+}
+
+fn select_prev_sibling(cx: &mut Context) {
+    let motion = |editor: &mut Editor| {


Could do something like this:

helix/helix-term/src/commands.rs

Lines 488 to 535 in 5549acd

fn move_impl<F>(cx: &mut Context, move_fn: F, dir: Direction, behaviour: Movement)

where

F: Fn(RopeSlice, Range, Direction, usize, Movement) -> Range,

{

let count = cx.count();

let (view, doc) = current!(cx.editor);

let text = doc.text().slice(..);

let selection = doc

.selection(view.id)

.clone()

.transform(|range| move_fn(text, range, dir, count, behaviour));

doc.set_selection(view.id, selection);

}

use helix_core::movement::{move_horizontally, move_vertically};

fn move_char_left(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Backward, Movement::Move)

}

fn move_char_right(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Forward, Movement::Move)

}

fn move_line_up(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Backward, Movement::Move)

}

fn move_line_down(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Forward, Movement::Move)

}

fn extend_char_left(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Backward, Movement::Extend)

}

fn extend_char_right(cx: &mut Context) {

move_impl(cx, move_horizontally, Direction::Forward, Movement::Extend)

}

fn extend_line_up(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Backward, Movement::Extend)

}

fn extend_line_down(cx: &mut Context) {

move_impl(cx, move_vertically, Direction::Forward, Movement::Extend)

}

dead10ck

Nicely done with the refactor!

pickfire · 2022-01-17T00:06:50Z

book/src/keymap.md

+| `Alt-k`  | Expand selection to parent syntax node                            | `expand_selection`                   |
+| `Alt-j`  | Shrink syntax tree object selection                               | `shrink_selection`                   |
+| `Alt-h`  | Select previous sibling node in syntax tree                       | `select_prev_sibling`                |
+| `Alt-l`  | Select next sibling node in syntax tree                           | `select_next_sibling`                |


This seemed a bit weird to me, shouldn't it be hl to expand/shrink and hj to navigate between sibling?

This makes sense to me. If you picture the tree vertically, with expand, you are going up the tree, shrink you are going down. Next sibling is moving laterally to the right, and previous is left.

I think about it the same way ☝️

I could see it going the other way too because often A-l will move your selection downwards (to the next function block for example), but that depends on the size of the selection (e.g. A-l will move right in a function's argument list if it's all on one line) which can be a little confusing

pickfire · 2022-01-17T00:08:19Z

helix-term/src/commands.rs

            doc.set_selection(view.id, selection);
        }
    };
    motion(cx.editor);
    cx.editor.last_motion = Some(Motion(Box::new(motion)));
 }

+fn select_sibling_impl<F>(cx: &mut Context, sibling_fn: &'static F)


I think we can remove the &'static and just leave F. It seemed weird to pass the function as reference. In the meantime, I think we can just do select_sibling_impl(cx, Node::next_sibling).

the borrow checker gets upset here without the &:

error[E0507]: cannot move out of `sibling_fn`, a captured variable in an `Fn` closure --> helix-core/src/object.rs:30:44 | 24 | sibling_fn: F, | ---------- captured outer variable ... 29 | select_node_impl(syntax, text, selection, |descendant, _from, _to| { | _______________________________________________- 30 | | find_sibling_recursive(descendant, sibling_fn) | | ^^^^^^^^^^ move occurs because `sibling_fn` has type `F`, which does not implement the `Copy` trait 31 | | }) | |_____- captured by this `Fn` closure For more information about this error, try `rustc --explain E0507`.

and also if we only remove the 'static lifetime bounds:

error[E0310]: the parameter type `F` may not live long enough --> helix-term/src/commands.rs:5546:41 | 5530 | fn select_sibling_impl<F>(cx: &mut Context, sibling_fn: &F) | - help: consider adding an explicit lifetime bound...: `F: 'static` ... 5546 | cx.editor.last_motion = Some(Motion(Box::new(motion))); | ^^^^^^^^ ...so that the reference type `&F` does not outlive the data it points at error[E0310]: the parameter type `F` may not live long enough --> helix-term/src/commands.rs:5546:41 | 5530 | fn select_sibling_impl<F>(cx: &mut Context, sibling_fn: &F) | - help: consider adding an explicit lifetime bound...: `F: 'static` ... 5546 | cx.editor.last_motion = Some(Motion(Box::new(motion))); | ^^^^^^^^^^^^^^^^ ...so that the type `[closure@helix-term/src/commands.rs:5534:18: 5544:6]` will meet its required lifetime bounds For more information about this error, try `rustc --explain E0310`.

Is there a way to make the borrow checker happy in these cases?

From what I understand the 'static bound is necessary for functions and there isn't such a thing as a non-'static fn. I can't find a decent explainer article for it anymore though

archseer · 2022-01-17T14:00:44Z

helix-term/src/commands.rs

+}
+
+fn select_next_sibling(cx: &mut Context) {
+    select_sibling_impl(cx, &|node| Node::next_sibling(&node))


Suggested change

select_sibling_impl(cx, &|node| Node::next_sibling(&node))

select_sibling_impl(cx, Node::next_sibling)

Does this compile?

looks like the typing is slightly off, rustc wants a &Node::next_sibling:

error[E0308]: mismatched types --> helix-term/src/commands.rs:5550:29 | 5550 | select_sibling_impl(cx, Node::next_sibling) | ^^^^^^^^^^^^^^^^^^ | | | expected reference, found fn item | help: consider borrowing here: `&Node::next_sibling` | = note: expected reference `&'static _` found fn item `for<'r> fn(&'r helix_core::tree_sitter::Node<'_>) -> std::option::Option<helix_core::tree_sitter::Node<'_>> {helix_core::tree_sitter::Node::<'_>::next_sibling}` error[E0308]: mismatched types --> helix-term/src/commands.rs:5554:29 | 5554 | select_sibling_impl(cx, Node::prev_sibling) | ^^^^^^^^^^^^^^^^^^ | | | expected reference, found fn item | help: consider borrowing here: `&Node::prev_sibling` | = note: expected reference `&'static _` found fn item `for<'r> fn(&'r helix_core::tree_sitter::Node<'_>) -> std::option::Option<helix_core::tree_sitter::Node<'_>> {helix_core::tree_sitter::Node::<'_>::prev_sibling}` For more information about this error, try `rustc --explain E0308`. error: could not compile `helix-term` due to 2 previous errors

But then doing &Node::next_sibling:

error[E0631]: type mismatch in function arguments --> helix-term/src/commands.rs:5550:29 | 5550 | select_sibling_impl(cx, &Node::next_sibling) | ------------------- ^^^^^^^^^^^^^^^^^^^ | | | | | expected signature of `for<'r> fn(helix_core::tree_sitter::Node<'r>) -> _` | | found signature of `for<'r> fn(&'r helix_core::tree_sitter::Node<'_>) -> _` | required by a bound introduced by this call | note: required by a bound in `select_sibling_impl` --> helix-term/src/commands.rs:5532:8 | 5530 | fn select_sibling_impl<F>(cx: &mut Context, sibling_fn: &'static F) | ------------------- required by a bound in this 5531 | where 5532 | F: Fn(Node) -> Option<Node>, | ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `select_sibling_impl`

add select_next_sibling and select_prev_sibling commands

855a0de

pickfire reviewed Jan 12, 2022

View reviewed changes

the-mikedavis added 2 commits January 13, 2022 10:32

refactor objects to use higher order functions

7f56104

address clippy feedback

807b64f

archseer mentioned this pull request Jan 14, 2022

feat(commands): sibling selections #1506

Closed

dead10ck approved these changes Jan 14, 2022

View reviewed changes

pickfire approved these changes Jan 14, 2022

View reviewed changes

the-mikedavis added 2 commits January 14, 2022 09:39

move selection cloning into commands

0c74d4e

add default keybindings under left/right brackets

d1df47f

use [+t,]+t for selecting sibling syntax nodes

5549acd

pickfire approved these changes Jan 15, 2022

View reviewed changes

archseer reviewed Jan 16, 2022

View reviewed changes

setup Alt-{j,k,h,l} default keymaps for syntax selection commands

198600c

the-mikedavis force-pushed the md-select-next-prev-sibling branch from a0d0129 to 198600c Compare January 16, 2022 02:39

the-mikedavis added 2 commits January 15, 2022 20:41

reduce boilerplate of select_next/prev_sibling in commands

b337a5b

import tree-sitter Node type in commands

1ce524b

dead10ck approved these changes Jan 16, 2022

View reviewed changes

pickfire reviewed Jan 17, 2022

View reviewed changes

archseer reviewed Jan 17, 2022

View reviewed changes

archseer merged commit 392dfa0 into helix-editor:master Jan 20, 2022

the-mikedavis deleted the md-select-next-prev-sibling branch January 20, 2022 15:55

the-mikedavis mentioned this pull request Feb 24, 2022

add tags.scm queries gleam-lang/tree-sitter-gleam#15

Merged

the-mikedavis mentioned this pull request Jul 15, 2022

Select sibling syntax node should not select parent node's sibling #3079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add select_next_sibling and select_prev_sibling commands #1495

add select_next_sibling and select_prev_sibling commands #1495

the-mikedavis commented Jan 12, 2022 •

edited

pickfire commented Jan 12, 2022 •

edited

pickfire Jan 12, 2022

the-mikedavis Jan 12, 2022

the-mikedavis Jan 13, 2022

the-mikedavis commented Jan 12, 2022

pickfire commented Jan 12, 2022

sudormrfbin commented Jan 13, 2022

the-mikedavis commented Jan 13, 2022

pickfire commented Jan 14, 2022

dead10ck left a comment

dead10ck Jan 14, 2022

matoous Jan 16, 2022

dead10ck Jan 14, 2022

the-mikedavis Jan 14, 2022 •

edited

archseer Jan 16, 2022

pickfire left a comment

the-mikedavis commented Jan 14, 2022

pickfire commented Jan 14, 2022 •

edited

pickfire left a comment •

edited

dead10ck commented Jan 15, 2022

archseer left a comment

archseer Jan 16, 2022

dead10ck left a comment

pickfire Jan 17, 2022

dead10ck Jan 17, 2022

the-mikedavis Jan 17, 2022

pickfire Jan 17, 2022 •

edited

the-mikedavis Jan 17, 2022

archseer Jan 17, 2022

archseer Jan 17, 2022

the-mikedavis Jan 17, 2022

	fn move_impl<F>(cx: &mut Context, move_fn: F, dir: Direction, behaviour: Movement)
	where
	F: Fn(RopeSlice, Range, Direction, usize, Movement) -> Range,
	{
	let count = cx.count();
	let (view, doc) = current!(cx.editor);
	let text = doc.text().slice(..);

	let selection = doc
	.selection(view.id)
	.clone()
	.transform(\|range\| move_fn(text, range, dir, count, behaviour));
	doc.set_selection(view.id, selection);
	}

	use helix_core::movement::{move_horizontally, move_vertically};

	fn move_char_left(cx: &mut Context) {
	move_impl(cx, move_horizontally, Direction::Backward, Movement::Move)
	}

	fn move_char_right(cx: &mut Context) {
	move_impl(cx, move_horizontally, Direction::Forward, Movement::Move)
	}

	fn move_line_up(cx: &mut Context) {
	move_impl(cx, move_vertically, Direction::Backward, Movement::Move)
	}

	fn move_line_down(cx: &mut Context) {
	move_impl(cx, move_vertically, Direction::Forward, Movement::Move)
	}

	fn extend_char_left(cx: &mut Context) {
	move_impl(cx, move_horizontally, Direction::Backward, Movement::Extend)
	}

	fn extend_char_right(cx: &mut Context) {
	move_impl(cx, move_horizontally, Direction::Forward, Movement::Extend)
	}

	fn extend_line_up(cx: &mut Context) {
	move_impl(cx, move_vertically, Direction::Backward, Movement::Extend)
	}

	fn extend_line_down(cx: &mut Context) {
	move_impl(cx, move_vertically, Direction::Forward, Movement::Extend)
	}

	select_sibling_impl(cx, &\|node\| Node::next_sibling(&node))
	select_sibling_impl(cx, Node::next_sibling)

add select_next_sibling and select_prev_sibling commands #1495

add select_next_sibling and select_prev_sibling commands #1495

Conversation

the-mikedavis commented Jan 12, 2022 • edited

pickfire commented Jan 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-mikedavis commented Jan 12, 2022

pickfire commented Jan 12, 2022

sudormrfbin commented Jan 13, 2022

the-mikedavis commented Jan 13, 2022

pickfire commented Jan 14, 2022

dead10ck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-mikedavis Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickfire left a comment

Choose a reason for hiding this comment

the-mikedavis commented Jan 14, 2022

pickfire commented Jan 14, 2022 • edited

pickfire left a comment • edited

Choose a reason for hiding this comment

dead10ck commented Jan 15, 2022

archseer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead10ck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickfire Jan 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-mikedavis commented Jan 12, 2022 •

edited

pickfire commented Jan 12, 2022 •

edited

the-mikedavis Jan 14, 2022 •

edited

pickfire commented Jan 14, 2022 •

edited

pickfire left a comment •

edited

pickfire Jan 17, 2022 •

edited