Skip to content

Allowing the creation or modification of Wiki{Pages,Tag,Toc}#318

Closed
saccarosium wants to merge 8 commits intolervag:masterfrom
saccarosium:custom-interface
Closed

Allowing the creation or modification of Wiki{Pages,Tag,Toc}#318
saccarosium wants to merge 8 commits intolervag:masterfrom
saccarosium:custom-interface

Conversation

@saccarosium
Copy link
Copy Markdown
Contributor

@saccarosium saccarosium commented Jul 25, 2023

Hi @lervag I'm back :),
When I first implemented the vim.ui.select (#281) backed for the Wiki{Pages,Tag,Toc}, I've overestimated the usefulness of this implementation. The function is very primitive and doesn't let you do anything that I will consider useful for searching my note (e.g. inability to open split from selected item and the lack of a preview).

For this reason I'm very confident very few are actually using the feature.

So since I think that adding every support for every fuzzy finder is not viable, and even if it was, we still could not represent the needs of all users, I've been thinking of an interface where people can overwrite/write the different backends for this commands. This can have the following benefits:

  • Removing existing wiki_fzf_pages_opts and wiki_fzf_tags_opts, since the user will be able to simply overwrite the fzf backend;
  • Make possible to keep this commands without creating new ones;
  • Opens possibility for create plugins to add support for other backends and uis.

This implementation is very hacked toghether and very awkward to use both in lua and vimscript (need your infinite vimL knowledge to figure out a way to do it that makes sense).

@lervag
Copy link
Copy Markdown
Owner

lervag commented Jul 26, 2023

Hi @lervag I'm back :)

Glad to see you! :)

When I first implemented the vim.ui.select (#281) backed for the Wiki{Pages,Tag,Toc}, I've overestimated the usefulness of this implementation. The function is very primitive and doesn't let you do anything that I will consider useful for searching my note (e.g. inability to open split from selected item and the lack of a preview).

For this reason I'm very confident very few are actually using the feature.

Interesting observation, and I think you may be right.

So since I think that adding every support for every fuzzy finder is not viable, and even if it was, we still could not represent the needs of all users, I've been thinking of an interface where people can overwrite/write the different backends for this commands.

That's the kind of interface I like!

This can have the following benefits:

  • Removing existing wiki_fzf_pages_opts and wiki_fzf_tags_opts, since the user will be able to simply overwrite the fzf backend;

  • Make possible to keep this commands without creating new ones;

  • Opens possibility for create plugins to add support for other backends and uis.

Yes, it would be nice to get some of these benefits.

This implementation is very hacked toghether and very awkward to use both in lua and vimscript (need your infinite vimL knowledge to figure out a way to do it that makes sense).

I think you may be onto something here. But I'd like us to take one step back. I think it can be useful to consider how a user would want to customize things. In your PR, you did not provide any documentation for the new option. And I think we would also need to update the related docs for :WikiPages and similar.

I believe it can be easier to figure out how to implement things after we have thought about the user perspective.

This is your current proposal:

let g:wiki_select_methods = {
      \ 'fzf': {
      \   'pages': 'call wiki#wiki#fzf#pages()',
      \   'tags': 'call wiki#wiki#fzf#tags()',
      \   'toc': 'call wiki#fzf#toc()',
      \ },
      \ 'ui_select': {
      \   'pages': 'lua require("wiki.ui_select").pages()',
      \   'tags': 'lua require("wiki.ui_select").tags()',
      \   'toc': 'lua require("wiki.ui_select").toc()',
      \ },
      \}

And you would use g:wiki_select_method to specify the desired backend.

First: Perhaps it would be better to call the new option g:wiki_select_backends. Next, the new option is only used together with the new option. So, perhaps it is better to combine these.

As a first attempt, I propose this:

*g:wiki_select_methods*
  A dictionary that specifies the selection method to be used for some
  commands. This allows the user quite a lot of flexibility in specifying which
  backend to use for which command, and also in creating custom backends as
  desired.

  The following table shows the option keys and the related commands:

    key     command
    ---     -------
    `pages` |WikiPages|
    `tags`  |WikiTags|
    `toc`   |WikiToc|

  The dictionary values can be one of two types:

    - |String|: The value must be either
        1) the name of a function to be executed, or
        2) a command line to be evaluated.
    - |Funcref|: A function that will be executed.

  The following builtin alternatives can be used:

    - fzf (https://github.com/junegunn/fzf.vim):
      - `wiki#fzf#pages()`
      - `wiki#fzf#tags()`
      - `wiki#fzf#toc()`
      - Note: This has a few additional options for more fine tuned control:
        - |g:wiki_fzf_pages_opts|
        - |g:wiki_fzf_tags_opts|
        - |g:wiki_fzf_pages_force_create_key|
    - Lua backend that uses |vim.ui.select| (neovim only!)
      - `require("wiki.ui_select").pages()`
      - `require("wiki.ui_select").tags()`
      - `require("wiki.ui_select").toc()`

  Default (neovim): >lua

    vim.g.wiki_select_methods = {
      pages = require("wiki.ui_select").pages,
      tags = 'equire("wiki.ui_select").tags,
      toc = require("wiki.ui_select").toc,
    }
<
  Default (Vim): >vim

    let g:wiki_select_methods = {
          \ 'pages': 'wiki#fzf#pages',
          \ 'tags': 'wiki#fzf#tags',
          \ 'toc': 'wiki#fzf#toc',
          \}
<

This would avoid adding a new option, but it would be a breaking change as we replace one option with a new one. I would also consider to remove the g:wiki_fzf options and instead somehow bake them into this new one. E.g., perhaps by using a lambda with an option block to the fzf#... functions as the default options.

What do you think?

@lervag
Copy link
Copy Markdown
Owner

lervag commented Jul 26, 2023

Also, note that I've recently implemented a better (IMHO) UI for input, confirm and select for other parts for neovim. I was not able to use vim.ui.select because the code is too tied to the standard pattern where the selection/input is blocking. But I think the new UI is quite good. Perhaps it would be possible to further improve it and allow it as one choice for this option as well..?

@saccarosium
Copy link
Copy Markdown
Contributor Author

let g:wiki_select_methods = {
     \ 'pages': 'wiki#fzf#pages',
     \ 'tags': 'wiki#fzf#tags',
     \ 'toc': 'wiki#fzf#toc',
     \}

This would avoid adding a new option, but it would be a breaking change as we replace one option with a new one. [...]

This makes definitely more sense. My idea is that the user could use either a functionref or a lambda function. I know how how this would looks like in lua, but I'm not sure how it is translated in viml:

-- As you show in the doc snippet
vim.g.wiki_select_methods = {
  pages = require("wiki.ui_select").pages,
  tags = require("wiki.ui_select").tags,
  toc = require("wiki.ui_select").toc,
}
-- or
vim.g.wiki_select_methods = {
  -- I don't know the telescope API is just an example
  pages = function() require'telescope'.find_files({cwd = vim.g.wiki_root}) end,
}

This is how I think of the final interface, at least in lua, for viml I don't really know.
How do function in viml be passed to other functions (if you happen to know)? Do we only have the option of passing strings and then calling call on them?

I would also consider to remove the g:wiki_fzf options and instead somehow bake them into this new one. E.g., perhaps by using a lambda with an option block to the fzf#... functions as the default options.

I don't really see value in this option since, I at least, would simply rewrite the function if there was something I didn't like. But probably better to keep it around since it doesn't hurt anybody. But also I think passing the parameter to a lambda would be a much nicer since you are not "polluting" the global vim "namespace" and could lead to a more descriptive and understandable experience (we could pass a dictionary with all flags you could activate { preview: "bat", layout: "reverce" }).

Also, note that I've recently implemented a better (IMHO) UI for input, confirm and select for other parts for neovim. I was not able to use vim.ui.select because the code is too tied to the standard pattern where the selection/input is blocking. But I think the new UI is quite good. Perhaps it would be possible to further improve it and allow it as one choice for this option as well..?

I see the new UI. I think is really neat. But I think that would be a good idea start thinking about dividing the plugin in separate plugin, perhaps the new UI is a good idea for a plugin named wiki-ui.vim, since is getting very big and I would love to see this plugin becoming more composable (but it is an other issue that should be further discussed).

@lervag
Copy link
Copy Markdown
Owner

lervag commented Jul 26, 2023

This makes definitely more sense. My idea is that the user could use either a functionref or a lambda function. I know how how this would looks like in lua,

-- As you show in the doc snippet
vim.g.wiki_select_methods = {
  pages = require("wiki.ui_select").pages,
  tags = require("wiki.ui_select").tags,
  toc = require("wiki.ui_select").toc,
}
-- or
vim.g.wiki_select_methods = {
  -- I don't know the telescope API is just an example
  pages = function() require'telescope'.find_files({cwd = vim.g.wiki_root}) end,
}

Yes, precisely. We can either specify a function directly or use a simple lambda to wrap a function call.

This is how I think of the final interface, at least in lua, for viml I don't really know. How do function in viml be passed to other functions (if you happen to know)? Do we only have the option of passing strings and then calling call on them?

We can do similar things here. I am proposing three alternatives:

  1. Using Funcrefs (see :help Funcref) and lambdas.
  2. Using the name of a function and then the call() method (see :help call()).
  3. Using a full command with execute similar to what you propose in your PR.

Both 2. and 3. means the value will be of type String. We can differ between them by checking with exists('*function-name'), see :help exists()). If the string is not recognized as a function name, then we will need to assume it is a command.

I would also consider to remove the g:wiki_fzf options and instead somehow bake them into this new one. E.g., perhaps by using a lambda with an option block to the fzf#... functions as the default options.

I don't really see value in this option since, I at least, would simply rewrite the function if there was something I didn't like. But probably better to keep it around since it doesn't hurt anybody. But also I think passing the parameter to a lambda would be a much nicer since you are not "polluting" the global vim "namespace" and could lead to a more descriptive and understandable experience (we could pass a dictionary with all flags you could activate { preview: "bat", layout: "reverce" }).

Yes, exactly, except - to avoid too much work - let's just use a string similar to what we do now.

" Alternative spec for default setting
let g:wiki_select_methods = {
      \ 'pages': { _ -> wiki#fzf#pages() },
      \}

" Add a previewer to the fzf options by passing an optional string argument
let g:wiki_select_methods = {
      \ 'pages': { _ -> wiki#fzf#pages('--preview "cat {1}"') },
      \}

This also shows how we can use a lambda function here in Vimscript, btw.

Also, note that I've recently implemented a better (IMHO) UI for input, confirm and select for other parts for neovim. I was not able to use vim.ui.select because the code is too tied to the standard pattern where the selection/input is blocking. But I think the new UI is quite good. Perhaps it would be possible to further improve it and allow it as one choice for this option as well..?

I see the new UI. I think is really neat. But I think that would be a good idea start thinking about dividing the plugin in separate plugin, perhaps the new UI is a good idea for a plugin named wiki-ui.vim, since is getting very big and I would love to see this plugin becoming more composable (but it is an other issue that should be further discussed).

Yes, I partly agree with this. With time, I've found I've implemented some "basic API functionality" that I would share between plugins. So, you will find things like these new UI features implemented exactly the same in both wiki.vim and VimTeX. Similar for other features, e.g. the path API, the jobs API, and perhaps more. It is quite clear that there would be benefits to refactoring these parts out into one (or more) separate library(-ies).

However, I also think there is a clear benefit to users of avoiding dependencies. It puts some more work on my shoulders, but not much.

I think the most important thing for wiki.vim is that I (we) keep working on making it modular, robust and possibly also more composeable. But this may not necessarily mean that we should take things out; at least not without properly discussing and considering the costs and gains. If you feel strongly about this, please feel free to open a new issue to discuss it further.

@saccarosium
Copy link
Copy Markdown
Contributor Author

I've pushed some changes and documentation. At the end I think is best to keep using g:wiki_select_method, because to me makes more sense since at the end is only one function or another one and that how you call the function make more sense, at least to me.

If you like to keep it like this we should have a deprecation warning so people who where using the old interface can change to the new one (I didn't implement it yet)

@lervag
Copy link
Copy Markdown
Owner

lervag commented Jul 27, 2023

I've pushed some changes and documentation. At the end I think is best to keep using g:wiki_select_method, because to me makes more sense since at the end is only one function or another one and that how you call the function make more sense, at least to me.

Yes, that's fine by me.

If you like to keep it like this we should have a deprecation warning so people who where using the old interface can change to the new one (I didn't implement it yet)

Yes. Or, I'm not sure. I think perhaps it should suffice if we add a breaking change notice and then make sure to highlight the change in the next release notes. I would prefer not to maintain deprecation notices.

Now it seems two things remain:

  • The Vimscript implementation.
  • Adding some tests.

Do you want me to complete this, or do you want to work on it more first?

@saccarosium
Copy link
Copy Markdown
Contributor Author

Let work on it a little longer. Probably on Saturday I will finish it.

I want to:

  • add the deprecation message
  • add a telescope backend. Is the most used fuzzy finder on neovim, is probably a good idea to have it as an option (I'll keep vim_ui_select as the default tough)
  • create a fzf-lua example to use it for myself and put it in the docs under wiki-advanced-config-3

Also is probably a good idea to put the select backend files in a subfolder in the lua directory if we want to extend the lua code (#319). Any suggestion for the name? I was thinking something like lua/interfaces or lua/select.

@saccarosium
Copy link
Copy Markdown
Contributor Author

I've finished most of the PR. The only things that are not done yet are:

  • the telescope toc function is not working, I didn't found a way to get the selected item
  • I've layed out option passing to the telescope functions but I don't pass anything to the calls

@lervag from now on you can finish the PR if you have the time.

@saccarosium saccarosium marked this pull request as ready for review July 29, 2023 16:45
@lervag
Copy link
Copy Markdown
Owner

lervag commented Aug 9, 2023

Thanks! Sorry for the delay, I've not had much time lately due to vacations and similar.

I've merged this now. I believe I fixed the issue you had with telescope for WikiToc. I also found a couple of minor bugs and I slightly improved the docs.

Feel free to open follow-up issues or PRs if I should have missed something!

@lervag lervag closed this Aug 9, 2023
@lervag
Copy link
Copy Markdown
Owner

lervag commented Aug 9, 2023

Sorry, I forgot to tag this PR while doing this. The merge commit is here: 0445962.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants