zed icon indicating copy to clipboard operation
zed copied to clipboard

Add new `argument` vim text object

Open vultix opened this issue 1 year ago • 6 comments

This PR adds a new argument vim text object, inspired by targets.vim.

As it's the first vim text object to use the syntax tree, it needed to operate on the Buffer level, not the MultiBuffer level, then map the buffer coordinates to DisplayPoint as necessary.

This required two main changes:

  1. innermost_enclosing_bracket_ranges and enclosing_bracket_ranges were moved into Buffer. The MultiBuffer implementations were updated to map to/from these.
  2. MultiBuffer::excerpt_containing was made public, returning a new MultiBufferExcerpt type that contains a reference to the excerpt and methods for mapping to/from Buffer and MultiBuffer offsets and ranges.

Release Notes:

  • Added new argument vim text object, inspired by targets.vim.

vultix avatar Feb 14 '24 20:02 vultix

We require contributors to sign our Contributor License Agreement, and we don't have @vultix on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

cla-bot[bot] avatar Feb 14 '24 20:02 cla-bot[bot]

@cla-bot check

vultix avatar Feb 14 '24 20:02 vultix

The cla-bot has been summoned, and re-checked this pull request!

cla-bot[bot] avatar Feb 14 '24 20:02 cla-bot[bot]

@vultix thanks for taking a pass at this! If you'd like to work with me to get this over the line feel free to book time: https://calendly.com/conradirwin/pairing.

Otherwise, I'll leave comments and you can address in your own time.

ConradIrwin avatar Feb 14 '24 21:02 ConradIrwin

  • I'm not sure about a better way to filter out strings, @maxbrunsfeld may know if we can use the tree-sitter stuff (it also seems like some languages have their comment characters defined as brackets).
  • For , as a separator, this seems like a reasonable place to start; we may also want ; too for things like C-style for loops, but I'm happy either way.
  • I think it's right to use the syntax-tree and not characters to do this (though it can lead to some disappointment if the language isn't supported; when the language is supported it works much better).
  • To get the right buffer, you can use text_anchor_for_position on the multi buffer. I'm less sure about the number of syntax layers. Looking at syntax_ancestor_for_node (option-up) it iterates through them all; that said, I can't imagine you'd likely need to handle more than one for just a function argument.

ConradIrwin avatar Feb 14 '24 21:02 ConradIrwin

@ConradIrwin I believe this is ready for review now. It ended up being a larger change than I was hoping, but I'm very happy with the simplifications MultiBufferExcerpt brings, both to existing and new code.

I'd be happy to hop on a paired programming session if you have questions/comments. Thanks!

vultix avatar Feb 23 '24 23:02 vultix

@vultix Thanks for the cleanup! I also like the MultiBufferExcerpt abstraction.

I am going to merge this as is, but if you'd like to take a pass at implementing it without the syntax tree instead, I'm open to that. It seems like you understand the trade-off much better than I do (I am mostly worried about edge-cases, but sounds like this approach is no panacea).

The worse of both worlds seems to be to have both (you called out implementation complexity as a problem with this approach, and I'm also worried about that if we do manual character iteration), but if there are advantages in user experience it's possible that that's the right answer.

Always happy to pair on this, or the Point/DisplayPoint/Row proposal if you'd like to get that started.

ConradIrwin avatar Feb 24 '24 02:02 ConradIrwin