Using % with selection=exclusive

[rparenzs]

I just started looking at this plugin but ran into a question right away:

If selection=exclusive and you start visual mode with the cursor on '(' of the string '(test)', then hit '%', the cursor goes to the closing ')' but only the text to the character before it is selected, '(test', as per the behavior of selection=exclusive. What you would really want is for the cursor to go one more position so that the closing ')' is also selected. This is in fact what the built-in '%' does, if you try it with 'unmap %'.

I am probably missing something here, but wanted to be sure. Thanks.

[chrisbra]

Yeah you are right. This case has not been considered yet. I think I fixed, please verify. Thanks.

[rparenzs]

Just a quick test so far, but it does not seem to handle the case where the right parenthesis is at the end of the line. In exclusive, the cursor should go past the end of the line one position in order to select it.

[chrisbra]

yeah, I wondered what should happen in that case. I suppose, one needs to set virtualedit to onemore. This might have other sideeffects however, so I was resistent to add it.

[rparenzs]

Ugh, that's a tricky special case. I didn't realize selection=exclusive would be a pain. Yeah, leaving 've' changed afterwards is not ideal, but I guess there's nothing for it. I was trying to think about a workaround, like something to post-fix it (map, autocmd OptionSet,, etc), but it didn't work.

[rparenzs]

By the way, another problem. The fix only needs to happen in visual mode (when you are selecting something). Right now it is happening in plain normal mode when you hit %, i.e., both going right one extra position and setting ve=onemore.

Also, maybe the changing 've' part would only happen when it has to, like when you are at the eol, not every time.

[chrisbra]

thanks, adjusted

[rparenzs]

I was checking out the fix, and it still does the wrong thing in normal mode. Can you maybe move the visual mode check up one "if"? That seems to work.

if &selection isnot# 'inclusive' && a:mode == 'v' if !(col('.') < col('$')-1)

vs current:

if &selection isnot# 'inclusive' if !(col('.') < col('$')-1) && a:mode == 'v'

[chrisbra]

it seemed to work for me, but I have changed it now

[rparenzs]

Thanks. Just to be sure after your worksforme comment, I double-checked both before (still fails) and after your last change (ok), and it works now. The case I was talking about was the plain one when you were not at the eol and just hit % without selecting: This is (another) test. Before, if you hit % on '(' it would incorrectly put the cursor on the space before 'test'. Also, if you went backwards and hit % on the closing paren ')', it incorrectly put the cursor on 'a' of another.

[rparenzs]

Sorry to revisit an old issue, but on the small chance you're interested, I did find a (kinda messy) way to keep 'set ve=onemore' from leaking into the user environment, which was a side effect of your fix.

The gist is that instead of moving the cursor to the eol by using 'onemore', you can set a mark to it (in the function). As it is now, after you exit the function, you set a mark and go virtual with> m'gv``

Instead, after you exit the function, you check if the mark has been set to eol; if so, just skip setting the mark again. The> gv`` will now take it past the last char.

(Edit: Deleted files, posted diff below)

[chrisbra]

thanks. Can you please post the diff somewhere? Else I'll have a look at your uploaded files, but a diff would make my work easier.

[rparenzs]

Of course, sorry about that. I wasn't sure it would make a difference. I assume you mean in the comment here. Let me figure out how to do this (my 'diff.exe' is from 2004, so may look old) ...

autoload\matchit.vim

@@ -217,7 +217,7 @@
     " move cursor one pos to the right, because selection is not inclusive
     " add virtualedit=onemore, to make it work even when the match ends the " line
     if !(col('.') < col('$')-1)
-      set ve=onemore
+      let eolmark = 1      " flag to set a mark on eol (since we cannot move there)
     endif
     norm! l
   endif
@@ -228,6 +228,11 @@
   if sp_return > 0
     execute final_position
   endif
+  if exists('eolmark') && eolmark
+    call setpos("''", [0, line('.'), col('$'), 0]) " set mark on the eol
+  else
+    call setpos("''", [0, line('.'), 1, 0])        " else set mark to anything else
+  endif
   return s:CleanUp(restore_options, a:mode, startpos, mid.'\|'.fin)
 endfun

plugin\matchit.vim

@@ -48,7 +48,8 @@

 nnoremap <silent> <Plug>(MatchitNormalForward)     :<C-U>call matchit#Match_wrapper('',1,'n')<CR>
 nnoremap <silent> <Plug>(MatchitNormalBackward)    :<C-U>call matchit#Match_wrapper('',0,'n')<CR>
