garbas / vim-snipmate

snipMate.vim aims to be a concise vim script that implements some of TextMate's snippets features in Vim.
www.vim.org/scripts/script.php?script_id=2540
2.01k stars 181 forks source link

Default choice in TAB-stop selection is not the first entry #304

Closed yann-morin-1998 closed 5 months ago

yann-morin-1998 commented 5 months ago

When a TAB-stop contains a selection, the default item to be selected in the popup menu is the second entry, not the first.

For example,this snippet (notice the d-c ordering on purpose):

snippet foo
    Foo ${1|a|b|d|c} bar

would yield the situation below:

foo<TAB>
Foo b bar
  | a |
  |<b>|
  | d |
  | c |

When we should expect:

foo<TAB>
Foo a bar
  |<a>|
  | b |
  | d |
  | c |

Note that the entries are (correctly!) not re-ordered, as the d-c ordering proves.

yann-morin-1998 commented 5 months ago

To be noted: when I tested the completion branch for #265, the behaviour was the expected one: the first entry in the list was selected by default.

I unfortunately no longer have the rebased branch I did test on, so I can't check the diff...

yann-morin-1998 commented 5 months ago

To be noted: when I tested the completion branch for #265, the behaviour was the expected one: the first entry in the list was selected by default.

I unfortunately no longer have the rebased branch I did test on, so I can't check the diff...

Thanks to the reflog, I could get ahold of the code I tested, and I could reproduce the expected behaviour, where the first item does get selected by default.

I don't understand "vim-lang", and though the diff is a bit large, here it is (--- is the code that works, +++ is master):

diff --git a/autoload/snipMate.vim b/autoload/snipMate.vim
index bd0276f..d50256e 100644
--- a/autoload/snipMate.vim
+++ b/autoload/snipMate.vim
@@ -106,7 +106,7 @@ function! s:insert_snippet_text(snippet, lnum, col, indent)
 endfunction

 function! snipMate#placeholder_str(num, stops) abort
-   return snipMate#sniplist_str(a:stops[a:num].placeholder, a:stops)
+   return snipMate#sniplist_str(get(get(a:stops, a:num, {}), 'placeholder', []), a:stops)
 endfunction

 function! snipMate#sniplist_str(snippet, stops) abort
@@ -176,7 +176,7 @@ function! s:build_loc_info(snippet, stops, lnum, col, seen_items) abort
            call s:add_update_objects(stub, seen_items)

            " if we've found a stop?
