ctrlpvim / ctrlp.vim

Active fork of kien/ctrlp.vim—Fuzzy file, buffer, mru, tag, etc finder.
ctrlpvim.github.com/ctrlp.vim
Other
5.58k stars 260 forks source link

Setting a custom matcher on a per-plugin basis #118

Open dset0x opened 9 years ago

dset0x commented 9 years ago

Hello,

By enabling users to have purpose-built matchers, it becomes easier to adapt CtrlP to cover use cases that it was never meant to. Is there a reasonable way to temporarily modify the matcher? If not, I would propose this patch in a PR:

diff --git a/autoload/ctrlp.vim b/autoload/ctrlp.vim
index 64e6852..c8b3cc1 100644
--- a/autoload/ctrlp.vim
+++ b/autoload/ctrlp.vim
@@ -501,6 +501,8 @@ endf
 fu! s:MatchedItems(items, pat, limit)
        let exc = exists('s:crfilerel') ? s:crfilerel : ''
        let items = s:narrowable() ? s:matched + s:mdata[3] : a:items
+       let ext_matcher = s:getextvar('matcher')
+       if type(ext_matcher) == type({}) | let s:matcher = ext_matcher | en
        if s:matcher != {}
                let argms =
                        \ has_key(s:matcher, 'arg_type') && s:matcher['arg_type'] == 'dict' ? [{

This, for instance, enabled me to to invoke ag inside the matcher to get very fast content matches in large projects. Coupled with a custom ctrlp#foo#accept the possibilities broaden.

mattn commented 9 years ago

SGTM but wait for other's comment.

tacahiroy commented 9 years ago

My understanding is that allows users to configure matcher using like this variable: g:ctrlp_path_matcher. I've played around with this patch a bit and noticed this worked for only some built-in modes, s:itemtype < 2. With this way, users can configure matcher for:

call add(g:ctrlp_ext_vars, {
  \ 'init':   'ctrlp#foo#init()',
....
  \ 'sort':   0,
  \ 'matcher':  g:ctrlp_foo_matcher
  \ })

However, this is inconsistent and I don't think any extensions contain matcher in its ext_var. The plugin need to provide a consistent way to do the same thing, IMHO.

Any thoughts?

dset0x commented 9 years ago

@tacahiroy This patch allows plugin writers to use ext_var indeed, but the intent is not to interface this to the user, but to ship a matcher with the plugin. The point is that the user should not need to know that the matcher was temporarily altered from their preference (although of course we can make it a variable however, as you propose).

matcher in ext_var is not supported as far as I understand, which is why nobody overrides it.


I think an example would help understand the use-case that I describe above:

Goal: Make a plugin that allows us to use CtrlP to search in files, without breaking other features of CtrlP.
call add(g:ctrlp_ext_vars, {
  \ 'init': 'ctrlp#silver#init()',
  \ 'accept': 'ctrlp#silver#accept',
  \ 'type': 'line',
  \ 'enter': 'ctrlp#silver#enter()',
  \ 'matcher': {'match': 'silvermatcher#AgMatch'},
  \ })

" In other words, we have a python matcher called `AgMatch` that calls `/usr/bin/ag` and returns `ag`-formatted output that looks like this:

" lib/python2.7/site-packages/setuptools/command/build_ext.py:289:        )
" lib/python2.7/site-packages/setuptools/command/build_ext.py:290:
" lib/python2.7/site-packages/setuptools/command/register.py:1:import distutils.command.register as orig
" lib/python2.7/site-packages/setuptools/command/register.py:2:
" lib/python2.7/site-packages/setuptools/command/register.py:3:class register(orig.register):
" lib/python2.7/site-packages/setuptools/command/register.py:4:    __doc__ = orig.register.__doc__

" `CtrlP` doesn't know how to parse this kind of output (and it shouldn't have to). This is why we need a custom matcher.

function! ctrlp#silver#init()
  return ["Start typing to search"]
endfunction

function! ctrlp#silver#accept(mode, str)
  call ctrlp#exit()
  " And here we naively parse `ag`'s output:
  let filename = split(a:str, ":")[0]
  let lineno = split(a:str, ":")[1]
  exec "keepalt edit " . filename
  exec ":" . lineno
endfunction

Is itemtype the same as ispath? If so, it seems to me that the distinction becomes irrelevant with plugins that do not work on files/buffers/etc.

dset0x commented 9 years ago

It seems that I missed your point about inconsistency. In my head there was no "one single way" in which matchers need to be declared for these plugins.


Edited: Perhaps see the last point in my last comment: We drop the distinction in plugins/matchers that are built for a single purpose.

We could perhaps make CtrlP decide on matcher like this:

For a plugin named foo, we want check against file type: * if g:ctrlp_foo_matcher['match'] exists, use that. * elif foo: ctrlp_ext_vars['matcher']['file'] exists, use that, * elif foo: ctrlp_ext_vars['matcher'] exists, use that, " Indeed, at this point it gets ugly * else, fallback to g:ctrlp_file_matcher. " Now we'd have to also promote this to support 'file' however.

tacahiroy commented 9 years ago

ah - Since I missed title of this ticket and misunderstood what you're aiming at, I guessed wrong :| Now, I see your point, so LGTM :+1:

ludovicchabant commented 9 years ago

So if I get this right, it means we would have 2 ways to specify custom matchers: g:ctrlp_match_func, and extensions, via ext_vars? (currently, only the first way works)

Sounds good, and the use-case presented originally is actually pretty cool I think. However, I think there are problems with the proposed patch. @dset0x, can you start a PR so we can review the code properly? Thanks!

mattn commented 9 years ago

@wsdjeg please explain what is meaning of your comment?

mattn commented 9 years ago

@wsdjeg we are NOT discussing about matcher#cmatch now.

dset0x commented 8 years ago

I'm afraid this has to wait a little bit longer for a PR as work has taken over my time lately. Thanks for your patience.