Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add select_next_sibling and select_prev_sibling commands #1495

Merged
merged 9 commits into from
Jan 20, 2022
Merged

add select_next_sibling and select_prev_sibling commands #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
855a0de
add select_next_sibling and select_prev_sibling commands
the-mikedavis Jan 12, 2022
7f56104
refactor objects to use higher order functions
the-mikedavis Jan 13, 2022
807b64f
address clippy feedback
the-mikedavis Jan 13, 2022
0c74d4e
move selection cloning into commands
the-mikedavis Jan 14, 2022
d1df47f
add default keybindings under left/right brackets
the-mikedavis Jan 14, 2022
5549acd
use [+t,]+t for selecting sibling syntax nodes
the-mikedavis Jan 14, 2022
198600c
setup Alt-{j,k,h,l} default keymaps for syntax selection commands
the-mikedavis Jan 16, 2022
b337a5b
reduce boilerplate of select_next/prev_sibling in commands
the-mikedavis Jan 16, 2022
1ce524b
import tree-sitter Node type in commands
the-mikedavis Jan 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions book/src/keymap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@
| `Alt-K` | Remove selections matching the regex | `remove_selections` |
| `$` | Pipe each selection into shell command, keep selections where command returned 0 | `shell_keep_pipe` |
| `Ctrl-c` | Comment/uncomment the selections | `toggle_comments` |
| `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` |
Comment on lines +121 to +124
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


### Search