-xnoremap <silent> <Plug>(MatchitVisualForward)     :<C-U>call matchit#Match_wrapper('',1,'v')<CR>m'gv``
+xnoremap <silent> <Plug>(MatchitVisualForward)     :<C-U>call matchit#Match_wrapper('',1,'v')<CR>
+      \:if col("''") != col("$") \| call execute("normal! m'") \| endif<cr>gv``
 xnoremap <silent> <Plug>(MatchitVisualBackward)    :<C-U>call matchit#Match_wrapper('',0,'v')<CR>m'gv``
 onoremap <silent> <Plug>(MatchitOperationForward)  :<C-U>call matchit#Match_wrapper('',1,'o')<CR>
 onoremap <silent> <Plug>(MatchitOperationBackward) :<C-U>call matchit#Match_wrapper('',0,'o')<CR>

[lacygoill]

The case I was talking about was the plain one when you were not at the eol and just hit % without selecting: This is (another) test. Before, if you hit % on '(' it would incorrectly put the cursor on the space before 'test'. Also, if you went backwards and hit % on the closing paren ')', it incorrectly put the cursor on 'a' of another.# inconsistent handling of test and [ when testing output of command

AFAICT, that's still broken:

vim -Nu NONE -i NONE -S <(cat <<'EOF'
    vim9script
    set packpath=/tmp/.vim runtimepath=/tmp/.vim
    delete('/tmp/.vim', 'rf')
    mkdir('/tmp/.vim/pack/matchit/opt/matchit', 'p')
    system('git clone https://github.com/chrisbra/matchit /tmp/.vim/pack/matchit/opt/matchit')
    &selection = 'exclusive'
    packadd matchit
    'This is (another) test.'->setline(1)
    feedkeys('f(v%')
EOF
)

The cursor is still on the space after the closing paren.

vim -Nu NONE -i NONE -S <(cat <<'EOF'
    vim9script
    set packpath=/tmp/.vim runtimepath=/tmp/.vim
    delete('/tmp/.vim', 'rf')
    mkdir('/tmp/.vim/pack/matchit/opt/matchit', 'p')
    system('git clone https://github.com/chrisbra/matchit /tmp/.vim/pack/matchit/opt/matchit')
    &selection = 'exclusive'
    packadd matchit
    'This is (another) test.'->setline(1)
    feedkeys('f)v%')
EOF
)

The cursor is still on the a after the opening paren.

---

One day, I would like to fix https://github.com/chrisbra/matchit/issues/29.

The problem is that the plugin code is hard to understand. That starts with the mappings. It looks weird to press some keys after the function call:

xnoremap <silent> <Plug>(MatchitVisualBackward)    :<C-U>call matchit#Match_wrapper('',0,'v')<CR>m'gv``
                                                                                                 ^----^

Is it really necessary? Is that because of the :return at the end of matchit#Match_wrapper()? It's useless anyway. s:CleanUp() doesn't return any value, and even if it did, the mapping doesn't care about what matchit#Match_wrapper() returns. And since we're at the end of the function, it will return no matter what; with or without :return. I think it should be replaced with a simple :call because it would be less confusing.

It also looks weird to have a whole if block after the mapping:

xnoremap <silent> <Plug>(MatchitVisualForward)     :<C-U>call matchit#Match_wrapper('',1,'v')<CR>
      \:if col("''") != col("$") \| exe ":normal! m'" \| endif<cr>gv``
      ^----------------------------------------------------------^

Besides, it can make other issues even more confusing.

For a start, how about we put all the logic back inside the function, and leave the mappings alone?

diff --git a/runtime/pack/dist/opt/matchit/autoload/matchit.vim b/runtime/pack/dist/opt/matchit/autoload/matchit.vim
index eafb7c055..3241d40b6 100644
--- a/runtime/pack/dist/opt/matchit/autoload/matchit.vim
+++ b/runtime/pack/dist/opt/matchit/autoload/matchit.vim
@@ -241,7 +241,13 @@ function matchit#Match_wrapper(word, forward, mode) range
   if exists('eolmark') && eolmark
     call setpos("''", [0, line('.'), col('$'), 0]) " set mark on the eol
   endif
-  return s:CleanUp(restore_options, a:mode, startpos, mid .. '\|' .. fin)
+  call s:CleanUp(restore_options, a:mode, startpos, mid .. '\|' .. fin)
+  if a:mode ==# 'v'
+    if !a:forward || col("''") != col('$')
+      normal! m'
+    endif
+    normal! gv``
+  endif
 endfun

 " Restore options and do some special handling for Operator-pending mode.
