mizlan / iswap.nvim

Interactively select and swap function arguments, list elements, and much more. Powered by tree-sitter.
MIT License
507 stars 22 forks source link

IMove, IMoveWith, and fixing a bug in ISwapWith #81

Closed IndianBoy42 closed 1 year ago

IndianBoy42 commented 1 year ago

For ISwapWith, when searching for the list node in treesitter, I've made it filter out if the cursor is in a list node (defined by the query) but the cursor is not in any of the named children. This makes ISwapWith work better with nested list as it would then instead find the outer list. It does create a minor inconsistency between ISwap and ISwapWith: the same cursor position may the inner list for ISwap but the outer list for ISwapWith. It doesn't make as much sense to do this filtering for ISwap. Also technically the "not in named children" condition also filters out the inner list if you're in a comment or whitespace inside the inner list.

Its not perfect but I think its better because previously ISwapWith would kinda break when used on the braces of a list

These two are in the same PR as cherry picking to separate them into branches didn't work. I can try to disentagle harder if you want

mizlan commented 1 year ago

By the way, technically there are multiple ways for it to work with the outer list:

  1. Using :ISwapNode to first select the first level of node. Within nested lists, this may be used to determine the depth of the list you'd like to swap with its sibling.
  2. Using :ISwap on the braces/whitespace in between outer list elements (not the comma though).
IndianBoy42 commented 1 year ago

Yeah I found that ISwap works but I always preferred the *With variants, feels more intuitive. And before this ISwapWith would end up in a glitched state if you tried to use it on the brace

mizlan commented 1 year ago

I need to think a bit more about the changing behavior of ISwapWith before merging this

These two are in the same PR as cherry picking to separate them into branches didn't work. I can try to disentagle harder if you want

Don't worry about it


I also wanted to consider adding highlights at different positions for IMove: highlight at start of node for nodes coming before cursor node, and highlight at end of node for nodes coming after cursor node, to better represent where the final element is going to go (going to be inserted to the side nearer to where the highlight is). Let me know if I should make the explanation of this better. but also I can add this feature on my own separately after this PR

IndianBoy42 commented 1 year ago

I think that labelling sounds fine. Like I said in the original comment, after using it the intuition I developed for the labels is not moving it after or before the node I select, rather than I'm thinking about moving it into the place of that node and everything else shuffles out of the way. Visually that's what it looks like to me. But I can also understand someone thinking about it as choosing a point in between existing nodes to slot the node into. Maybe a configuration option?

mizlan commented 1 year ago

Yeah, don't worry about that, I might write it up if I feel like it

mizlan commented 1 year ago

This is just about ready to be merged, I am very happy about these features :)

Thank you for taking all this time to understand the codebase and add these features, it means a lot to me

IndianBoy42 commented 1 year ago

Thanks a lot, its no problem, plugin programming is a really fun free time thing for me

mizlan commented 1 year ago

Is this ready to merge?

IndianBoy42 commented 1 year ago

I think so, i've been using it for the last 2 weeks and I don't think i've found anything broken