-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
textobjects for html/xml tags #3282
Conversation
Please do not change the behavior of If |
Yeah I don't see a compelling reason for a breaking change here: more languages have queryable test conventions than XML tags. We could use |
There are also the navigation commands helix/helix-term/src/keymap/default.rs Lines 100 to 121 in 0c08ff1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XML has been added so we could add queries for that as well (#4518), or leave that for a separate PR
runtime/queries/html/textobjects.scm
Outdated
(element | ||
(start_tag) | ||
(_)* @xml_element.inside | ||
(end_tag)? | ||
. ) @xml_element.around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some odd interaction here with the *
vs the ?
actually. I think we may need to split these into two separate stanzas so that the version with the end_tag
takes precedence:
(element | |
(start_tag) | |
(_)* @xml_element.inside | |
(end_tag)? | |
. ) @xml_element.around | |
(element | |
(start_tag) | |
(_)* @xml_element.inside | |
(end_tag) | |
. ) @xml_element.around | |
(element | |
(start_tag) | |
(_)* @xml_element.inside | |
. ) @xml_element.around |
Otherwise a snippet like:
<a href="print.html" title="Print this book" aria-label="Print this book">
<i id="print-button" class="fa fa-print"></i>
</a>
If the cursor is on the whitespace at the start of line 2, mie
will select the </a>
Thanks so much for this PR! I tested the changes (rebased the current main) and it works reliably for around-queries. For inside-queries it works when the cursor is already on the content nested in a tag. I would expect the element the cursor is in to be the context the selection is applied to: <section>
<p [CURSOR] class="sr-only"> text </p>
</section>
Currently only |
The <section>
<p [CURSOR] class="sr-only"> text </p>
</section> It would be ideal in the above case for Furthermore, i think it would be great if the Text selection is trimmed of whitespace by default but that should be part of another PR |
So in the end i had to modify how textobjects are captured from rust to capture the |
let mut second_cursor = QueryCursor::new(); | ||
|
||
// // This is helpful in capturing inner nodes in html, jsx and other xml like | ||
// // languages where it's diffucult to capture the *.inner textobjects. | ||
// // e.g.: <p[CURSOR] att="val">this is a <a>link</a></p> | ||
// // because of how treesitter s-exp work, when trying to capture the p inside of element | ||
// // we capture the whole p element instead. | ||
// // So, Here we Take the p element as the root node and then capture our inner element | ||
let node = if better_capture && textobject == TextObject::Inside { | ||
let capture_name = format!("{}.{}", object_name, TextObject::Around); // eg. function.inner | ||
let outer_node = lang_config | ||
.textobject_query()? | ||
.capture_nodes(&capture_name, slice_tree, slice, &mut cursor)? | ||
.filter(|node| node.byte_range().contains(&byte_pos)) | ||
.min_by_key(|node| node.byte_range().len())?; | ||
|
||
match outer_node { | ||
CapturedNode::Single(outer_node) => { | ||
let capture_name = format!("{}.{}", object_name, TextObject::Inside); // eg. function.inner | ||
lang_config | ||
.textobject_query()? | ||
.capture_nodes(&capture_name, outer_node, slice, &mut second_cursor)? | ||
.next()? | ||
} | ||
_ => outer_node, | ||
} | ||
} else { | ||
let capture_name = format!("{}.{}", object_name, textobject); // eg. function.inner | ||
lang_config | ||
.textobject_query()? | ||
.capture_nodes(&capture_name, slice_tree, slice, &mut cursor)? | ||
.filter(|node| node.byte_range().contains(&byte_pos)) | ||
.min_by_key(|node| node.byte_range().len())? | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the block comment explanation, however: There's now three almost identical format!
calls and three chains of lang_config.textobject_query()?.capture_nodes
. The // et. function.inner
does not feel correct and in general the intent is kind of hard to get a grasp on (reading the code only).
Maybe you can refactor it a bit DRYer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh i would love make it DRY but this my first time writing rust so I don't know if making this a Closure is the rust way of Doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I tried to extract that into a Closure, but i had issues returning an Iterators from the closure and then i had some lifetimes issues on top of that.
Extracting that to a separate function resulted in a similar lifetimes issue, which i don't understand
@@ -260,18 +260,47 @@ pub fn textobject_treesitter( | |||
object_name: &str, | |||
slice_tree: Node, | |||
lang_config: &LanguageConfiguration, | |||
better_capture: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should more closely express what's going on. Why would you ever not use the better_capture
? Maybe even use an enum here, to make the call site more explicit/readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's Temporary name, I thought someone reviewing the PR could suggest a Better name
closing this one out as stale, thank you for contributing! |
I bundled the html and jsx queries with this pr to show how it work in practice
mae
- will select the element and all it's child element, this is can also be used to select self closing element like we see in jsxmie
- will select the all the child nodes of an element