Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jdsutherland opened this issue Jul 31, 2020 · 32 comments
Closed

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

jdsutherland opened this issue Jul 31, 2020 · 32 comments

Comments

@jdsutherland
Copy link
Contributor

jdsutherland commented Jul 31, 2020

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 jdsutherland changed the title Shortcut to add an argument Shortcut to add new item (param) Jul 31, 2020
@jdsutherland jdsutherland changed the title Shortcut to add new item (param) Shortcut to add new item (param/attribute) Jul 31, 2020
@jdsutherland
Copy link
Contributor Author

jdsutherland commented Aug 1, 2020

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
Copy link
Owner

AndrewRadev commented Aug 1, 2020

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
Copy link
Contributor Author

jdsutherland commented Aug 2, 2020

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; }
  1. 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 Adding javascriptreact / typescriptreact filetypes vim/vim#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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

AndrewRadev commented Aug 23, 2020

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
Copy link
Contributor Author

jdsutherland commented Aug 23, 2020

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
Copy link
Contributor Author

jdsutherland commented Aug 23, 2020

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: #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
Copy link
Owner

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
Copy link
Owner

@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
Copy link
Contributor Author

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

@AndrewRadev
Copy link
Owner

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
Copy link
Owner

I've added an optional setting for the feature to jump back to where the mapping was triggered when it's done:

*g:sideways_add_item_cursor_restore*
>
let g:sideways_add_item_cursor_restore = 1
<
Default value: 0
If you set this variable to 1 and your vim has the |+textprop| feature, the
plugin will jump back to where you triggered the mapping when you leave insert
mode. By default, it's turned off, since it might be surprising. It could be
useful if you decide to add a new item while doing something else and would
like to get back to it after.
Note, however, that this relies on the |InsertLeave| autocommand, so if you
exit insert mode via Ctrl+C (which doesn't trigger it), it won't jump until
the next time you leave insert mode normally. If exiting via Ctrl+C is a part
of your workflow, it's recommended you keep this setting off.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

jdsutherland commented Mar 17, 2021

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
Copy link
Owner

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 AndrewRadev reopened this Mar 17, 2021
@AndrewRadev
Copy link
Owner

AndrewRadev commented Apr 14, 2021

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

@jdsutherland
Copy link
Contributor Author

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

@AndrewRadev
Copy link
Owner

AndrewRadev commented Apr 14, 2021

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?

if !(has('patch-8.1.1113') || has('nvim-0.4.0'))
" then ++once is not available
return
endif

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
Copy link
Contributor Author

jdsutherland commented Apr 15, 2021

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
Copy link
Owner

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

@AndrewRadev
Copy link
Owner

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
Copy link
Contributor Author

jdsutherland commented Apr 24, 2021

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.

Screen.Recording.2021-04-24.at.9.42.25.AM.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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

jdsutherland commented May 1, 2021

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
Copy link
Contributor Author

jdsutherland commented May 1, 2021

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:

let repeat_invocation = ":call sideways#new_item#Repeat(\"" . a:action . "\", \"". escape(last_insert, '"\') . "\")\<cr>"

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants