Improve or disable CtrlP-Integration (by default)

[MattesGroeger]

~~I'm quite sure this was the case in an older version of this plugin. The problem with the current implementation – that only shows the line preview: it's hard to see where the bookmark is located. Especially when it is an empty line.~~

The Ctrl-Integration does not provide information about the file and line-number which makes it really hard to see where bookmarks are located.

I'd suggest to do one of these two options: a) Add file and line-number to the CtrlP window entries (if possible) b) Deactivate the CtrlP-integration by default

[nkgm]

Works for me in quickfix - shows path/to/file|lineno| content OR annotation. Is this related to CtrlP/Unite integration?

[MattesGroeger]

You are right, I had CtrlP installed and it was automatically using it. Now seeing how (bad) the CtrlP-Integration works I'd rather change the default setting to let g:bookmark_disable_ctrlp = 1. What do you think? If people want to use it, they should explicitly enable that.

[nkgm]

Principle of least surprise 👍 The integration should definitely be improved though.

[nkgm]

I've been looking at the docs and I can't seem to find a Unite counterpart for g:bookmark_disable_ctrlp, which makes me wonder, what happens if you have both and you want to use one/the other/quickfix? This feels somewhat inconsistent.

Being a recent Fzf convert from CtrlP, I can see 2 basic use cases:

1. Quickly navigate to bookmark using a fuzzy selection window (like CtrlP behaves at the moment with bookmark_auto_close implied)
2. Go through the list of bookmarks with quickfix window open and unimpaired ]q/[q mappings

Right now you can't have both because you have to decide what <Plug>BookmarkShowAll should do based on a configuration variable. This feels a bit backwards. Wouldn't it be much simpler to have <Plug>BookmarkShowAll, <Plug>BookmarkShowAllCtrlP, <Plug>BookmarkShowAllUnite, <Plug>BookmarkShowAllFzf, and leave it up to the user to pick their poison? You're trading less configuration for more flexibility - I'd take that deal any time :)

[MattesGroeger]

@nkgm I like your idea. I also feel that all these configurations get a bit out of hand.

According to your suggestion the default key binding (ma) would call <Plug>BookmarkShowAll and the user could override this to use the integration of his choice, right? I would like that.

[nkgm]

Yes keep good old quickfix default as ma and let the user decide if they want to override/create additional mappings for more providers. Of course we'd have the respective commands :BookmarkShowAllCtrlP, :BookmarkShowAllUnite etc. While doing this, it would also pay to bring https://github.com/MattesGroeger/vim-bookmarks/issues/92 into the mix. What do you think @MattesGroeger?

[nkgm]

Another point to consider wrt Unite is it is now superceded by Denite and no longer actively developed.

[MattesGroeger]

Regarding #92: Would this mean we'd also need :BookmarShowAllFile, :BookmarShowAllFileCtrlP and :BookmarkShowAllFileUnite? Whereas :BookmarShowAllFile would get it's own default mapping (mf?).

Regarding Unite: I've never used it myself and not sure about the adoption rate. If not maintained and used anymore I'd be fine with removing support if it makes things easier... or what where you aiming at @nkgm?

[nkgm]

We're probably looking at a long lived [WIP] PR where all worthy alternatives would be explored in practice. As a starting point I'm thinking :BookmarkShow [args], :BookmarkShowCtrlP [args], with BookmarkShowAll* variants calling their BookmarkShow* equivalents under the hood.

As for Unite vs Denite, I'm thinking Denite only - no point in bloating the core with support for a deprecated (though great) plugin. For die hard Unite fans, it should be fairly easy to roll their own support in their vimrc if we provide a gist as a starting point. Ultimately, if this becomes a case of feature creep, we keep things simple and leave the opinionated parts to userland.

I'm keen to work on this PR, yet be warned I'm only moderately experienced in Fzf and CtrlP and maybe a little in Denite, even though it's definitely worth getting to know more. So, as far as timelines go, we're looking at a few weeks, and that's after sorting https://github.com/MattesGroeger/vim-bookmarks/issues/118 :)

[MattesGroeger]

@nkgm I like your suggestion regarding naming and dropping the Unite support/providing a gist! Please go ahead if you find time and please provide some detailed explaination for the upcoming changes in the PR description that outlines the thought process for future reference and potential discussion 👍 😃

[Roy-Orbison]

Am I missing something, or is this not in the current version?

[nkgm]

Short answer no. I was meant to look into this after fixing https://github.com/MattesGroeger/vim-bookmarks/issues/118 but turned out to be a wider issue with persistence affecting more open issues than the original one, so it has to take priority.