-           if len(item) > 2 && type(item[1]) != type({})
+           if len(item) > 2 && type(item[1]) != type({}) && !exists('stub.items')
                let col = s:build_loc_info(item[1:-2], stops, lnum, col, seen_items)
            else
                let col += len(snipMate#placeholder_str(id, stops))
diff --git a/autoload/snipmate/jumping.vim b/autoload/snipmate/jumping.vim
index 2b8085a..a1297e5 100644
--- a/autoload/snipmate/jumping.vim
+++ b/autoload/snipmate/jumping.vim
@@ -46,7 +46,11 @@ function! s:state_set_stop(backwards) dict abort
    call cursor(self.cur_stop.line, self.cur_stop.col)
    let self.prev_len    = col('$')
    let self.changed = 0
-   let ret = self.select_word()
+   if exists("self.cur_stop.items")
+       let ret = self.select_item()
+   else
+       let ret = self.select_word()
+   endif
    if (self.stop_no == 0 || self.stop_no == self.stop_count - 1) && !a:backwards
        call self.remove()
    endif
@@ -67,6 +71,13 @@ function! s:state_jump_stop(backwards) dict abort
        unlet! self.cur_stop.placeholder " avoid type error for old parsing version
        let self.cur_stop.placeholder = [strpart(getline('.'),
                    \ self.start_col - 1, self.end_col - self.start_col)]
+
+       " Remove selection items if the stop has changed and the new placeholder
+       " is not one of the selection items
+       if exists('self.cur_stop.items') &&
+                   \ !count(self.cur_stop.items, self.cur_stop.placeholder)
+           call remove(self.cur_stop, 'items')
+       endif
    endif

    return self.set_stop(a:backwards)
@@ -151,30 +162,14 @@ function! s:state_update_mirrors(change) dict abort
            let oldSize = strlen(newWord)
        endif

-       " Split the line into three parts: the mirror, what's before it, and
-       " what's after it. Then combine them using the new mirror string.
-       " Subtract one to go from column index to byte index
-
-       let theline = getline(mirror.line)
-
-       " part before the current mirror
-       let beginline  = strpart(theline, 0, mirror.col - 1)
-
        " current mirror transformation, and save size
        let wordMirror= substitute(newWord, get(mirror, 'pat', ''), get(mirror, 'sub', ''), get(mirror, 'flags', ''))
        let mirror.oldSize = strlen(wordMirror)

-       " end of the line, use the oldSize because with the transformation,
-       " the size of the mirror can be different from those of the snippet
-       let endline    = strpart(theline, mirror.col + oldSize -1)
-
-       " Update other object on the line
+       " Update other objects on the line
        call self.update(mirror, changeLen, mirror.oldSize - oldSize)

-       " reconstruct the line
-       let update = beginline.wordMirror.endline
-
-       call setline(mirror.line, update)
+       call s:set_line(mirror.line, mirror.col, oldSize, wordMirror)
    endfor

    " Reposition the cursor in case a var updates on the same line but before
@@ -194,7 +189,9 @@ function! s:state_find_update_objects(item) dict abort
            call add(item.update_objects, stop)
        endif

+       let placeholder_len = len(snipMate#sniplist_str(stop.placeholder, b:snip_state.stops))
        for mirror in get(stop, 'mirrors', [])
+           let mirror.oldSize = placeholder_len
            if mirror.line == item.line && mirror.col > item.col
                call add(item.update_objects, mirror)
            endif
@@ -222,9 +219,29 @@ function! s:state_update(item, change_len, mirror_change) dict abort
    endfor
 endfunction

+" Split the line into three parts: the mirror, what's before it, and
+" what's after it. Then combine them using the new mirror string.
+" Subtract one to go from column index to byte index
+function! s:set_line(line, col, len, word)
+   let theline = getline(a:line)
+   let begin = strpart(theline, 0, a:col - 1)
+   let end = strpart(theline, a:col + a:len - 1)
+   call setline(a:line, begin . a:word . end)
+endfunction
+
+function! s:state_select_item() dict abort
+   let items = map(copy(self.cur_stop.items), 'snipMate#sniplist_str(v:val, b:snip_state.stops)')
+   call s:set_line(line('.'), self.start_col, self.end_col - self.start_col, '')
+   call complete(self.start_col, items)
+   for i in range(index(self.cur_stop.items, self.cur_stop.placeholder) + 1)
+       call feedkeys("\<C-N>")
+   endfor
+   return ''
+endfunction
+
 call extend(s:state_proto, snipmate#util#add_methods(s:sfile(), 'state',
            \ [ 'remove', 'set_stop', 'jump_stop', 'remove_nested',
-           \ 'select_word', 'update_changes', 'update_mirrors',
+           \ 'select_word', 'update_changes', 'update_mirrors', 'select_item',
            \ 'find_next_stop', 'find_update_objects', 'update' ]), 'error')

 " vim:noet:sw=4:ts=4:ft=vim
diff --git a/autoload/snipmate/parse.vim b/autoload/snipmate/parse.vim
index 4cf60e9..017c454 100644
--- a/autoload/snipmate/parse.vim
+++ b/autoload/snipmate/parse.vim
@@ -78,11 +78,32 @@ function! s:parser_varend() dict abort
         call extend(ret, self.placeholder())
     elseif self.same('/')
         call add(ret, self.subst())
+    elseif self.next == '|'
+        call add(ret, self.select())
     endif
     call self.same('}')
     return ret
 endfunction

+function! s:parser_select() dict abort
+    let items = []
+    while self.same('|')
+        let str = self.text('|}')
+        call s:mark_vars_in_select(str)
+        call add(items, str)
+    endwhile
+    return ['select'] + items
+endfunction
+
+function! s:mark_vars_in_select(str)
+    for item in a:str
+        if type(item) == type([])
+            call add(item, { 'select' : 1 })
+        endif
+        unlet! item " avoid E706
+    endfor
+endfunction
+
 function! s:parser_placeholder() dict abort
     let ret = self.text('}')
     return empty(ret) ? [''] : ret
@@ -171,6 +192,7 @@ endfunction

 function! s:parser_text(till) dict abort
     let ret = []
+    let till = '\V\[' . escape(a:till, '\') . ']'
     let target = ret

     while self.pos < self.len
@@ -206,7 +228,7 @@ function! s:parser_text(till) dict abort
         endif

         " Empty lines are ignored if this is tested at the start of an iteration
-        if self.next ==# a:till
+        if self.next =~# till
             break
         endif
     endwhile
@@ -268,11 +290,26 @@ endfunction
 function! s:parser_create_stubs() dict abort

     for [id, dict] in items(self.vars)
+
+        " only instance is in a selection, so remove it
+        if len(dict.instances) == 1 && type(dict.instances[0][-1]) == type({})
+                    \ && dict.instances[0][-1] == { 'select' : 1 }
+            call remove(self.vars, id)
+            continue
+        endif
+
         for i in dict.instances
             if len(i) > 1 && type(i[1]) != type({})
                 if !has_key(dict, 'placeholder')
-                    let dict.placeholder = i[1:]
-                    call add(i, dict)
+                    if type(i[1]) == type([]) && i[1][0] == 'select'
+                        let dict.placeholder = i[1][1]
+                        let dict.items = i[1][1:]
+                        let i[1] = dict.placeholder
+                        call add(i, dict)
+                    else
+                        let dict.placeholder = i[1:]
+                        call add(i, dict)
+                    endif
                 else
                     unlet i[1:]
                     call s:create_mirror_stub(i, dict)
@@ -281,6 +318,7 @@ function! s:parser_create_stubs() dict abort
                 call s:create_mirror_stub(i, dict)
             endif
         endfor
+
         if !has_key(dict, 'placeholder')
             let dict.placeholder = []
             let j = 0
@@ -292,6 +330,7 @@ function! s:parser_create_stubs() dict abort
             call add(dict.instances[j], dict)
             call filter(dict.mirrors, 'v:val isnot oldstub')
         endif
+
         unlet dict.instances
     endfor

@@ -301,9 +340,13 @@ function! s:create_mirror_stub(mirror, dict)
     let mirror = a:mirror
     let dict = a:dict
     let stub = get(mirror, 1, {})
-    call add(mirror, stub)
-    let dict.mirrors = get(dict, 'mirrors', [])
-    call add(dict.mirrors, stub)
+    if stub == { 'select' : 1 }
+        unlet mirror[1:]
+    else
+        call add(mirror, stub)
+        let dict.mirrors = get(dict, 'mirrors', [])
+        call add(dict.mirrors, stub)
+    endif
 endfunction

 function! snipmate#parse#snippet(text, ...) abort
@@ -318,6 +361,6 @@ endfunction

 call extend(s:parser_proto, snipmate#util#add_methods(s:sfile(), 'parser',
             \ [ 'advance', 'same', 'id', 'add_var', 'var', 'varend',
-            \   'line', 'string', 'create_stubs', 'pat',
+            \   'line', 'string', 'create_stubs', 'pat', 'select',
             \   'placeholder', 'subst', 'expr', 'text', 'parse',
             \ ]), 'error')
