k-takata / minpac

A minimal package manager for Vim 8+ (and Neovim)
835 stars 30 forks source link

buffer-local mappings not executed until timeout #132

Closed lacygoill closed 3 years ago

lacygoill commented 3 years ago

Describe the bug

When using a buffer-local mapping installed by minpac, we might need to wait until the mapping timeout.

To Reproduce

Run this shell command:

vim -Nu NONE -S <(cat <<'EOF'
    vim9script
    set pp=/tmp/.vim rtp=/tmp/.vim
    delete('/tmp/.vim', 'rf')
    system('git clone https://github.com/k-takata/minpac.git /tmp/.vim/pack/minpac/opt/minpac')
    packadd minpac
    minpac#init()
    minpac#add('k-takata/minpac', {type: 'opt'})
    nno sa <nop>
    set timeout timeoutlen=5000
    minpac#update()
EOF
)

Wait for minpac#update() to finish, then press s to open the status window.

We need to wait 5 seconds before the status window is opened.

Expected behavior

The status window is opened immediately.

Environment

Additional context

The issue comes from the user mapping sa which causes Vim to wait until the timeout to decide whether s is part of a longer mapping, or should be processed alone.

I think all <buffer> mappings should be defined with the <nowait> argument. As a suggestion here is a patch which should fix the issue:

diff --git a/autoload/minpac/progress.vim b/autoload/minpac/progress.vim
index 86fc093..dd48e6f 100644
--- a/autoload/minpac/progress.vim
+++ b/autoload/minpac/progress.vim
@@ -67,8 +67,8 @@ function! s:syntax() abort
 endfunction

 function! s:mappings() abort
-  nnoremap <silent><buffer> q :q<CR>
-  nnoremap <silent><buffer> s :call minpac#status()<CR>
+  nnoremap <silent><buffer><nowait> q :q<CR>
+  nnoremap <silent><buffer><nowait> s :call minpac#status()<CR>
 endfunction

 " vim: set ts=8 sw=2 et:
diff --git a/autoload/minpac/status.vim b/autoload/minpac/status.vim
index 8da3132..ac06b74 100644
--- a/autoload/minpac/status.vim
+++ b/autoload/minpac/status.vim
@@ -138,10 +138,10 @@ function! s:syntax() abort
 endfunction

 function! s:mappings() abort
-  nnoremap <silent><buffer> <CR> :call <SID>openSha()<CR>
-  nnoremap <silent><buffer> q :q<CR>
-  nnoremap <silent><buffer> <C-j> :call <SID>nextPackage()<CR>
-  nnoremap <silent><buffer> <C-k> :call <SID>prevPackage()<CR>
+  nnoremap <silent><buffer><nowait> <CR> :call <SID>openSha()<CR>
+  nnoremap <silent><buffer><nowait> q :q<CR>
+  nnoremap <silent><buffer><nowait> <C-j> :call <SID>nextPackage()<CR>
+  nnoremap <silent><buffer><nowait> <C-k> :call <SID>prevPackage()<CR>
 endfunction

 function! s:nextPackage() abort
@@ -175,7 +175,7 @@ function! s:openSha() abort
   call append(1, l:sha_content[1])
   1delete _
   setlocal nomodifiable
-  nnoremap <silent><buffer> q :q<CR>
+  nnoremap <silent><buffer><nowait> q :q<CR>
 endfunction

 function! s:find_name_by_sha(sha) abort
lacygoill commented 3 years ago

There is also a small typo in the README file:

diff --git a/README.md b/README.md
index f97e524..5783a06 100644
--- a/README.md
+++ b/README.md
@@ -370,7 +370,7 @@ If `"NONE"` is specified, package directories are listed instead of plugin direc

 `{plugname}` specifies a plugin name. Wildcards can be used. If omitted or an empty string is specified, `"*"` is used.

-If `{nameonly}` is TRUE, plugin (or package) names are listed instead of the direcotries. Default is FALSE.
+If `{nameonly}` is TRUE, plugin (or package) names are listed instead of the directories. Default is FALSE.

 E.g.:
k-takata commented 3 years ago

I haven't fixed the <nowait> issue yet. Is it okay?

lacygoill commented 3 years ago

Well, I think it's subjective. Some people might disagree and think that their global mappings should have the priority over local ones. I really don't know what's best. I have had experiences where I was surprised to see that the local mappings were not executed immediately. But I also had opposite experiences, where I was surprised to see that my global mappings were not executed, because of local ones.

In the end, I still think that local ones should have the priority over global ones. It leads to the less surprising situations. That is, I think that – on average – it takes more time to understand why a mapping is not executed immediately, than to understand why Vim executes an unexpected command in a "special" buffer (i.e. a buffer tied to a plugin).

Again, that's just my opinion. I don't have data to back this up.