Expand Down Expand Up @@ -261,8 +265,6 @@ Mappings in the style of [vim-unimpaired](https://github.com/tpope/vim-unimpaire
| `]D` | Go to last diagnostic in document (**LSP**) | `goto_last_diag` |
| `[space` | Add newline above | `add_newline_above` |
| `]space` | Add newline below | `add_newline_below` |
| `]o` | Expand syntax tree object selection. | `expand_selection` |
| `[o` | Shrink syntax tree object selection. | `shrink_selection` |

## Insert Mode

Expand Down
90 changes: 53 additions & 37 deletions helix-core/src/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,56 +1,72 @@
use crate::{Range, RopeSlice, Selection, Syntax};
use tree_sitter::Node;

pub fn expand_selection(syntax: &Syntax, text: RopeSlice, selection: &Selection) -> Selection {
let tree = syntax.tree();

selection.clone().transform(|range| {
let from = text.char_to_byte(range.from());
let to = text.char_to_byte(range.to());
pub fn expand_selection(syntax: &Syntax, text: RopeSlice, selection: Selection) -> Selection {
select_node_impl(syntax, text, selection, |descendant, from, to| {
if descendant.start_byte() == from && descendant.end_byte() == to {
descendant.parent()
} else {
Some(descendant)
}
})
}

// find parent of a descendant that matches the range
let parent = match tree
.root_node()
.descendant_for_byte_range(from, to)
.and_then(|node| {
if node.start_byte() == from && node.end_byte() == to {
node.parent()
} else {
Some(node)
}
}) {
Some(parent) => parent,
None => return range,
};
pub fn shrink_selection(syntax: &Syntax, text: RopeSlice, selection: Selection) -> Selection {
select_node_impl(syntax, text, selection, |descendant, _from, _to| {
descendant.child(0).or(Some(descendant))
})
}

let from = text.byte_to_char(parent.start_byte());
let to = text.byte_to_char(parent.end_byte());
pub fn select_sibling<F>(
syntax: &Syntax,
text: RopeSlice,
selection: Selection,
sibling_fn: &F,
) -> Selection
where
F: Fn(Node) -> Option<Node>,
{
select_node_impl(syntax, text, selection, |descendant, _from, _to| {
find_sibling_recursive(descendant, sibling_fn)
})
}

if range.head < range.anchor {
Range::new(to, from)
} else {
Range::new(from, to)
}
fn find_sibling_recursive<F>(node: Node, sibling_fn: F) -> Option<Node>
where
F: Fn(Node) -> Option<Node>,
{
sibling_fn(node).or_else(|| {
node.parent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ this, was admiring exactly the same thing

.and_then(|node| find_sibling_recursive(node, sibling_fn))
})
}

pub fn shrink_selection(syntax: &Syntax, text: RopeSlice, selection: &Selection) -> Selection {
fn select_node_impl<F>(
syntax: &Syntax,
text: RopeSlice,
selection: Selection,
select_fn: F,
) -> Selection
where
F: Fn(Node, usize, usize) -> Option<Node>,
{
let tree = syntax.tree();

selection.clone().transform(|range| {
selection.transform(|range| {
let from = text.char_to_byte(range.from());
let to = text.char_to_byte(range.to());

let descendant = match tree.root_node().descendant_for_byte_range(from, to) {
// find first child, if not possible, fallback to the node that contains selection
Some(descendant) => match descendant.child(0) {
Some(child) => child,
None => descendant,
},
let node = match tree
.root_node()
.descendant_for_byte_range(from, to)
.and_then(|node| select_fn(node, from, to))
{
Some(node) => node,
None => return range,
};

let from = text.byte_to_char(descendant.start_byte());
let to = text.byte_to_char(descendant.end_byte());
let from = text.byte_to_char(node.start_byte());
let to = text.byte_to_char(node.end_byte());

if range.head < range.anchor {
Range::new(to, from)
Expand Down
34 changes: 32 additions & 2 deletions helix-term/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use helix_core::{
object, pos_at_coords,
regex::{self, Regex, RegexBuilder},
search, selection, shellwords, surround, textobject,
tree_sitter::Node,
unicode::width::UnicodeWidthChar,
LineEnding, Position, Range, Rope, RopeGraphemes, RopeSlice, Selection, SmallVec, Tendril,
Transaction,
Expand Down Expand Up @@ -363,6 +364,8 @@ impl MappableCommand {
rotate_selection_contents_backward, "Rotate selections contents backward",
expand_selection, "Expand selection to parent syntax node",
shrink_selection, "Shrink selection to previously expanded syntax node",
select_next_sibling, "Select the next sibling in the syntax tree",
select_prev_sibling, "Select the previous sibling in the syntax tree",
jump_forward, "Jump forward on jumplist",
jump_backward, "Jump backward on jumplist",
save_selection, "Save the current selection to the jumplist",
Expand Down Expand Up @@ -5490,7 +5493,7 @@ fn expand_selection(cx: &mut Context) {
// save current selection so it can be restored using shrink_selection
view.object_selections.push(current_selection.clone());

let selection = object::expand_selection(syntax, text, current_selection);
let selection = object::expand_selection(syntax, text, current_selection.clone());
doc.set_selection(view.id, selection);
}
};
Expand All @@ -5516,14 +5519,41 @@ fn shrink_selection(cx: &mut Context) {
// if not previous selection, shrink to first child
if let Some(syntax) = doc.syntax() {
let text = doc.text().slice(..);
let selection = object::shrink_selection(syntax, text, current_selection);
let selection = object::shrink_selection(syntax, text, current_selection.clone());
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)
Copy link
Contributor

@pickfire pickfire Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

where
F: Fn(Node) -> Option<Node>,
{
let motion = |editor: &mut Editor| {
let (view, doc) = current!(editor);

if let Some(syntax) = doc.syntax() {
let text = doc.text().slice(..);
let current_selection = doc.selection(view.id);
let selection =
object::select_sibling(syntax, text, current_selection.clone(), sibling_fn);
doc.set_selection(view.id, selection);
}
};
motion(cx.editor);
cx.editor.last_motion = Some(Motion(Box::new(motion)));
}

fn select_next_sibling(cx: &mut Context) {
select_sibling_impl(cx, &|node| Node::next_sibling(&node))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Does this compile?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

}

fn select_prev_sibling(cx: &mut Context) {
select_sibling_impl(cx, &|node| Node::prev_sibling(&node))
}

fn match_brackets(cx: &mut Context) {
let (view, doc) = current!(cx.editor);

Expand Down
7 changes: 5 additions & 2 deletions helix-term/src/keymap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,11 @@ impl Default for Keymaps {
"S" => split_selection,
";" => collapse_selection,
"A-;" => flip_selections,
"A-k" => expand_selection,
"A-j" => shrink_selection,
"A-h" => select_prev_sibling,
"A-l" => select_next_sibling,

"%" => select_all,
"x" => extend_line,
"X" => extend_to_line_bounds,
Expand All @@ -569,13 +574,11 @@ impl Default for Keymaps {
"d" => goto_prev_diag,
"D" => goto_first_diag,
"space" => add_newline_above,
"o" => shrink_selection,
},
"]" => { "Right bracket"
"d" => goto_next_diag,
"D" => goto_last_diag,
"space" => add_newline_below,
"o" => expand_selection,
},

"/" => search,
Expand Down