zk-org / zk-nvim

Neovim extension for zk
https://github.com/zk-org/zk
GNU General Public License v3.0
530 stars 42 forks source link

Added support for fzf-lua as a picker #138

Closed vicrdguez closed 1 year ago

vicrdguez commented 1 year ago

Hi!

I moved to use fzf-lua and have been working these days to be able to use it with zk-nvim. While you are already supporting fzf, fzf-lua adds some nice goodies and this picker makes finding notes more consistent if you are a user of the plugin.

The small drawback is that is not very configurable right now, since picker_options is not exposed to be used with built-in commands, and this does not implement an extension like with telescope, so the picker can't be configured from fzf-lua options either. For that reason I've added some default actions that I think are handy, like opening selections in splits or tabs. If picker_options can be exposed at a later time, I can change this so people can configure mappings and other options themselves.

Take a look and let me know if this fits what you want to have in the plugin, and also if there is anything wrong I should change!

Cheers

vicrdguez commented 1 year ago

Noticed, that the current state of the picker works well for commands that just open notes, but not on the others that insert content to notes, like ZkLinks, since zk callback is skipped to make fzf-lua actions work. Changed the PR as draft now, I'll take a look on how to solve this issues.

vicrdguez commented 1 year ago

I think all should be working now. I've change the title of the picker for inserting links to be able to identify when ZkInsertLink(AtSelection) is being used instead of any other commands, so the selected note gets passed to zk-nvim callback instead of using fzf-lua builtin actions.

Let me know if you think there is a better way to do this

mickael-menu commented 1 year ago

Hi @vicrdguez, thanks for contributing your implementation.

Unfortunately I'm not a Lua dev, so I can't really review in depth your PR.

No configuration options (picker_options) seems like an acceptable trade-off for now. However not calling the callback from the picker functions is a real problem, as it's critical to the integration in zk-nvim. As you've seen with ZkInsertLink, but people can also add custom commands to do other things with the picked notes.

I didn't understand why you chose to use fzf-lua for the editing option instead of always relying on the callback?

vicrdguez commented 1 year ago

Hi @mickael-menu

I'm not a luadev either, just an enthusiast. I'm just learning so things are very likely not perfect.

The reason why I defaulted to fzf-lua actions for the editing part is because using the callback was interfering with fzf-lua picker actions. What I've noticed is that the picker will send downstream the choice under the cursor no matter what keymap you press, and I could not find a way to not do it.

For example: you open the notes picker, but decided to cancel without choosing a note. You can press the default <C-c> keymap for that. However the note under the cursor gets sent to the callback anyway, and the note under the cursor is opened even if you don't want to. This happens the same if you for example want to open the note in a split, the behaviour is not as expected if you use the fzf-lua split action + the callback for edit.

I understand what you say though, the callback is one of the main integration points for customization, but I've noticed the following:

So, since you can't specify a callback for edit() anyway, then it was just a matter of identifying all the commands that just rely on edit() and exclude calling the callback just for those (which will execute just :e), and we can rely on the fzf-lua file_edit action, without any change on visible behavior. The way the check is implemented is a little rudimentary though, the code just identifies said commands by the Picker title, it is part of the last commit I pushed.

For all the other commands, the callback is executed normally, allowing for all the customization that was possible already if I'm not wrong.

vicrdguez commented 1 year ago

I guess this is also a limitation of fzf-lua's customizability as well, that is not present on telescope, were you can define actions on pickers independently.

vicrdguez commented 1 year ago

Right, so I've been daily driving the last changes I've just committed this last week. I miss-understood the conflict occurring between zk-nvim's callback and fzf-lua's open file action. The clash is definitely happening when they are executed together, but for the default action (pressing Enter/Return) really all needed is to open/edit the file. So for the said default action, the callback can be called as with any other picker like telescope, and just we rely on fzf-lua for actions like split opens.

I've also changed vim.tbl_extend for vim.tbl_deep_extend to enable customization of the actions by using the picker_options table. I managed to get a find or create command (which I was really after) with the following:

function M.find_or_create(opts, picker_opts, create_cb, pick_cb)
    -- same exact callback as zk-nvim/lua/zk.lua
    local def_pick_cb = function(notes)
        if picker_opts and picker_opts.multi_select == false then
            notes = { notes }
        end
        for _, note in ipairs(notes) do
            vim.cmd("e " .. note.absPath)
        end
    end
    -- by default we just have the option to create the note in main
    local def_create_cb = function(_, fzf_opts)
        local prompt_input = fzf_opts.__resume_data.last_query
        local dir = opts.dir or "main"
        require("zk.commands").get("ZkNew")({ title = prompt_input, dir = dir })
    end

    picker_opts = vim.tbl_extend("force", {
        title = "Zk find",
        fzf_lua = {
            actions = {
                ["ctrl-o"] = create_cb or def_create_cb
            }
        }
    }, picker_opts or {})

    require("zk").pick_notes(opts, picker_opts, pick_cb or def_pick_cb)
end

Which behaves exactly the same as ZkNotes command, but if you press (in this case) ctrl-o, a note will be created with whatever you have input in the picker prompt.

I guess this would also work similarly for telescope, since the mappings can be changed for the extension pickers implementing a custom "create callback" for particular key press.