tpope / vim-repeat

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

Undo may open the wrong fold #80

Open lacygoill opened 4 years ago

lacygoill commented 4 years ago

Describe the bug

The undo command may open the wrong fold.

To Reproduce

Run this shell command (you'll need to update the path to the plugin):

vim -Nu NONE -S <(cat <<'EOF'
    set rtp^=/path/to/vim-repeat
    nno <c-b> xp:call repeat#set("\<lt>c-b>")<cr>
    let buf =<< trim END
        " fold A {{{1
        " some text
        " fold B {{{1
        " some text
    END
    %d
    pu!=buf
    setl fdm=marker
EOF
) /tmp/file

Result: The A fold is opened.

Expected behavior

The B fold is opened.

Screenshots

gif

Environment

Additional context

The issue is due to the zv command being executed too early: https://github.com/tpope/vim-repeat/blob/c947ad2b6a16983724a0153bdf7f66d7a80a32ca/autoload/repeat.vim#L135

The undo command is written in the typeahead via feedkeys() which doesn't execute it immediately. In contrast, zv is executed via :norm which does execute the command immediately. As a result, zv is executed before the undo command, while it should be executed after.

There are several solutions, but I think the simplest one is to just pass zv to feedkeys().


It's a regression introduced by https://github.com/tpope/vim-repeat/commit/1b82cad74cd5d5caf3c4092365cda54dc1fb853e.

lacygoill commented 4 years ago

There are several solutions, but I think the simplest one is to just pass zv to feedkeys().

Actually no, the simplest fix would be to use the t flag: https://github.com/tpope/vim-repeat/pull/81#issuecomment-627657697

Edit: t would cause the undo/redo command to be saved twice during a recording which is wrong. So, passing zv to feedkeys() is better.

Edit 2: Yes, but only on the condition that zv is really needed; that is if the cursor is in a closed fold; otherwise, we may prevent u, U, and C-r from printing some info about the undo state, as reported in #27. See this comment for more details.