AndrewRadev / sideways.vim

A Vim plugin to move function arguments (and other delimited-by-something items) left and right.
http://www.vim.org/scripts/script.php?script_id=4171
MIT License
481 stars 9 forks source link

Shortcut to add new item (param/attribute) #40

Closed jdsutherland closed 3 years ago

jdsutherland commented 4 years ago

I think it's useful to have a shortcut to add a new item (for me, generally a new param or an xml-like attribute). e.g. (cursor at one):

dict = {█one: 1, two: 2, three: 3}

nnoremap ,aa :SidewaysJumpRight<cr>i<space><left>,<left> results in:

dict = {one: 1, █, two: 2, three: 3}

This generally works but has a few issues:

  1. It wraps around if at the tail item. e.g. (cursor on foo) const fn = (█foo) => results in const fn = (█, foo) => instead of const fn = (foo, █) =>

  2. Not everything uses , delimiters (e.g. html/css/jsx/cucumber tables/ocaml/probably others) A workaround for html/css/jsx would be to use a separate map that doesn't insert , like: nnoremap ,as :SidewaysJumpRight<cr>i<space><left>

For 1., one solution might be to add an option to disable wraparound.

For 2., I suspect it might be a bit of work to get a single command working across all supported filetypes so I'm not sure it's worth the effort compared to simply making additional mappings.

I wish vim allowed a with text objects.

Anyway, I just wanted to see if there is any interest or possibly find out if I'm overlooking something and it can be done more easily. Thanks for the great plugin.

jdsutherland commented 4 years ago

Another workaround involves using visual mode with the textobj, via https://www.reddit.com/r/vim/comments/1z93ww/append_to_a_text_object

omap aa <Plug>SidewaysArgumentTextobjA
xmap aa <Plug>SidewaysArgumentTextobjA
omap ia <Plug>SidewaysArgumentTextobjI
xmap ia <Plug>SidewaysArgumentTextobjI
nmap ,aa via<esc>a

dict = {█one: 1, two: 2, three: 3} ,aa results in dict = {one: 1█, two: 2, three: 3} then you enter the delimiter and the addition.

This requires 1 or 2 more keystrokes (delimiter and space or just space for xml-like) but has the benefit of one mapping. Perhaps this could be integrated inside the plugin using the delimiter definitions to avoid the additional keystrokes.

AndrewRadev commented 4 years ago

Thanks for the great plugin.

Thank you, I appreciate it :)

It's actually not hard to create a basic "add new item" mapping. My biggest problem is, honestly, what mapping to pick, my keyboard is running out of combinations.

I've created a basic implementation in the add-new-item branch. I would actually recommend you browse through the code, since the logic is relatively straightforward (for now): https://github.com/AndrewRadev/sideways.vim/compare/add-new-item#diff-b2c8768bc89b5faad1da6ae1eeb8bef8

You'll need to map your own favorite key sequence at the moment:

nmap ,ia <Plug>SidewaysArgumentInsert
nmap ,aa <Plug>SidewaysArgumentAppend

Note that there's two separate mappings -- "insert" will add space for a new item before the current one and "append" will add one after it. That removes ambiguity at the starts and ends of list, just like Vim manages entering insert mode at the two edges of a line. No problems with wrapping, either.

To decide which delimiter to use, I take the existing "delimiter pattern" from the definitions of the various lists, and replace any occurrence of optional whitespace with a single space. It's hacky, but it seems to work. If necessary, a separate key could be added to the g:sideways_definitions entries instead, but we'll see what I find during testing.

How it could fail is with a list like foo(1,2,3) -- if someone's coding style doesn't use whitespace after the comma, the added item will stand out. This might be solvable with a setting that could be global or buffer-local about the user's preference.

There could also be a problem with multiline items:

dict = {
  one:   1,
  two:   2,
  three: 3,
}

Adding a new item here would happen on the same line, rather than on a new one like the rest of the code. Maybe I could check if the end-line of the previous item and start-line of the next one are -1 and +1 of the current one, and if that's the case, substitute \_s* in the delimiter with a newline instead... But I think I need to put some more time and testing into it.

If you'd like, try it out for now. As it is, it doesn't affect the existing interface of the plugin, so I don't particularly mind adding it, and if I can think of good default mappings for it, I might get used to it as well :). I'll try to work on it some more, see how far I can take it. Let me know if you encounter issues or interesting cases, or have different idea for how to solve the problem.

jdsutherland commented 4 years ago

Wow, this looks great. Very nice of you to implement this.

My biggest problem is, honestly, what mapping to pick, my keyboard is running out of combinations.

You and me both.

Re multiline items: personally I would just use o–I don't see the advantage of using the mapping in this context.

Thanks! I will definitely be using the branch and will report back. It seems to be working well so far, for the most part. I've noticed a few issues:

  1. css when cursor on the value

    a { color: █#fff; background: blue; text-decoration: underline; }

    <Plug>SidewaysArgumentAppend results in:

    a { color: #fff\s█; background: blue; text-decoration: underline; }
  2. react with ft=javascriptreact doesn't work. It displays :call sideways#new_item#Add('a') without action. It does work if ft=javascript.jsx. I'm not sure what the current status of javascript.jsx vs javascriptreact is but from https://github.com/vim/vim/issues/4830, it sounds like Bram decided on javascriptreact so I don't know if that means javascript.jsx is dead. Does javascriptreact need added here? If you'd like, I can create a pr.

AndrewRadev commented 4 years ago

Yes, you have it right about the javascriptreact filetype -- I've added it in the right place, and it seems to work as it should now. I'm wondering if I should remove the "jsx" thing, but it probably won't hurt to keep it as a fallback.

I've also fixed the CSS issue.

Re multiline items: personally I would just use o –I don't see the advantage of using the mapping in this context.

I guess that's true. It would be nice to have a consistent method of opening a new argument, I guess, so you wouldn't have to think about whether or not it's a new line or an existing one, but I agree that intuition would probably suggest a simple o anyway.

I can still think of one potential benefit -- adding a new item at the end of a list when it doesn't have a trailing comma (some styleguides enforce no trailing comma). On that note, I wonder if it wouldn't also be useful to have analogues for I and A for these mappings, not just i and a. Well, something I'll think about, I guess.

jdsutherland commented 4 years ago

Having used it more, I find myself wishing I didn't have to use o. It seems less distracting to maintain that common interface, as it were.

I can still think of one potential benefit -- adding a new item at the end of a list when it doesn't have a trailing comma (some styleguides enforce no trailing comma)

Yeah, I just encountered this. Could definitely save a few keystrokes here.

AndrewRadev commented 4 years ago

Okay, I've created two more mappings for inserting at the beginning and appending at the end of a list (and I had to rename the existing ones). I've also added logic to add items on separate lines, but only if all the existing items so far are on separate lines. I think this is the best I can do in terms of guessing where to place the new item.

I'd appreciate if you keep using the new version and give me some more feedback. Here's the configuration I'll be trying out, though feel free to use different mappings:

nmap s,i <Plug>SidewaysArgumentInsertBefore
nmap s,a <Plug>SidewaysArgumentAppendAfter
nmap s,I <Plug>SidewaysArgumentInsertFirst
nmap s,A <Plug>SidewaysArgumentAppendLast
jdsutherland commented 4 years ago

This looks great. SidewaysArgumentAppendLast is a huge savings and will get a ton of use.

One thing I noticed (unsure if related to Sideways.vim) is when I use SidewaysArgumentInsert.. on newline delimited items, the cursor is not indented:

<div class="example"
     style="color: red;"
     ▋something="other"
  Example
</div>

<Plug>SidewaysArgumentInsertBefore

<div class="example"
     style="color: red;"
▋
     something="other"
  Example
</div>

Do you get similar behavior?

jdsutherland commented 4 years ago

Something that came up (maybe non-related and should go in separate issue) is a desire for Sideways to work for html class values:

<div id="app" class="min-h-screen bg-gray-200 antialiased xl:flex xl:flex-col xl:h-screen">

Similar to how https://github.com/rhysd/vim-textobj-anyblock uses the most narrow region for multiple matches (this might already be true for Sideways?), it'd be nice Sideways could work within a class attribute values when the cursor is inside, while continuing to work on the full attribute when outside.

So, within the string:

<div id="app" class="▋min-h-screen bg-gray-200 antialiased xl:flex xl:flex-col xl:h-screen">

<Plug>SidewaysArgumentAppendLast

<div id="app" class="min-h-screen bg-gray-200 antialiased xl:flex xl:flex-col xl:h-screen ▋">

And then outside in an attribute:

<div id="app" ▋class="min-h-screen bg-gray-200 antialiased xl:flex xl:flex-col xl:h-screen">

<Plug>SidewaysArgumentAppendLast

<div id="app" class="min-h-screen bg-gray-200 antialiased xl:flex xl:flex-col xl:h-screen" ▋>

Edit: I tried implementing this and started a draft PR: https://github.com/AndrewRadev/sideways.vim/pull/41. It works for <Plug>SidewaysArgumentAppend../Insert but :SidewaysLeft/Right aren't working and I'm not sure why.

Another consideration is limiting this to class. There are likely other attributes that would benefit from this but not all are delimited by space (e.g. style="color: red;"). I think class would get the most use in practice so it seemed like the best starting point.

AndrewRadev commented 4 years ago

One thing I noticed (unsure if related to Sideways.vim) is when I use SidewaysArgumentInsert.. on newline delimited items, the cursor is not indented:

Yeah, I saw some indentation issues, but they were tricky to pinpoint. One of the difficult things about extracting mapping implementations to functions, going in and out of insert mode is a bit weird. And using map <expr> has its own issues.

I do think I managed to fix this issue and pushed to the add-new-item branch. Let me know if you see anything else.

Something that came up (maybe non-related and should go in separate issue) is a desire for Sideways to work for html class values:

As mentioned in #41, this should be implemented now, I agree that it's a good idea to implement. The only thing that bothers me a bit is that it would also be useful for eruby and php and such, but I'll either have to extract the definitions with variables or maybe replicate them with changes. There's the (very likely) possibility of the items including <% %> tags or <? ?> ones. Maybe I'll just put <> down as brackets and be done with it, but I'll leave that for later.

AndrewRadev commented 4 years ago

@jdsutherland I've added some documentation to the add-new-item branch and I think it's about ready to release. Feature-wise, I think this is the best I can do with it, and if there are any bugs, we can solve them directly on master.

I'm thinking I'll merge it on Monday or so, but let me know if you have any objections.

jdsutherland commented 4 years ago

@AndrewRadev I've been using it a lot and haven't run into any other issues, so you have my full support!

AndrewRadev commented 4 years ago

Awesome, in that case, I'll just go ahead and merge it now, and if any new problems pop up, feel free to reopen this issue or create a new one.

AndrewRadev commented 4 years ago

I've added an optional setting for the feature to jump back to where the mapping was triggered when it's done: https://github.com/AndrewRadev/sideways.vim/blob/a0288868499fbffcff80c321867f8cefc9124e81/doc/sideways.txt#L332-L347

I made this for myself, because I somehow feel like whenever I want to add a new item like this, it's in the middle of something else and I'd like to get back after (otherwise, I'd just add the item manually). Maybe it's just me, who knows. In any case, the setting is there, if you'd like to try it out.

jdsutherland commented 4 years ago

Cool. I'm curious what context you're using this in; for me, the cursor is usually right next to where the addition is happening.

jdsutherland commented 3 years ago

I tried making it repeatable but it doesn't work:

command! SidewaysArgumentAppendAfter call sideways#new_item#Add('a') | silent! call repeat#set("\<Plug>SidewaysArgumentAppendAfter")
nnoremap <Plug>SidewaysArgumentAppendAfter :<c-u>SidewaysArgumentAppendAfter<cr>

I referenced the existing repeatable SidewaysLeft:

command! SidewaysLeft  call sideways#MoveLeft()  | silent! call repeat#set("\<Plug>SidewaysLeft")
nnoremap <silent> <Plug>SidewaysLeft  :<c-u>SidewaysLeft<cr>

Is it not as simple to make this repeatable? Is it because calling SidewaysArgumentAppendAfter and then entering text don't count as a single change in the way repeat works?

AndrewRadev commented 3 years ago

Is it because calling SidewaysArgumentAppendAfter and then entering text don't count as a single change in the way repeat works?

Yes, I think so. What the mapping does is essentially append a delimiter and leave the cursor in insert mode, but then it's done. That probably does get recorded for repetition, but then when you type something in and exit insert mode, that change overwrites it.

What could be done is to attach an event on insert-leave. Here's a proof-of-concept patch that seems to work specifically for appending an item:

diff --git autoload/sideways/new_item.vim autoload/sideways/new_item.vim
index 5179dd1..0f46975 100644
--- autoload/sideways/new_item.vim
+++ autoload/sideways/new_item.vim
@@ -102,6 +102,8 @@ function! s:InsertAfter(item, delimiter_string, new_line)
   else
     exe 'normal! a'.delimiter_string
     call feedkeys('a', 'n')
+
+    autocmd InsertLeavePre <buffer> ++once call repeat#set("\<Plug>SidewaysArgumentAppendAfter")
   endif

   if g:sideways_add_item_cursor_restore && has('textprop')

It's not ideal for various reasons, but it does seem to work (sort of, it gets messed up by one personal repeat-related mapping, but that's a very, very specific problem I'd have, and I can live with it).

I'll do some thinking about how to do it in a more sensible way, but I can't promise when that'll be. I can see how this could be very, very convenient.

AndrewRadev commented 3 years ago

@jdsutherland I made a branch, repeat-adding-new-item that seems to work. Try it out and let me know what you think.

jdsutherland commented 3 years ago

@AndrewRadev I'm using neovim and I see that has('nvim-0.4.0') returns early. Is this not workable with neovim then?

AndrewRadev commented 3 years ago

That's weird. I don't use neovim myself, but I have it installed at version 0.4.4 and has('nvim-0.4.0') returns true. Do you have an older version?

The reason I put that check is because I've got a plugin that tells me autocmd-once is available from nvim 0.4.0 onward. There might be some weirdness with that check, though. Could you try to comment out that entire if-clause, see what happens in your installation? https://github.com/AndrewRadev/sideways.vim/blob/761b4d737f9a43e6021bb80a02418ae20c205495/autoload/sideways/new_item.vim#L200-L203

If you don't have ++once, you should quickly see an error anyway. But it is true that it requires relatively recent versions of things. If it's a problem, I can hack around it, it'll just be a clumsier setup.

jdsutherland commented 3 years ago

Sorry, disregard the previous about the early return – false alarm.

It seems to work fine so far, but it's not quite what I imagined. Currently: const fn = (█a) => + SidewaysArgumentAppendAfter + (insert b), then const fn = (a, █b) + . results in: const fn = (a, b, █) (insert mode) I was hoping that repeat would enter the same input, e.g. const fn = (a, b, █b)

My main motivation for this is for refactoring add parameter. Right now I often use :grep then /PATTERN with cgn to do this in a repeatable way throughout the quickfix list, but it'd be easier with this. It would also be pretty nice in React, where it's common to add some state at a root component that can end up getting passed down through multiple components (requires appending the same line multiple times). It'd be nice to be able to repeat that with one key.

I'm not sure how viable/difficult implementing like that would be though.

Anyway, it would still be useful as is. Thank you for adding this!

AndrewRadev commented 3 years ago

Yes, what you say makes a lot of sense. It should be doable -- I'll try to implement it when I can.

AndrewRadev commented 3 years ago

It took some fiddling, but I think I got it to work on the branch -- could you try it out and let me know?

jdsutherland commented 3 years ago

It doesn't seem to work. Hopefully this screencap illustrates what's going on. I use neovim but I briefly tested in latest vim and got the same behavior.

https://user-images.githubusercontent.com/5385846/115966458-7f493380-a4e2-11eb-8309-ecdbf7af7fd9.mov

To be clear, the issues seems to be that:

  1. repeat just inserts the previous insert text, without adding the appropriate delimiter(s) const fn = (█a) => + SidewaysArgumentAppendAfter + (insert b), then const fn = (a, █b) + . results in: const fn = (a, bb)

  2. repeat on jsx/html attributes replaces the current line with the previous insert text rather than appending as a new line. I'm not sure if jsx/html attributes require additional logic as a special case or this issue will go away if 1) is resolved.

AndrewRadev commented 3 years ago

I think there's probably some kind of mismatch between our configurations -- there might be a config that changes behaviour in a way I hadn't expected. That example works fine on me on a Vim 8.2.2680 (which, admittedly, is very, very recent, so might be something in the versions, too). I did see an issue when trying to activate . after an undo, but that's not what's happening in the screencast, from what I can see.

Could you let me know the version on your Vim (for neovim I see it's 0.5.0) and share your .vimrc file?

jdsutherland commented 3 years ago

I just tested again using Vim 8.2.2800 with .vimrc:

call plug#begin('~/.vim/plugged')
Plug 'AndrewRadev/sideways.vim', { 'branch': 'repeat-adding-new-item' }
call plug#end()

Issue 1. seems to be the same as in screencast.

AndrewRadev commented 3 years ago

Could you double-check that you have repeat-vim installed? I tried it on neovim, version NVIM v0.5.0-dev+1285-g0150cc416 with the above config, and I was able to replicate the issue exactly, until I realized that it's missing vim-repeat, and adding it seems to work:

call plug#begin('~/.config/nvim/plugged')
Plug 'AndrewRadev/sideways.vim', { 'branch': 'repeat-adding-new-item' }
Plug 'tpope/vim-repeat'
call plug#end()

This is particularly for the "empty config" version -- there might still be some problem even with vim-repeat added, but please check this for now.

jdsutherland commented 3 years ago

Sorry, I indeed forgot to add vim-repeat in the minimal vimrc testing with vim. After adding vim-repeat, the issue appears resolved.

I definitely had vim-repeat installed with neovim (as in the screencast) though.

I just tried the successful minimal vimrc in neovim and the issue is resolved as well, so this suggests the problem lies in some other conflict in my configuration. I will do some bisecting and report back.

jdsutherland commented 3 years ago

I found the culpable line:

nnoremap ; :
nnoremap : ; " <== this line

~So apparently either sideways or vim-repeat is depending on default : not being remapped? Does this make sense to you?~ I think I added the map early on in my vim career and don't use it anyway (I use https://github.com/rhysd/clever-f.vim) so I'll simply remove it.

Edit: The issue stems from : in :call getting mapped: https://github.com/AndrewRadev/sideways.vim/blob/2a86162ac26d060cd7b12b0487b7b335f4ea7f6c/autoload/sideways/new_item.vim#L218

I realized this after changing it to nnoremap : / and seeing the error message: E486: Pattern not found: call sideways#new_item#Repeat("SidewaysArgumentAppendAfter", "foo")

Now that it's working, it seems great but I will continue to test and report back. This is going to be a killer feature. Thanks!

AndrewRadev commented 3 years ago

Oh, that's interesting. I really would like for this to work regardless of mapping, but I can't think of how I'd pull it off. I guess vim-repeat needs to respect mappings :/. Well, maybe I can leave a warning in the documentation and handle it later if it comes up, given you're okay with removing it.

jdsutherland commented 3 years ago

There's an issue here. After reading and trying a few things, I couldn't get it working with the remap. I'm not sure if these are my failures or the fixes referenced there aren't applicable to this context.

Well, maybe I can leave a warning in the documentation and handle it later if it comes up, given you're okay with removing it.

👍

jdsutherland commented 3 years ago

I've yet to run into any issues using this. I think it's good to go, unless you have concerns around the mapping issue.

AndrewRadev commented 3 years ago

Merged :+1:. I think there's nothing I can do on my side for the repeat issue. I'll rethink it if it ever comes up.