diff --git a/doc/SnipMate.txt b/doc/SnipMate.txt
index 584b20b..4238248 100644
--- a/doc/SnipMate.txt
+++ b/doc/SnipMate.txt
@@ -482,6 +482,21 @@ empty string if it hasn't. The second returns the filename if it's been named,
 and "name" if it hasn't. The third returns the filename followed by "_foo" if
 it has been named, and an empty string if it hasn't.

+                                                         *SnipMate-selections*
+Selections~
+
+Note: Version 1 of the parser is required for selections.
+
+In addition to providing sample text as a placeholder, you can specify a list
+of options that the user can choose from via |popupmenu-completion|:
+>
+    snippet todo
+        # ${1|TODO|FIXME|BUG}: $2
+<
+Notice that the : is replaced by the pipe |. Currently mirrors or evaluations
+are not supported within selection text. If alterations are made to the
+selected text, the stop becomes a regular stop, losing the selections.
+
                                                              *SnipMate-visual*
 The VISUAL Stop~

diff --git a/t/jumping.vim b/t/jumping.vim
index 1c3ab0d..7d2a92c 100644
--- a/t/jumping.vim
+++ b/t/jumping.vim
@@ -74,8 +74,15 @@ describe 'snippet state'
             Expect b:snip_state.stop_no == 3
         end

-        it 'does something at the ends'
-            "
+        it 'wraps around when going before the first or after the last stop'
+            let b:snip_state.stops = { 0 : {}, 1 : {}, 2 : {}, 3 : {} }
+            let b:snip_state.stop_count = 4
+            let b:snip_state.stop_no = 3
+            call b:snip_state.find_next_stop(0)
+            Expect b:snip_state.stop_no == 1
+            let b:snip_state.stop_no = 1
+            call b:snip_state.find_next_stop(1)
+            Expect b:snip_state.stop_no == 3
         end

     end
diff --git a/t/parser.vim b/t/parser.vim
index 3c2aa6e..65a28a6 100644
--- a/t/parser.vim
+++ b/t/parser.vim
@@ -1,6 +1,9 @@
 describe 'snippet parser'

     before
+        " two optional arguments:
+        " first one:  whether or not to create the stop stubs
+        " second one: whether or not to return the stops
         function! Parse(snippet, ...)
             let [snip, stops] = snipmate#parse#snippet(a:snippet, (a:0 ? a:1 : 1))
             return (a:0 > 1 && a:2) ? [snip, stops] : snip
@@ -149,4 +152,19 @@ describe 'snippet parser'
         Expect Parse("foo\n\nbar") == [['foo'], [''], ['bar']]
     end

+    it 'parses a selection as a special var named "select" with each item'
+        Expect Parse("${1|foo|bar|baz|select}") ==
+                    \ [[[1, ['select', 'foo', 'bar', 'baz', 'select']]]]
+    end
+
+    it 'stores selection items in the var dictionary'
+        let [snip, stops] = Parse("${1|foo|bar|baz|select}", 0, 1)
+        Expect stops[1].items == ['foo', 'bar', 'baz', 'select']
+    end
+
+    it 'sets a selections placeholder to the first item'
+        let [snip, stops] = Parse("${1|foo|bar|baz|select}", 0, 1)
+        Expect stops[1].placeholder == 'foo'
+    end
+
 end
ajzafar commented 5 months ago

This is actually working as expected on my end. Do you mind sharing the output of se cpt? cot? here? I suspect there's a configuration difference that needs to be taken into account for this to work right. I have complete=.,w,b,u,t,i completeopt=longest,menu, preview.

yann-morin-1998 commented 5 months ago

@ajzafar Here is the output:

:se cpt
complete=.,w,b,u,t,i
:se cot
completeopt=menu,preview

And if I set completeopt=longest,menu,preview, then I get the expected behaviour!

ajzafar commented 5 months ago

I can confirm that se cot+=longest will fix this for you. Why that fixes it, I don't know and will have to do more digging.

On Wed, Jun 5, 2024, 11:03 Yann E. MORIN @.***> wrote:

@ajzafar https://github.com/ajzafar Here is the output:

:se cpt complete=.,w,b,u,t,i :se cot completeopt=menu,preview

— Reply to this email directly, view it on GitHub https://github.com/garbas/vim-snipmate/issues/304#issuecomment-2150301343, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEOJLOJITVKYPGBBUT3NQTZF4SDZAVCNFSM6AAAAABIZFQGP2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQGMYDCMZUGM . You are receiving this because you were mentioned.Message ID: @.***>

yann-morin-1998 commented 5 months ago

I can confirm that se cot+=longest will fix this

Thanks. I've already set that in my ~/.vim/vimrc.

I'm not sure if the issue can be considered a local configuration issue. If you believe so, then I'll be happy if you close this issue. If you believe you want to fix it in code, then I'll be happy to test that here.

Thanks!

ajzafar commented 5 months ago

Okay I understand this issue now. SnipMate is preserving the last selected selection by using feedkeys() to feed a certain number of <C-N>s to make the right selection. The number of <C-N>s needed depends on 'cot': some values of 'cot' will select the first completion option, while others will not. SnipMate now determines how many should be fed no matter what 'cot' is set to. I believe.

I also found and fixed another bug where SnipMate wouldn't remember the selection if selections are the same length. So thanks for that.

Please reopen if I'm incorrect about how SnipMate now handles 'cot'.

yann-morin-1998 commented 5 months ago

I reverted to my initial completeopt=menu,preview, and the behaviour is still as expected now.

Good job, thanks a lot! :-)