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.

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

feat(command): select all siblings and children #7445

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

dead10ck
Copy link
Member

@dead10ck dead10ck commented Jun 25, 2023

This adds two new related commands:

select_all_siblings

Selects all siblings of the node under the cursor.

asciicast

select_all_children

Selects all children of the node under the cursor.

asciicast

select_all_children_in_selection

Selects all children of the node under the cursor, but only if the children are contained inside the starting selection.

Edit: saving this one for a future PR

@pascalkuthe
Copy link
Member

after trying this out for a bit I think select_all_children would benefit a lot from only selecting children that are actually contained in the selection. The current implementation is also useful but i found that quite a lof of nodes have children I don't care about. If someone really wants all children inside the node they can do a-o first. That change would also make the command more predictable I think

@dead10ck
Copy link
Member Author

after trying this out for a bit I think select_all_children would benefit a lot from only selecting children that are actually contained in the selection.

Hmm, but in this case, the command would basically never do anything when the selection is just a cursor, or really any time you don't happen to have an entire node contained in the selection.

But on the other hand, I could see this being useful for if you only want a subset of the children. I'll add a third command that does this.

@dead10ck
Copy link
Member Author

Just added select_all_children_in_selection that only selects if all resulting ranges are contained within the starting selection.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 30, 2023
@dead10ck dead10ck force-pushed the select-all-siblings branch 2 times, most recently from e52d4c5 to 9bc49a0 Compare July 19, 2023 03:26
@dead10ck dead10ck force-pushed the select-all-siblings branch from 9bc49a0 to 4086b3b Compare August 1, 2023 02:36
@dead10ck dead10ck force-pushed the select-all-siblings branch from 4086b3b to 9104939 Compare August 29, 2023 01:44
@pascalkuthe pascalkuthe added this to the next milestone Sep 1, 2023
@dead10ck dead10ck force-pushed the select-all-siblings branch from 98a9e9b to afd5c49 Compare October 24, 2023 22:20
@dead10ck dead10ck force-pushed the select-all-siblings branch from afd5c49 to bd6214e Compare November 20, 2023 12:30
@dead10ck dead10ck force-pushed the select-all-siblings branch from bd6214e to 1302f26 Compare January 27, 2024 01:19
@dead10ck dead10ck force-pushed the select-all-siblings branch from 1302f26 to b0ad2cd Compare February 29, 2024 14:51
@dead10ck dead10ck force-pushed the select-all-siblings branch 4 times, most recently from fef4927 to 24aeefe Compare April 1, 2024 23:45
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

This should be Adapted to cross tree sitter injection layer like the othrr motions now do, the rest lgtm

@@ -40,6 +41,85 @@ pub fn select_next_sibling(syntax: &Syntax, text: RopeSlice, selection: Selectio
})
}

fn find_parent_with_more_children(mut node: Node) -> Option<Node> {
Copy link
Member

Choose a reason for hiding this comment

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

I think thus also needs to be able to cross injection layer so I would use the new tree cursor here

helix-core/src/object.rs Outdated Show resolved Hide resolved
@dead10ck
Copy link
Member Author

dead10ck commented Apr 7, 2024

Sounds good, will do

@dead10ck dead10ck force-pushed the select-all-siblings branch from 24aeefe to 82d4c78 Compare April 7, 2024 19:38
@dead10ck
Copy link
Member Author

dead10ck commented Apr 7, 2024

Ok I've rebased and changed all the motions to use the new in-crate TreeCursor instead of the tree-sitter API directly. This required adding a child iterator in the TreeCursor module.

Comment on lines 244 to 247
let found = match self.named {
true => self.cursor.goto_first_named_child(),
false => self.cursor.goto_first_child(),
};
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
let found = match self.named {
true => self.cursor.goto_first_named_child(),
false => self.cursor.goto_first_child(),
};
let found = self.cursor.goto_first_child_impl(self.named);

Comment on lines 249 to 253
if !found {
return None;
}

return Some(self.cursor.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
if !found {
return None;
}
return Some(self.cursor.node());
return found.then_some(self.cursor.node());

Comment on lines 256 to 259
let found = match self.named {
true => self.cursor.goto_next_named_sibling(),
false => self.cursor.goto_next_sibling(),
};
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
let found = match self.named {
true => self.cursor.goto_next_named_sibling(),
false => self.cursor.goto_next_sibling(),
};
let found = if self.named {
self.cursor.goto_next_named_sibling()
} else
self.cursor.goto_next_sibling(),
};

I don't love matching on bools. Nothing really wrong with it but this is one of the "multiple ways to do the same things so always doing it the same way make code more consistent".

may be worth making named a parameter (with an _impl` function) so you don't need the if else at all.

Comment on lines 261 to 266
// subsequent iterations, keep visiting siblings until we run out
if found {
return Some(self.cursor.node());
}

None
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
// subsequent iterations, keep visiting siblings until we run out
if found {
return Some(self.cursor.node());
}
None
// subsequent iterations, keep visiting siblings until we run out
found.then_some(self.cursor.node())

helix-term/src/commands.rs Outdated Show resolved Hide resolved
Comment on lines 5007 to 5011
if selection.contains(&all_children) {
all_children
} else {
selection
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I am not sure this is quite what I had in mind here. If I can tell this right this would just not do anything as soon as any child is not in the selection. Instead I would like to see this be sort of like anl intersection. We should select the children covered by a selection.

In pseudo code all_chidren.retain(|range| selection.contains(range) (just better implemented so its not O(n^2).

If this is too complex just omit this and I think I can implement this when solving #3007

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, maybe I'll just leave it out for now

Comment on lines 78 to 80
if !children.is_empty() {
Some(children.into_iter())
} else {
None
}
Copy link
Member

Choose a reason for hiding this comment

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

Every callsite has a fallback so lets just inline that code here to avoid duplication

Suggested change
if !children.is_empty() {
Some(children.into_iter())
} else {
None
}
if children.is_empty() {
vec![fallback]
} else {
chidren
}

cursor: &'n mut TreeCursor<'n>,
text: RopeSlice,
direction: Direction,
) -> Option<<Vec<Range> as std::iter::IntoIterator>::IntoIter> {
Copy link
Member

Choose a reason for hiding this comment

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

I would just return a Vec and let the callsite call into_iter or if you really want to return an iterator use an anonymous impl Iterator instead this is a bit hard to read

@dead10ck dead10ck force-pushed the select-all-siblings branch from 7cdf1d4 to 77b7d94 Compare April 8, 2024 14:54
@dead10ck
Copy link
Member Author

dead10ck commented Apr 8, 2024

Good suggestions, I took care of them.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry for throwing a wrench into this with the TreeCursor changes - thanks for rebasing and improving the TreeCursor while you were at it

@the-mikedavis the-mikedavis merged commit c99c333 into helix-editor:master Apr 9, 2024
6 checks passed
@dead10ck dead10ck deleted the select-all-siblings branch April 9, 2024 15:06
@sudormrfbin
Copy link
Member

I can finally stop splitting by commas inside a function call to select the arguments :) But it seems the docs weren't updated with the new alt-a and alt-I keybinds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants