tpope / vim-repeat

repeat.vim: enable repeating supported plugin maps with "."
http://www.vim.org/scripts/script.php?script_id=2136
2.59k stars 81 forks source link

Go back to using feedkeys(), but restore typeahead #11

Closed AndrewRadev closed 11 years ago

AndrewRadev commented 11 years ago

Since there seems to be a bug with "norm" in Vim prior to 7.3.100, better go back to using feedkeys.

To make that work with a custom mapping, the rest of the typeahead buffer is consumed and then fed back.

Update: Since the typeahead restoration is fairly convoluted, just go back to using feedkeys().

inkarkat commented 11 years ago

In general, I don't like the stuff you need to introduce for this, but I acknowledge your need.

I wasn't involved in the original discussion; what I don't understand from that is: Wouldn't a simple modification of

:nnoremap <silent> . :<C-U>call repeat#run(v:count)<CR>

to

:nnoremap <silent> . :<C-U>call repeat#run(v:count)<Bar>call feedkeys('`]', 'n')<CR>

already address your need (without modifying repeat.vim except for making the function public)?!

AndrewRadev commented 11 years ago

Interestingly enough, it doesn't work for me. I hadn't really thought about that and it seems like it should do the trick, but for some reason, it doesn't.

My test harness is a simple text file with:

"foo"
"bar"

And I use surround.vim to ds" on the first word and then . to delete the quotes around the second word. The mapping I use is:

runtime autoload/repeat.vim
nnoremap . :call repeat#run(v:count)<cr>`]

With this mapping, the cursor is positioned as it were after deleting the quotes around the second word. Changing it to your suggestion doesn't.

To experiment with this, I tried mapping m to these:

nmap m :call feedkeys('')<bar>ls<cr>
nmap m :call feedkeys('j')<bar>ls<cr>
nmap m :call feedkeys('j')<bar>throw "OK"<cr>

The first one shows a list of buffers, while the second one doesn't. The third one, however, throws an error. This makes sense, since the feedkeys() call adds the keys to be executed after the current mapping is done (because it's entirely one mapping), and so the second mapping's ls is cleared due to the following j and the third mapping's j is never executed due to the exception.

I'm not sure how that works out with the repeat.vim mapping. It seems to me like the feedkeys(]) is executed first, but that doesn't look right to me. If anyone has a better idea of what's going on, please let me know.

inkarkat commented 11 years ago

Did you try this with the original or the latest repeat.vim? I used the original (working around the script-local function via :call <SNR>042_repeat), and a plugin similar to surround.vim, and it worked flawlessly.

AndrewRadev commented 11 years ago

Huh, you're right, it does work. Before, I tried it with the newer version, I just removed the typeahead stuff. It seems like I've missed something. Either way, I could use that to get the mapping to work.

That said, I'm not very happy about it. It works like a charm, but to me, the fact that feedkeys() is being used as an implementation should not be important to anyone that wants to wrap repeat calls with anything else. I'd much rather go with the typeahead storage and restoring to make it straightforward to anyone else to map their own repeats. Of course, I realize I'm currently the only one (along with a friend of mine) with a use case like that, so I don't know how much this matters to other users.

In the end, it's @tpope's call. If the pull request is rejected, I'll just use your workaround. Thanks for that, by the way, quite obvious in hindsight :).

inkarkat commented 11 years ago

Vim does not allow simple wrapping of mappings; you're already relying on repeat.vim implementation details (like the function name that you had to expose, and the fact that this isn't a :map-expr). Should @tpope some time in the future decide to drop old Vim support and switch to :normal, your feedkeys() addition will continue to work, anyway.

Your typeahead storage is a sufficiently special trick that I would hesitate to include this, especially since the two of you already had a less-than-stellar record of preventing regressions.

tpope commented 11 years ago

I think it probably makes sense to go back to the old battle tested implementation.

AndrewRadev commented 11 years ago

Reasonable decision, I guess :). I've rebased commits and I've left just one that restores feedkeys(), but leaves the repeat#run an autoloaded function.

tpope commented 11 years ago

Thanks.