Issue #2 resolved

3 bugfixes and one enhancement.

Ingo Karkat
created an issue

{{{ Hello Takeshi-san,

I have written a custom completion function that I wanted to use with autocomplpop.vim, and encountered some minor bugs that prevented the integration. I'm sharing the bugfixes and enhancement with you in the hope that you incorporate them into your script, as these aren't hacks required for my plugin, but general (though apparently rare) issues.

My completion function "MinLengthComplete" only offers completions that add more than n characters:

The built-in insert mode completion |i_CTRL-N| offers any completion matches, even ones that only add nothing or only a few characters, which can often be inserted more quickly by just typing the characters than browsing through the completion list. This plugin offers a custom completion function that only offers "worthwhile" matches that add at least n characters. It uses a backward |i_CTRL-P| search, so that previously typed words are offered first. (Note: This plugin hasn't yet been released, but I may plan to publish it on if you approve of my changes.)

Here's the list of changes to autocomplpop.vim:

  • BUG: When the configured completion is <C-p> or <C-x><C-p>, the command to restore the original text (in on_popup_post()) must be reverted, too. While experimenting with backwards-completion (<C-p>) to get preceding words offered before following words, I found out that the popup menu doesn't work correctly in these cases. I added a check in s:PopupFeeder.on_popup_post().

  • BUG: When using a custom completion function (<C-x><C-u>) that also uses an s:...() function name, the s:GetSidPrefix() function dynamically determines the wrong SID, because it is called from that surrounding context. Now calling s:DetermineSidPrefix() once during sourcing and caching the value in s:SID. The pattern in s:GetSidPrefix() is too broad; one could fix this by changing the pattern to

    return matchstr(expand('<sfile>'), '<SNR>\d+_\zeGetSidPrefix') Instead, I optimized further and looked up the SID only once during sourcing, using a cached s:SID value from then on.

  • BUG: Should not use custom defined <C-X><C-...> completion mappings. Now consistently using unmapped completion commands everywhere. (Beforehand, s:PopupFeeder.feed() used mappings via feedkeys(..., 'm'), but s:PopupFeeder.on_popup_post() did not due to its invocation via :map-expr.) In my vimrc, I have mapped all <C-n>, <C-x><C-f>, ... mappings to do some additional magic like pre-selecting the first match. That clashed with autocomplpop. In fact, autocomplpop partially uses the mappings, partially not. Since mappings cannot be used consistently in all places (in a :map <expr>, the returned <expr> is always executed as-is without remapping), I instead split the feedkeys() in s:PopupFeeder.feed() so that the completion command is not mapped, but the <Plug>AutocomplpopOnPopupPost must be mapped.

  • ENH: Added support for setting a user-provided 'completefunc' during the completion, configurable via g:AutoComplPop_Behavior. Auto-completion should not change the default 'completefunc'. Using the existing s:OptionManager, the value of 'completefunc' is temporarily overwritten during the auto-completion. If it were possible to consistently allow mappings for completion (see point above), one could alternatively do without this new option, and configure custom mappings for that. I actually prefer this new option, it makes it clearer which user-completion is actually used.

I'm attaching a diff against autocomplpop.vim 2.6.

-- kind regards, ingo

-- Ingo Karkat -- /^-- /^-- /^-- /^-- /^-- /^-- -- -- --


Comments (11)

  1. Takeshi NISHIDA repo owner

    I applied your patch: 1a8dcf726b63 . Thanks!

    I found that I prefer <C-p> to <C-n>, which I make autocomplpop use. I might change default behavior like that. I want to ask your opinion.

    BTW: I have a plan to refactor the whole plugin before releasing 2.7. It will cause you some tasks on writing custom completion functions which are used with this plugin.

    Thanks again.

  2. Ingo Karkat reporter

    Thank you for accepting my patch! I agree with you that <C-p> is a better default completion than <C-n>; recently typed words should be offered first, as there is a higher chance that one wants to re-use them. I only don't like the Vim behavior of presenting the list of completions upside-down in this case (with the first completion offer placed at the bottom of the popup menu), but I avoid this via my custom completion function, which generates the list of completions in backward order, but (being a <C-x><C-u> completion, not <C-p>) is listed in the default, top-to-bottom order.

    With my recollection of the source code, I don't see an immediate need for refactoring, but if you have ideas to improve things (without introducing too many new bugs :-), go ahead! I'll try to follow this project, so that I may be able to provide early feedback on your refactorings, as I have a quite complex Vim setup with regards to completions.

    One thing that I wished were easier is replacing the default completion; as I see it, there are two options:

    a) Defining g:AutoComplPop_Behavior before sourcing autocomplpop, but then I would have to duplicate all the configuration from the script.

    b) Changing g:AutoComplPop_Behavior after sourcing autocomplpop. I currently do this:

    " We could initialize g:AutoComplPop_Behavior before sourcing autocomplpop.vim,
    " but then we would have to duplicate the entire configuration for each
    " filetype. Instead, we rather make minor modifications after the fact. 
    function! s:AutoComplPopBehavior()
        for l:filetype in keys(g:AutoComplPop_Behavior)
    	let l:firstBehav = g:AutoComplPop_Behavior[l:filetype][0]
    	" Change the default completion from CTRL-N to the custom
    	" MinLengthComplete function. 
    	if l:firstBehav.command ==# "\<C-n>"
    	    let l:firstBehav.command = "\<C-x>\<C-u>"
    	    let l:firstBehav.completefunc = 'MinLengthComplete#MinLengthComplete'
    call s:AutoComplPopBehavior()
    delfunction s:AutoComplPopBehavior 

    -- arigatou, ingo

  3. Ingo Karkat reporter

    One thing that I wished were easier is replacing the default completion; as I see it, there are two options...

    I see, you already have addressed this via the new g:acp_behaviorKeywordCommand option. Thanks!

  4. Ingo Karkat reporter

    Thanks, that was quick. A quick test revealed no problems, and my custom completion function is still working.

    Even though I could now use :let g:acp_behaviorKeywordCommand = "\<C-x>\<C-u>", I still need to do my postprocessing (as outlined in comment #5) in order to also set the 'completefunc' for each default completion configuration element; I would need an option "g:acp_behaviorCompleteFunc" to get rid of that. OTOH, my use case may be rare, and I don't mind having to postprocess.

    The refactored autocomplpop now requires Vim 7.2, I guess mainly because you don't want to test with Vim 7.1 any more, not for technical reasons. I can understand this; OTOH, this forces users who share their .vim config on multiple machines to keep the old autocomplpop 2.6 plus its different configuration around. (The same criticism applies to fuzzyfinder, too.) I still have some older Linux systems around that still run Vim 7.1, so I would greatly welcome if both fuf and acp could support the current + previous (i.e. 7.1) version of Vim.

    Oh, and for the next occasion of "tinkering", I would suggest splitting off an autoload script, as you have done with fuf; I only use acp occasionally, so a reduction of Vim load time is always appreciated.

    Overall, good job, thanks Takeshi-san!

  5. Takeshi NISHIDA repo owner

    I still need to do my postprocessing

    I added g:acp_behaviorUserDefinedFunction option and g:acp_behaviorUserDefinedPattern option. Please try them.

    you don't want to test with Vim 7.1 any more

    No. I have no motivation.

    I would suggest splitting off an autoload script

    Yes, I did.

  6. Ingo Karkat reporter

    Great. Just great. With your changes in AutoComplPop 2.8, I'm able to do without my postprocessing, and replacing the default (CTRL-P) completion with a user-defined one is very straightforward now:

    " Do not use the default (CTRL-P) completion, just my user-defined one. 
    let g:acp_behaviorKeywordLength = -1
    " Replace default completion with my user-defined one. 
    let g:acp_behaviorUserDefinedFunction = 'MinLengthComplete#MinLengthComplete'
    let g:acp_behaviorUserDefinedPattern = '\k\{4,}$'

    Thanks a lot, Takeshi-san!

  7. Log in to comment