Include treesitter objects in select mode

[atagen]

Previously, when using bracket gotos with select mode enabled, treesitter object-based options would ignore select mode completely.

This patch implements select mode behaviour consistent with paragraph bracket gotos for treesitter objects.

[the-mikedavis]

I gave this a try and I don't see it being consistent with v]p in all cases. For example on the functions right after this edit:

#[|fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}]#

fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}

v]f changes to

fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}#[
|]#
fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}

instead of

#[|fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}

fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}]#

(where #[|]# marks a selection with | being the cursor).

[atagen]

Thanks for taking a look at this.
I've fixed the error you've pointed out above in my most recent commit.

However, I've run into a bigger issue - when our cursor is at the start of a function block, I can't determine what the user intended from the available information in goto_ts_object_impl's context (or any method I can find for Range / Selection / etc so far).

Specifically, when the range faces backwards and user selects forwards this could mean they are extending a first selection further forward (which the code currently handles correctly), or that they are selecting backwards, and want to deselect the most recently encountered block (which presently breaks and acts as if it were the first selection).

Insofar as the directions of the range and new range and concerned these two cases look the same, and without actively tracking selected treesitter objects (or at least having their selections placed in the recent action buffer) I'm currently drawing a blank on how to differentiate them further.

If any obvious solution pokes its head out at you reading this, please advise, otherwise I think I have more or less discovered why this feature didn't already exist.. :)

[the-mikedavis]

Actually I think my comment may have been wrong: those extensions were correct based on the head and anchor of the selection range. The overall behavior is a bit surprising though and I think it's because ]f puts the cursor at the beginning of the selection:

#[|fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}]#

where ]p puts the cursor at the end of the selection. So in order to have consistent behavior, I think we should change movement::goto_treesitter_object to respect the direction passed in rather than always have the selection backwards.

@sudormrfbin I see you in the blame for

helix/helix-core/src/movement.rs

Lines 436 to 437 in c107f4e

	// head of range should be at beginning
	Some(Range::new(end_char, start_char))

What do you think about having the tree-sitter motions not always return the selection backwards?

[atagen]

I considered this, and came to a similar conclusion about treesitter selects always putting the cursor first.

I did test it earlier without touching goto_treesitter_object by merely flipping new_range when looking backwards (and changing the corresponding references to head/anchor in the selection).

While I agree with fixing them to match paragraph selects in respecting the direction they were made in, it seems the edge case can still take place quite easily by selecting several function blocks backwards and then selecting one forwards (as one might to deselect). This again gives us no way to differentiate from an initial forward selection and extension - so I fear there's still going to be a missing piece of context for implementing this.

[atagen]

to use your earlier example:

#[|fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}

fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}

fn goto_next_class(cx: &mut Context) {
    goto_ts_object_impl(cx, "class", Direction::Forward)]
}]#

v]p from here would give us #[| at the start of goto_prev_function, and you would expect the same from v]f, but instead we end up with:

#[fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}

fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}|]#

fn goto_next_class(cx: &mut Context) {
    goto_ts_object_impl(cx, "class", Direction::Forward)
}

because it looks identical to us, context wise (ie. based on directional comparison between old and new ranges), to when we use ]fv]f in the first instance to begin selecting.

Hopefully this makes what I'm saying about backwards then forwards selecting a bit clearer.

[sudormrfbin]

@sudormrfbin I see you in the blame for

helix/helix-core/src/movement.rs

Lines 436 to 437 in c107f4e

	// head of range should be at beginning
	Some(Range::new(end_char, start_char))

What do you think about having the tree-sitter motions not always return the selection backwards?

I wrote it that way with the intention of making it easier to see (for example in the function object) the function declaration rather than the last statement in the function definition. Now that I think about it, the current behavior of next/prev treesitter object is very different from next/prev word, paragraph, etc:

• Selections always has the head at the beginning of the object (the problem mentioned above).
• w, ]p, etc select from the current head to the next word or paragraph, while ]f always select the whole next function. This for example makes it impossible to select from the current position to the end of the function.

If these two inconsistencies can be fixed, I think the select mode treesitter variants can be implemented easily without the edge cases similar to how select mode w, ]p, etc are done.

[atagen]

I have implemented the goto_treesitter_object movement changes as discussed above, and cleaned up goto_ts_object_impl.

As far as I can tell from testing, this patch now works properly and treesitter select should be semantically in line with other select modes 😄

Please let me know if you find anything else out of the ordinary.

[the-mikedavis]

Sweet this is looking pretty good!

There's one case I think is a bit awkward:

#[fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}

fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}|]#

going backward (v[f) will bring the cursor to the head of the current function:

#[fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}

|]#fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}

It's easy to work around this by trimming whitespace from the selection with _ but I think I would expect

#[fn goto_next_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Forward)
}|]#

fn goto_prev_function(cx: &mut Context) {
    goto_ts_object_impl(cx, "function", Direction::Backward)
}

instead. What do you think?

[atagen]

Looking into this now, I'm not sure there is a better way than auto filtering whitespace for this specific movement - seems like TS will plough on accruing whitespace in its current node until it encounters something it can decide is a new one. I'll see what I come up with.

[atagen]

I've added the whitespace clipping - it definitely feels cleaner for function deselections. There is a side effect that deselecting backward from a closure will now leave your cursor totally clear of the closure (ie. on the paren or comma preceding it in the function call). This doesn't feel too bad, though, and as tradeoffs go I'd rather take the whitespace skip.

[the-mikedavis]

This behavior is feeling right 👍

[the-mikedavis]

The whitespace trimming feels a little hacky - I suspect movement::goto_treesitter_object is returning the current object when we're trying to go to the previous one:

1 | #[fn goto_next_function(cx: &mut Context) {
2 |     goto_ts_object_impl(cx, "function", Direction::Forward)
3 | }
4 | 
5 | fn goto_prev_function(cx: &mut Context) {
6 |     goto_ts_object_impl(cx, "function", Direction::Backward)
7 | }|]#

I think in this case movement::goto_treesitter_object for v[f should point to the function ending on L3 rather than the beginning of the function on L5

[atagen]

The whitespace trimming feels a little hacky - I suspect movement::goto_treesitter_object is returning the current object when we're trying to go to the previous one:

1 | #[fn goto_next_function(cx: &mut Context) {
2 |     goto_ts_object_impl(cx, "function", Direction::Forward)
3 | }
4 | 
5 | fn goto_prev_function(cx: &mut Context) {
6 |     goto_ts_object_impl(cx, "function", Direction::Backward)
7 | }|]#

I think in this case movement::goto_treesitter_object for v[f should point to the function ending on L3 rather than the beginning of the function on L5

hm, is that not the behaviour you're getting? that's precisely what happened for me when I just tested it now, and what I intended to implement..

edit: oh, are you suggesting clipping the whitespace from the position goto_treesitter_object returns to get to the end of the previous block is itself a hack? that's.. probably true, it wants to go to the start of the current block, not the end of the previous one. unfortunately, I wasn't sure how to replace that max pick inside goto_treesitter_object with something resembling "second max". that would fix it properly, but possibly still needs special casing, and as I mentioned that second-previous node also contains all that trailing whitespace (so will land us below the end of the second function, between the two).

[atagen]

Testing my assumptions, I collected the filtered nodes to a Vec, sorted by key, and popped the last one before returning the next last - this leaves us in the whitespace before the just-deselected function, as suspected. Popping again puts us at the top of the previous function, effectively deselecting both in one movement.

There's no way around treesitter including whitespace at the end of the item either, as you'll see from the issue I linked earlier (it's suggested to simply consult the original buffer using the TS node's start and end bytes). Hence, without diving in and filtering the whitespace as we have, I'm at a loss how to achieve the desired behaviour for Helix.

[the-mikedavis]

I think that's because of the direction of the range returned by movement::goto_treesitter_object: it jumps to the beginning of the prior function because we're using the returned range's head rather than anchor

[atagen]

It seems that no amount of manipulation I throw at the treesitter movement can make this work in a straightforward fashion.. actually, all I found is that we probably need to trim whitespace forward as well :)

At this point, I'll step back unless the whitespace skip is deemed acceptable or some other route is found.

[the-mikedavis]

I've picked this up in #3266