diff --git a/runtime/pack/dist/opt/matchit/plugin/matchit.vim b/runtime/pack/dist/opt/matchit/plugin/matchit.vim
index 51ba3a7f5..7cb6031cf 100644
--- a/runtime/pack/dist/opt/matchit/plugin/matchit.vim
+++ b/runtime/pack/dist/opt/matchit/plugin/matchit.vim
@@ -49,16 +49,15 @@ set cpo&vim
 nnoremap <silent> <Plug>(MatchitNormalForward)     :<C-U>call matchit#Match_wrapper('',1,'n')<CR>
 nnoremap <silent> <Plug>(MatchitNormalBackward)    :<C-U>call matchit#Match_wrapper('',0,'n')<CR>
 xnoremap <silent> <Plug>(MatchitVisualForward)     :<C-U>call matchit#Match_wrapper('',1,'v')<CR>
-      \:if col("''") != col("$") \| exe ":normal! m'" \| endif<cr>gv``
-xnoremap <silent> <Plug>(MatchitVisualBackward)    :<C-U>call matchit#Match_wrapper('',0,'v')<CR>m'gv``
+xnoremap <silent> <Plug>(MatchitVisualBackward)    :<C-U>call matchit#Match_wrapper('',0,'v')<CR>
 onoremap <silent> <Plug>(MatchitOperationForward)  :<C-U>call matchit#Match_wrapper('',1,'o')<CR>
 onoremap <silent> <Plug>(MatchitOperationBackward) :<C-U>call matchit#Match_wrapper('',0,'o')<CR>

 " Analogues of [{ and ]} using matching patterns:
 nnoremap <silent> <Plug>(MatchitNormalMultiBackward)    :<C-U>call matchit#MultiMatch("bW", "n")<CR>
 nnoremap <silent> <Plug>(MatchitNormalMultiForward)     :<C-U>call matchit#MultiMatch("W",  "n")<CR>
-xnoremap <silent> <Plug>(MatchitVisualMultiBackward)    :<C-U>call matchit#MultiMatch("bW", "n")<CR>m'gv``
-xnoremap <silent> <Plug>(MatchitVisualMultiForward)     :<C-U>call matchit#MultiMatch("W",  "n")<CR>m'gv``
+xnoremap <silent> <Plug>(MatchitVisualMultiBackward)    :<C-U>call matchit#MultiMatch("bW", "n")<CR>
+xnoremap <silent> <Plug>(MatchitVisualMultiForward)     :<C-U>call matchit#MultiMatch("W",  "n")<CR>
 onoremap <silent> <Plug>(MatchitOperationMultiBackward) :<C-U>call matchit#MultiMatch("bW", "o")<CR>
 onoremap <silent> <Plug>(MatchitOperationMultiForward)  :<C-U>call matchit#MultiMatch("W",  "o")<CR>

Can someone test, and report whether that causes issues?

---

Also, there are 2 mappings which look wrong, because they set the mode argument to n (which stands for normal mode), while they are installed in visual mode. Shouldn't it be v instead?

diff --git a/runtime/pack/dist/opt/matchit/plugin/matchit.vim b/runtime/pack/dist/opt/matchit/plugin/matchit.vim
index 51ba3a7f5..41d590b07 100644
--- a/runtime/pack/dist/opt/matchit/plugin/matchit.vim
+++ b/runtime/pack/dist/opt/matchit/plugin/matchit.vim
@@ -57,8 +57,8 @@ onoremap <silent> <Plug>(MatchitOperationBackward) :<C-U>call matchit#Match_wrap
 " Analogues of [{ and ]} using matching patterns:
 nnoremap <silent> <Plug>(MatchitNormalMultiBackward)    :<C-U>call matchit#MultiMatch("bW", "n")<CR>
 nnoremap <silent> <Plug>(MatchitNormalMultiForward)     :<C-U>call matchit#MultiMatch("W",  "n")<CR>
-xnoremap <silent> <Plug>(MatchitVisualMultiBackward)    :<C-U>call matchit#MultiMatch("bW", "n")<CR>m'gv``
-xnoremap <silent> <Plug>(MatchitVisualMultiForward)     :<C-U>call matchit#MultiMatch("W",  "n")<CR>m'gv``
+xnoremap <silent> <Plug>(MatchitVisualMultiBackward)    :<C-U>call matchit#MultiMatch("bW", "v")<CR>m'gv``
+xnoremap <silent> <Plug>(MatchitVisualMultiForward)     :<C-U>call matchit#MultiMatch("W",  "v")<CR>m'gv``
 onoremap <silent> <Plug>(MatchitOperationMultiBackward) :<C-U>call matchit#MultiMatch("bW", "o")<CR>
 onoremap <silent> <Plug>(MatchitOperationMultiForward)  :<C-U>call matchit#MultiMatch("W",  "o")<CR>

[lacygoill]

AFAICT, that's still broken:

Ah you were talking about normal mode, not visual mode.