'.' Doesn't Repeat Custom Text Objects Before Version 7.4

[lag13]

Hello! First off, great plugin. I don't often need to use it but when I do it's really nice to have. Get's me another step closer to having my editor do exactly what I'm thinking. So thanks!

The Issue

I think the title says it all. On vim before version 7.4, running the exchange mapping on a custom text object doesn't repeat correctly. The problem is that prior to version 7.4, using an operator with a custom text object was not automatically repeatable. To make a text-object repeatable, you needed to do some extra work utilizing repeat.vim. See https://github.com/wellle/targets.vim/blob/master/autoload/targets.vim#L498-L507 for an example. Since your code calls repeat#invalidate() it prevents this from working. I know the repeat#invalidate() code was put in for a reason in PR #32 but I think we can add support for versions prior to 7.4 and keep the existing functionality. I have two possible solutions to propose. If you or @wellle or @wilywampa could take a look (since you were all involved in that PR) that would be much appreciated.

Solution 1

https://github.com/tommcdo/vim-exchange/compare/tommcdo:master...lag13:improve-repeat-compatibility1?expand=1

Solution 2

https://github.com/tommcdo/vim-exchange/compare/tommcdo:master...lag13:improve-repeat-compatibility2?expand=1

How/Why This works

To make custom text objects repeatable, vim-repeat registers a one-time CursorMoved handler which sets g:repeat_tick equal to b:changedtick. Apparently this works because the CursorMoved event will only trigger after the operator + operator-pending mappings have completed. The reason your operator won't repeat correctly with a custom text object is because calling repeat#invalidate() will clear the CursorMoved autocommand before it gets a chance to do it's thing. So my solutions work because they make b:changedtick and g:repeat_tick differ (which wipes out a previously used repeat#set() call) but they leave the CursorMoved autocommand (which will make the text object repeatble). Kudos to guns for finding out how to make custom text objects repeatable in the first place https://github.com/tpope/vim-repeat/issues/8.

[wilywampa]

Of the proposed solutions I prefer the first because it touches less stuff, but I'm wondering if the version check should go in vim-repeat instead of vim-exchange.

[lag13]

@wilywampa I like the first one better too. I made the second because I didn't know if it was considered bad practice or not to reference a variable used internally by a plugin. But then again, it's a global variable so I guess it should be fair game. And actually I think the second solution could fail in one case so maybe we should just disregard it.

Where specifically would the version check go in repeat.vim? Would it go in the repeat#invalidate() function?

[wilywampa]

Yes. Add a version check around the autocmd! repeat_custom_motion here: https://github.com/tpope/vim-repeat/blob/master/autoload/repeat.vim#L58

[lag13]

So after that change repeat#invalidate() would now look like this correct?

function! repeat#invalidate()
    if v:version >= 704
        autocmd! repeat_custom_motion
    endif
    let g:repeat_tick = -1
endfunction

Yes that would also work, but for some reason that makes me wary. Because it seems that the purpose of repeat#invalidate() (just from reading the comments) is to completely wipe the previously set repeat, but making this change wouldn't make that the case anymore. What do you think?

[wilywampa]

After playing with all three plugins and other custom motions for awhile, my opinion is that it should go into targets.vim. If the change went into vim-repeat, it would break repeating operations that don't increase b:changedtick. If it went into vim-exchange, it would fix repeating of targets.vim motions, but not repetition of other custom motions because targets.vim is already doing specific things to support 7.3 which most (all?) other custom motions do not do. Since this change would only benefit targets.vim, that's where I think it belongs. That would look like this:

" set up repeat.vim for older Vim versions
function! s:prepareRepeat(delimiter, which, modifier)
    if v:version >= 704 " skip recent versions
        return
    endif

    if v:operator ==# 'y' && match(&cpoptions, 'y') ==# -1 " skip yank unless set up
        return
    endif

    let cmd = v:operator . a:modifier
    if a:which !=# 'c'
        let cmd .= a:which
    endif
    let cmd .= a:delimiter
    if v:operator ==# 'c'
        let cmd .= "\<C-r>.\<ESC>"
    endif

    silent! call repeat#set(cmd, v:count)

    " Set up our own autocmd so a targets.vim motion can repeat after
    " repeat#invalidate() is called
    augroup repeat_targets_motion
        autocmd!
        autocmd CursorMoved <buffer> let g:repeat_tick = b:changedtick | autocmd! repeat_targets_motion
    augroup END
endfunction

[lag13]

My opinion would still be to put this change in exchange.vim (so my solution 1). I think equivalent code to targets.vim's s:prepareRepeat() function is all that should be needed to make a text object repeatable. The reason the CursorMoved autocommand was incorporated into repeat.vim in the first place was so custom text objects would not have to do it themselves and so re-writing that code feels wrong to me.

If it went into vim-exchange, it would fix repeating of targets.vim motions, but not repetition of other custom motions because targets.vim is already doing specific things to support 7.3 which most (all?) other custom motions do not do.

I'm not exactly sure what you mean here. Exchange.vim shouldn't really be concerned with fixing other custom motions. It is the custom motion's responsibility to make itself repeatable on older versions. But exchange is a unique operator and should realize that by calling repeat#invalidate() it wipes out any possibility that custom text objects are repeatable.

In short I feel like there is a "standard" way to make a text object repeatable using repeat.vim and altering that "standard" just to make exchange.vim work feels wrong. If we instead updated exchange.vim then every custom text object which adhered to that "standard" would just work.

[lag13]

And I just realized that maybe my first solution was a bit overzealous. I think we could get the same desired result by just replacing the line:

silent! call repeat#invalidate()

with:

let g:repeat_tick = -1

So no need for a version check or anything like that.

[wilywampa]

Anything but that, please. That defeats the purpose of the autocmd! repeat_custom_motion in vim-repeat's invalidate() and breaks functionality. If you use <Plug>whatever which calls repeat#set(), then do the first half of vim-exchange, then move the cursor and press ., <Plug>whatever is repeated instead of doing the second half of vim-exchange (unless you happened to use a custom text object which called repeat#set() which targets does if and only if you have Vim <7.4 but is otherwise uncommon).

[tommcdo]

Let me clarify something: repeat.vim is not the standard way to make operators repeatable; that happens naturally in Vim. Only non-standard custom operators require hooking into repeat.vim. The unique behavior of exchange.vim (namely, using the operator twice for a single operation) is not an example of such non-standard behavior. In fact, exchange.vim doesn't even need repeat.vim in order to be repeatable. The very existence of repeat.vim is the only reason exchange.vim needs to integrate with it. If repeat.vim never existed, exchange.vim would repeat perfectly -- even with (standard) custom text objects.

It kind of sounds like I'm ranting about how much I hate repeat.vim, but that's not the case at all. It's vital for non-standard operators. An example of a plugin that requires repeat.vim is surround.vim. The operators from that plugin take additional information after the motion: the character or text to surround the motion with. They use repeat.vim to replay that additional information when the . command is used. Otherwise, you'd be prompted to type the surround character again. Another plugin that needs repeat.vim is my own lion.vim.

Anyway, I'm currently unsure about adding this workaround to exchange.vim. You originally said that exchange.vim doesn't work with custom text objects prior to 7.4, but now that I know you're talking about targets.vim, I'm reluctant to believe that claim. Similar to how only non-standard custom operators (surround.vim, lion.vim) need repeat.vim, only non-standard custom text objects need it as well. Standard text objects basically just select a range of text, but targets.vim does a lot more and needs to register additional information to be repeated.

So I ask you: In Vim 7.3, can exchange.vim repeat standard custom text objects? Try it out with vim-dentures, which defines a text object for lines with a common indentation level. At present, I'm too lazy to find a box with Vim 7.3 on it to test this myself.

[tommcdo]

I'm curious about whether Solution 1 breaks the example @wilywampa mentioned under Vim 7.3.

Maybe what we really need is for repeat.vim to use separate handling for operator repeating vs text object repeating.

---

@tpope, thoughts? TL;DR exchange.vim works by invoking the operator twice. It doesn't need repeat.vim support, but calls repeat#invalidate() on the first invocation to avoid . repeating whatever custom operator previously called repeat#set(). However, if it was a text object (such as targets.vim) that called repeat#set(), all is lost. This apparently only affects Vim 7.3.

[lag13]

Anything but that, please. That defeats the purpose of the autocmd! repeat_custom_motion in vim-repeat's invalidate() and breaks functionality. If you use whatever which calls repeat#set(), then do the first half of vim-exchange, then move the cursor and press ., whatever is repeated instead of doing the second half of vim-exchange

I don't think that is necessarily true. Maybe I haven't given it enough thought but I don't think that we have to worry too much about the CursorMoved autocommand hanging around after a mapping finishes. In my short bursts of testing it seems that the CursorMoved autocommand always gets triggered when the mapping completes. For example, I added some echo's to the repeat#set() function like this:

function! repeat#set(sequence,...)
    let g:repeat_sequence = a:sequence
    let g:repeat_count = a:0 ? a:1 : v:count
    let g:repeat_tick = b:changedtick
    augroup repeat_custom_motion
        autocmd!
        autocmd CursorMoved <buffer> let g:repeat_tick = b:changedtick | echom "CursorMoved was triggered" | autocmd! repeat_custom_motion
    augroup END
    echom "Repeat was set"
endfunction

And then I created a file with these contents:

function! Test()
    copy .
    call repeat#set('t')
endfunction
nnoremap t :call Test()<CR>

Running that 't' mapping results in this output:

Repeat was set
CursorMoved was triggered

So by the time the mapping finishes, the CursorMoved autocommand was already triggered and destroyed. So the only thing to worry about is getting rid of g:repeat_tick.

[lag13]

Anyway, I'm currently unsure about adding this workaround to exchange.vim. You originally said that exchange.vim doesn't work with custom text objects prior to 7.4, but now that I know you're talking about targets.vim, I'm reluctant to believe that claim. Similar to how only non-standard custom operators (surround.vim, lion.vim) need repeat.vim, only non-standard custom text objects need it as well. Standard text objects basically just select a range of text, but targets.vim does a lot more and needs to register additional information to be repeated.

I'm not necessarily just talking about targets.vim. I just used that as an example of a plugin that defines some text objects. Prior to 7.4 any text object you created using vimscript would not be automatically repeatable. Instead, running the dot command after executing an operator+your-text-object would run that operator on the same length of text that was previously operated on. For instance, if you had a "number" text object (which I do) 'id' and this code:

123
12345

If you ran did on the number 123 it would delete 123. But then if you moved your cursor to the 1 in 12345 and ran '.' it would delete only 3 characters because that was the number of characters operated on previously. This would result in 45 when you would want the whole number to be deleted. I just tested this and that is what happened.

Couldn't run vim-dentures on 7.3 :) failed for some reason. But I do use another indent text object plugin https://github.com/michaeljsmith/vim-indent-object and got similar results as my number test above. Namely running dii on the first indented 12345 line deleted that line (as expected). But running '.' on the next pair of indented 12345's only deleted one line.

12345
    12345
12345
    12345
    12345
12345

[wilywampa]

I don't think that is necessarily true. Maybe I haven't given it enough thought but I don't think that we have to worry too much about the CursorMoved autocommand hanging around after a mapping finishes.

It's definitely true. I just double-checked in Vim 7.3 with no patches and 7.4.777. Simple example:

" Repeatedly ascend directory structure by pressing '.'
nnoremap <silent> <Leader>.. :<C-u>cd ..<bar>echo getcwd()<bar>silent! call repeat#set("\<Leader>..")<CR>

Now go to any file (but not on the last line) and type <Leader>..cxxj.. With no changes to any of the plugins, it exchanges the two lines as expected. If g:repeat_tick is set to -1 without canceling the autocmd, it will cd up a directory instead of exchanging the lines.

[lag13]

Ahh shoot. Really sorry about all that back and forth then. Curse you counter example!!! (shakes fist angrily). I suppose my original question still stands though. I do realize that my request is probably not high on anyone's todo list seeing as how 7.4 came out about 2 years ago, but I've run into this issue a handful of times and thought it might be nice to get resolved. Part of me still wants to say the fix should go in this plugin (because of that "standard" speech I gave), but it's starting to not seem all that possible. Regardless if this issue gets closed or not, thanks for taking the time to respond to me.

By the way, I know of only one other plugin (not that I've been actively looking) which calls on repeat.vim to make it's text object repeatable: https://github.com/justinmk/vim-sneak/blob/master/plugin/sneak.vim#L215-L218

[tommcdo]

The more I think about it, the more I agree that if a fix is to be implemented, it should probably be here. From what I know, no other operator works the way the exchange does (invoking twice), so hoping for a fix in repeat.vim is probably asking too much.

That said, I'm not totally sold on the proposed solution, and even less sold on whether this fairly small edge case on Vim 7.3 is worth the fuss I'm expecting an acceptable solution to be.

Slightly tangential curiosity... Do repeat-integrated operators play nicely with repeat-integrated text objects? Combining surround.vim and targets.vim, for example: is ysin)] repeatable? I'm guessing at the targets.vim mappings; my memory is that in) means "inside the next set of parentheses". Thus this command should put square brackets inside the next set of round parentheses.

