Make m textobject look for pairs enclosing selections

[EpocSquadron]

Right now, this textobject only looks for pairs that surround the cursor. This ensures that the pair found encloses each selection, which is likely to be intuitively what is expected of this textobject.

I'm marking this as a draft because I haven't adjusted the tests to expect the new behavior, and I'm not 100% sure all the edge cases are covered. I wanted to maybe get eyes on it to help me find them.

[the-mikedavis]

This behavior feels intuitive to me as you say 👍

I think the tests don't need any adjustment because there aren't cases yet that cover this behavior. It would probably be good to add one or two that have different behavior between this and master

[EpocSquadron]

I implemented a test and identified that it was missing some cases. In particular, if the start of the selection was before the opening pair it would reduce the selection down. I tried modifying the logic, but now in that case it simply fails to find a matching pair at all.. I've been banging my head against the desk for an hour, so I have to step away from this. Any help would be appreciated.

[A-Walrus]

The function you modified is find_nth_closest_pairs_pos. I believe it is not actually used by any of the tests. They use find_nth_pairs_pos. You can replace the code for find_nth_closest_pairs_pos with a panic!() and the test will still fail for the same reason, the assertions are different, and not because the function panicked (it was never called):

thread 'surround::test::test_find_nth_pairs_range' panicked at 'assertion failed: `(left == right)`
  left: `Ok((10, 15))`,
 right: `Ok((4, 21))`: Beginning range (9, 13), character '(', skip 1', helix-core/src/surround.rs:275:13

[EpocSquadron]

Thanks so much! My next hacking session on this might not be for a little while but I should be a lot more productive on it now

[the-mikedavis]

Can we split these tests out into helix-term/tests/test/movement.rs or at least add a few cases there? mam/mim could really use some overall tests and the tests should end up being easier to read because of the selection DSL

[EpocSquadron]

Can we split these tests out into helix-term/tests/test/movement.rs or at least add a few cases there? mam/mim could really use some overall tests and the tests should end up being easier to read because of the selection DSL

I could take a stab at that.

[EpocSquadron]

Tests have been translated over to the new integration system, but they're still not passing. Ran out of time to work through correctness issues with test cases. Will come back later.

[EpocSquadron]

There's an issue with the PR and counts that I need to address, and staring at the code some more I think there's some more simplification to do.

[EpocSquadron]

Tests all pass now, the trouble was with the skip logic.

There remains the pathological case of when a single character cursor is on the opening of a pair, but that was an issue prior to this refactor and should be left for a separate fix. The new tests should help a lot with such an endeavour.

[dead10ck]

The code looks good to me, thanks for the improvement! I'm assuming this code affects the "around" motions too. Would you be able to add ma tests that complement the mi tests?

[EpocSquadron]

I can do another pass on tests yeah.

[archseer]

Looks like the code fails some lints, can you run cargo fmt?

[EpocSquadron]

Looks like the code fails some lints, can you run cargo fmt?

Yes I can, also intended to write the ma tests I've just had a lot going on personally. Might be another week or two before I get time. I'm not opposed to a maintainer pushing to my branch however.

[EpocSquadron]

@archseer Added the mam equivalent tests, added multi-cursor tests, and added range direction preservation to the logic (with more tests). I can't think of much else to do here aside from that annoying bug where being on the opening brace skips out to the next pair. I've tried everything with regards to that and at this point I think it's better to address in a separate PR.

[EpocSquadron]

Looks like this partly resolves #4568

Reading through that it doesn't actually appear this PR addresses any of the several issues identified. Can you enlighten me?

, but it still fails with the cursor on an opening bracket. I think it'd be a good idea to add if is_opening_pair(text.char(pos)) before the loop in find_nth_closest_pairs_pos (removed error checking for conciseness).

At this point this PR has been sitting for a while. I'd rather see that issue with the opening bracket fixed in a follow-up. Feel free to work on that on your end.

Also I think it's a good idea to try and use the tree-sitter when available for getting pairs — that'd also allow mim and mam to work with strings and other language specific pairs. However, having a simpler algorithm to fall back to is still a good idea.

100%. When I started work on this the conceit was I would quickly resolve the non-tree-sitter version and then move on to working on the tree-sitter version. Didn't end up getting that far.

Do you want to work on all this yourself or should I do this myself?

I won't be able to spend a lot of time on helix contributions for a while, go for it!

[EpocSquadron]

@archseer any chance this makes it into the release?

[archseer]

Oh, sorry but I completely missed this PR. I'm only merging small changes or bugfixes this close to release, but it'll definitely make it into the next cycle!

[EpocSquadron]

Lint issue is unrelated to changes in this PR.

[gabydd]

Seems like a2ad2e6 deleted some indents rustfmt wants

[the-mikedavis]

Sorry for the delay, let's get this merged! I'm liking the new integration tests and the main change in find_nth_closest_pairs_pos looks good 👍