[lag13]

That said, I'm not totally sold on the proposed solution, and even less sold on whether this fairly small edge case on Vim 7.3 is worth the fuss I'm expecting an acceptable solution to be.

I agree. This is has been a lot trickier than I originally envisioned. Disregard any of my proposed ideas because they do not work in all cases.

Slightly tangential curiosity... Do repeat-integrated operators play nicely with repeat-integrated text objects? Combining surround.vim and targets.vim, for example: is ysin)] repeatable? I'm guessing at the targets.vim mappings; my memory is that in) means "inside the next set of parentheses". Thus this command should put square brackets inside the next set of round parentheses.

Haha nice catch. I've never had cause to do it and so I never thought about it. No is the answer, it does not repeat. The first repeat#set() gets called in the text-object and then when it's finished repeat#set() gets called in the operator which effectively wipes out the previous repeat#set() invocation just like calling repeat#invalidate() does. Perhaps that example is a good motivation to come up with separate repeat handling for text objects and everything else as you suggested.

[wellle]

For the record, it does work in Vim 7.4.640:

a (b) c (d) e

ysin)].

a ([b]) c ([d]) e

[lag13]

Sorry, should have clarified that doing ysin)] doesn't repeat prior to 7.4. After 7.4 it's all good.

[tommcdo]

At this point it kind of feels like Vim 7.3 is a lost cause. If even basic text objects (ones that don't require additional input) don't repeat properly, I see no good reason to try to support more complex ones.

Since exchange.vim's call to repeat#invalidate() is not causing any trouble in Vim 7.4, then I think we should just leave it as is.

[lag13]

I agree. And the more I think about it, the more I think you might have been right when you said:

Maybe what we really need is for repeat.vim to use separate handling for operator repeating vs text object repeating.

Because the problem seems to be that exchange.vim has no way of knowing whether a previous invocation of repeat#set() came from a text-object or from some other random mapping. If it could make this distinction then it could take appropriate action. Perhaps such a change to repeat.vim could also end up making commands like ysin)] repeatable on older versions of vim. But who knows.

Anyway, thanks again to all who took the time to respond. Feel free to close this issue.

[wellle]

Even though I didn't really participate, this was quite interesting to follow. Thanks for the healthy discussion :+1: