nvim-tree / nvim-tree.lua

A file explorer tree for neovim written in lua
Other
7.23k stars 610 forks source link

Expose DEFAULT_KEYMAPS And Current Mappings #1858

Closed hinell closed 1 year ago

hinell commented 1 year ago

I requet to export default keymapping to ensure stable api .e.g.:

local   keymaps = require("nvim-tree").DEFAULT_KEYMAPS
...

Can this functionality be implemented utilising API?

require("nvim-tree.keymap").keymaps

Note The callbacks should be exported explicitly!

mrjones2014 commented 1 year ago

Copying my comment here in case you are interested in helping me figure out how an implementation would work with an upstream plugin: https://github.com/mrjones2014/legendary.nvim/issues/289#issuecomment-1364618688

Hey there, this is awesome, thanks for filing the issue!

This may be better suited actually for #184, nvim-tree is probably a good candidate for a first one for a proof of concept.

I'm not sure exactly what they will look like, but I have a few ideas. The best idea I have currently is along the lines of:

Legendary.nvim must be loaded before other plugins. We have a set of plugins, or plugins can be provided by the other plugin itself, at legendary.plugins.[plugin name]. This way, plugins can be looked up dynamically; that is, if they wanted to, nvim-tree could provide its own legendary.nvim integration by adding lua/legendary/plugins/nvim_tree.lua in their repo, just for an example.

The plugin is just a plugin-specific script that must handle registering its keymaps, commands, autocmds, and functions. It may set up autocmds to do so on BufEnter for example, or it may call some custom logic to determine the correct buffer to attach to.

So for this example, and nvim-tree / legendary.nvim plugin would look like:

User's config:

require('legendary').setup({
  plugins = {
    nvim_tree = true,
  }
})

File: lua/legendary/plugins/nvim_tree.lua

vim.api.nvim_create_autocmd('FileType', {
  pattern = 'NvimTree',
  once = true,
  callback = function()
        local   nvimtreemappings = require("nvim-tree.keymap").keymaps
        local   keystodisable    = vim.tbl_map(function(e) return e.key end, nvimtreemappings)
        -- Set up Legendary plugin keybindigns
        local keymaps = vim.tbl_map(function(e)
            -- just set up a description-only keymap
            local entry = {}
            entry.mode  = "n"
            entry[1]    = e.key        -- Key
            if type(e.key) == "table" then entry[1] = e.key[1] end
            entry.description = "Nvim-tree: " .. e.desc
            entry.opts = { buffer = bufnr, noremap = true }
            return entry
        end, nvimtreemappings)
        require('legendary').keymaps(keymaps)
  end
})
hinell commented 1 year ago

@mrjones2014 No need for a copy. Replied in the original issue/thread.

mrjones2014 commented 1 year ago

Yeah I meant if nvim-tree would be interested in having the plugin in this repo

alex-courtis commented 1 year ago

Copying my comment here in case you are interested in helping me figure out how an implementation would work with an upstream plugin: mrjones2014/legendary.nvim#289 (comment)

Left some notes about on_attach callback and future direction of mappings: https://github.com/mrjones2014/legendary.nvim/issues/289#issuecomment-1367680904

Yeah I meant if nvim-tree would be interested in having the plugin in this repo

I'm not quite sure exactly what you mean but we are always open to new ideas.

Well, it can, but the following is not guaranteed to be stable:

Mappings are considered to be API and will never change, only be added to. :help nvim-tree-default-mappings is a source of truth: it is always updated with DEFAULT_MAPPINGS.

Is that enough, considering the future in which view.mapping will be deprecated in favour of an on_attach callback where you have full control, with all mappings invoking public API?

mrjones2014 commented 1 year ago

Basically what we would need on the legendary.nvim side is a way to access an up-to-date list of the keymaps (including/considering user config of nvim-tree) with descriptions.

That sounds like what require("nvim-tree.keymap").keymaps is, is that correct?

So a legendary.nvim / nvim-tree plugin integration would look like legendary.nvim wrapping the on_attach function, calling the users' first, then calling our own to peek at the keymaps the user may have set.

alex-courtis commented 1 year ago

That would work nicely: we have a solution for now and a solution for the future.

Let's let the DEFAULT_KEYMAPS keymaps out via API. Create a deep copy of DEFAULT_KEYMAPS instead of returning the actual defaults.

We can also expose the current keymaps in the same manner. Not sure of a use case, but costs little.

We could expose the current mappings for modification however setting up the callbacks would be tricky. Would require a valid use case.

mrjones2014 commented 1 year ago

We can also expose the current keymaps in the same manner.

I think this is all that would be needed. These would be set after the user's on_attach, correct? So we could wrap that function to get their configured keymaps.

alex-courtis commented 1 year ago

I think this is all that would be needed. These would be set after the user's on_attach, correct? So we could wrap that function to get their configured keymaps.

That would work nicely. You can enumerate all their mappings for the buffer.

One difficulty might be identifying which mappings are nvim-tree specific. A convention approach may be necessary via, say, a prefix in the description. That will be available for the default mappings.

mrjones2014 commented 1 year ago

Couldn’t I use require("nvim-tree.keymap").keymaps?

One other thing is actually hooking into on_attach because if the user config loads after our plugin, they’d overwrite our on_attach. Could you possibly do a doautocmd User NvimTreeAttached that we could hook into?

alex-courtis commented 1 year ago

Couldn’t I use require("nvim-tree.keymap").keymaps?

Yes, however that is not API and may be subject to change e.g. refactor.

We will add API for future reliability.

One other thing is actually hooking into on_attach because if the user config loads after our plugin, they’d overwrite our on_attach. Could you possibly do a doautocmd User NvimTreeAttached that we could hook into?

That's really interesting... please elaborate on how that will work.

Adding an event is a possibility :help nvim-tree-event

mrjones2014 commented 1 year ago

That's really interesting... please elaborate on how that will work.

I was originally thinking that we’d literally wrap the on_attach function, which would mean if the user then called setup with their own on_attach, ours would be overwritten.

But actually I think we could just use the autocmd I described in my original comment (unless something more specific to nvim-tree is added):

vim.api.nvim_create_autocmd('FileType', {
  pattern = 'NvimTree',
  once = true,
  callback = function()
        -- assuming this autocmd runs after the user’s on_attach
        -- we can grab the keymaps here, but we’d need a stable API to do so
        require('legendary').keymaps(keymaps)
  end
})

However I think the FileType autocmd would run before the user’s on_attach so I think we’d need a custom event. So, whatever code internally calls the user’s on_attach function should then do vim.cmd.doautocmd(‘User NvimTreeAttachPost’) and then on the legendary.nvim side, we’d do:

vim.api.nvim_create_autocmd('User', {
  pattern = 'NvimTreeAttachPost',
  once = true,
  callback = function()
        -- this event should fire immediately after the user’s on_attach function
        -- grab the user’s configured keymaps from a stable nvim-tree API
        require('legendary').keymaps(keymaps)
  end
})
alex-courtis commented 1 year ago
    api.events.subscribe(Event.OnAttachPost, function(data)
      -- not sure what data we could provide
      require('legendary').keymaps(keymaps)
    end)

We would not need to concern ourselves with once etc. as this would always be run once after the user's on_attach

I was originally thinking that we’d literally wrap the on_attach function, which would mean if the user then called setup with their own on_attach, ours would be overwritten.

Alternative: user provides legendary with their on_attach, which legendary calls in the "real" on_attach.

alex-courtis commented 1 year ago

This change breaks down into three parts:

I'll split the last one into a new issue if you're OK with the proposal.

mrjones2014 commented 1 year ago

Yeah, that sounds good. That’s exactly what we’d need for a proper integration

alex-courtis commented 1 year ago

1869

alex-courtis commented 1 year ago

@hinell @mrjones2014 I would be grateful if you tested the new API:

cd /path/to/nvim-tree.lua
git pull
git checkout 1858-add-current-default-mapping-api

:help nvim-tree.api.config.mappings.current()

mrjones2014 commented 1 year ago

Working on testing this today.

mrjones2014 commented 1 year ago

This works. I left several comments about the events API, but this keymapping API works great.

hinell commented 1 year ago

@alex-courtis Why not to export callbacks? This fails for me on NVIM v0.9.0-dev-5d5fa8 if used via Legendary api.

The actions on nvim-tree stop working both from keyboard and from Legendary. In the latter case this might be because callbacs for keys aren't exported. I request to export them explicitly.

local keys = require("nvim-tree.api").config.mappings.default()
print(vim.inspect(keys)) =>
-- Prints :
  ...
  }
    action = "edit",
    desc = "open a file or folder; root will cd to the above directory",
    key = { "<CR>", "o", "<2-LeftMouse>" } 
  } 
  ...
mrjones2014 commented 1 year ago

In this case (description-only items added to legendary.nvim) legendary.nvim is just using vim.api.nvim_feedkeys(vim.api.nvim_replace_termcodes(keys, true, false, true), 't', true) so if they're mapped on the nvim-tree buffer they should work.

hinell commented 1 year ago

@mrjones2014 Well I've tested the https://github.com/nvim-tree/nvim-tree.lua/pull/1876 further.

mrjones2014 commented 1 year ago

Ah yeah this is due to the fact that nvim-tree deletes and recreates the buffer when opened/closed. That’s something we’ll need to resolve to make a proper integration.

alex-courtis commented 1 year ago

@alex-courtis Why not to export callbacks? This fails for me on NVIM v0.9.0-dev-5d5fa8 if used via Legendary api.

I've merged #1877 TreeAttachedPost which passes bufnr however I'm not quite sure if that's what you require here.

It doesn't work via on_attach = ... callback

That's concerning. What exactly isn't working? The call to Api.config.mappings ?

alex-courtis commented 1 year ago

Ah yeah this is due to the fact that nvim-tree deletes and recreates the buffer when opened/closed. That’s something we’ll need to resolve to make a proper integration.

1895

hinell commented 1 year ago

What exactly isn't working?

@alex-courtis The commands of the nvim-tree invoked via Legendary. This is because you didn't export action callbacks in the https://github.com/nvim-tree/nvim-tree.lua/pull/1876 PR.

For an example see the entry[2] = e.callback line in:

mrjones2014 commented 1 year ago

This is because you didn't export action callbacks in the https://github.com/nvim-tree/nvim-tree.lua/pull/1876 PR.

No, you don’t need the callbacks. You can just create description-only keymaps that will be triggered via nvim_feedkeys when selected from the legendary finder, and leave the actual handler up to nvim-tree.

hinell commented 1 year ago

@mrjones2014 Well I would certainly need them. Additionally there could be a separate plugin that would use them. This issue isn't strictly related only to Legendary...

alex-courtis commented 1 year ago

@alex-courtis The commands of the nvim-tree invoked via Legendary. This is because you didn't export action callbacks in the #1876 PR.

action_cb and mode are only present when the user has specified them in view.mappings.list.

Default mappings are handled internally: there is no action_cb.

The documentation can be clarified.

@mrjones2014 Well I would certainly need them. Additionally there could be a separate plugin that would use them. This issue isn't strictly related only to Legendary...

If mapping via feedkeys isn't sufficient we could add something like api.action.execute("collapse_all")

Please keep in mind that on_attach is the future direction and view.mappings.list will be legacy-only.

hinell commented 1 year ago

... the future direction and view.mappings.list will be legacy-only.

That's fine. I'm not using it anyway. Exported action callbacks would be handy in case I don't want to pollute global shortcuts space and only bind it locally, whether via Legendary or not.

As we know nvim-tree binds shortcuts globally. This way it may impact performance.

https://github.com/nvim-tree/nvim-tree.lua/blob/ccb6d8a518d32e22bf5874f90e6c22661a5d8b46/lua/nvim-tree/keymap.lua#L238-L246

alex-courtis commented 1 year ago

As we know nvim-tree binds shortcuts globally. This way it may impact performance.

That will go away. All the mappings: default and user will be buffer local and using API only.

Are we good to close this one? https://github.com/nvim-tree/nvim-tree.lua/issues/1895 is still in progress.

hinell commented 1 year ago

@alex-courtis Why didn't you export callbacks? I don't think anyone is going to be harmed.

alex-courtis commented 1 year ago

@alex-courtis Why didn't you export callbacks? I don't think anyone is going to be harmed.

We don't actually use direct callbacks for the default actions; the logic for dispatching default actions is a little odd: https://github.com/nvim-tree/nvim-tree.lua/blob/949913f1860eb85024fa1967dbd89ac797777b0d/lua/nvim-tree/actions/dispatch.lua#L85

Creating callbacks for defaults is a step sideways.

A better solution is to set the callbacks to API, which is used for on_attach / direct mappings.

I'll have a go and see how it works out.

What's your timeframe on this? Finishing and launching on_attach #1579 should make the problem go away.

gegoune commented 1 year ago

@alex-courtis if one would expect all actions internally use API, so default actions could be easily amended by users, would that expectation be invalid?

alex-courtis commented 1 year ago

@alex-courtis if one would expect all actions internally use API, so default actions could be easily amended by users, would that expectation be invalid?

That sounds reasonable: use case: user retrieves a default action, puts that in their mapping list with a different key and calls setup again. A non-public-API wrapper would suit rather than a full migration to public API.

I will have to time-box this to 1 hour as my time is limited. #1549 is the end-state solution that needs work.

Edit: this is more complex RE node injections and possiblilty of failure. To cover the use case of ammending default actions the user can simply leave action_cb nil and set action only. This matches the current mechanism of setting mappings.list.

hinell commented 1 year ago

A better solution is to set the callbacks to API

A lightweight wrapper around action callbacks (ACBs) sounds good. Feel free to close this issue. If something is broken or there is a specific use case for direct ACBs - I will let you know.

What's your timeframe on this?

I'm NOT using on_attach. I'm using the Legendary plugin API for autocmd that automatically binds both autocmd and buffers' keymaps (shortcuts).

alex-courtis commented 1 year ago

A lightweight wrapper around action callbacks (ACBs) sounds good. Feel free to close this issue. If something is broken or there is a specific use case for direct ACBs - I will let you know.

That is the Right Thing To Do. It's actually done here properly via API, just not yet released.

Closing this issue... we will all get to our planned end state eventually.

mikehaertl commented 1 year ago

@alex-courtis I see that the methods to fetch the default mappings that were only added recently (https://github.com/nvim-tree/nvim-tree.lua/commit/bac962caf472a4404ed3ce1ba2fcaf32f8002951) are now already deprectated again (https://github.com/nvim-tree/nvim-tree.lua/commit/74959750f7823d6e069d3948a645f3c7a4c00638).

I've also seen the new migration guide for mappings. As a new user I actually only want to override 1 single mapping so there's nothing to migrate for me.

But still it seems like the new plan is that in that case you still have to copy all the default mappings into your config. This would extremely bloat my config file and feels a bit weird.

Is there really no way to apply the defaults (e.g. by calling some api method in on_attach) and only add your overrides afterwards?

mikehaertl commented 1 year ago

@alex-courtis From looking at the current code here's the solution that almost works for me:

  on_attach = function(bufnr)
    local api = require('nvim-tree.api')
    local keymap = require('nvim-tree.keymap')
    keymap.default_on_attach()
    vim.keymap.set('n', '+', api.tree.change_root_to_node, { desc='nvim-tree: CD', buffer=bufnr, noremap=true, silent=true, nowait=true})
  end

Only drawback: Help for default mappings is lost (no idea why that is). I'm also aware that nivm-tree.keymap.default_on_attach() is not part of the official API.

So what is your suggestion for my use case? Shouldn't something like this be added to the API?

mrjones2014 commented 1 year ago

I see that the methods to fetch the default mappings that were only added recently (https://github.com/nvim-tree/nvim-tree.lua/commit/bac962caf472a4404ed3ce1ba2fcaf32f8002951) are now already deprectated again (https://github.com/nvim-tree/nvim-tree.lua/commit/74959750f7823d6e069d3948a645f3c7a4c00638).

This also makes the whole plan for legendary.nvim plugin integrations impossible again 😐

alex-courtis commented 1 year ago

Is there really no way to apply the defaults (e.g. by calling some api method in on_attach) and only add your overrides afterwards?

That is not present in the API. I'll get back to you.

Only drawback: Help for default mappings is lost (no idea why that is). I'm also aware that nivm-tree.keymap.default_on_attach() is not part of the official API.

You would need to pass bufnr to default_on_attach

alex-courtis commented 1 year ago

This also makes the whole plan for legendary.nvim plugin integrations impossible again neutral_face

Reading through earlier discussions.

All the information from api.config.mappings.active can be retrieved via nvim_buf_get_keymap.

The actions are present in the form of the callback, which can be nvim-tree API or user defined. The callback can be compared with the API to determine what it does.

That is not an answer, however I am confident we can come to a solution.

alex-courtis commented 1 year ago

Eager fetching of mappings, before nvim-tree is setup or opened, could be done:

I believe that telescope does something similar.

alex-courtis commented 1 year ago

Once again, the above is not an answer, just a possibility.

What are you doing with the mappings from the deprecated API that you can't do via nvim_buf_get_keymap?

If it's a matter of defaults vs active we can work something out.

alex-courtis commented 1 year ago

Only drawback: Help for default mappings is lost (no idea why that is). I'm also aware that nivm-tree.keymap.default_on_attach() is not part of the official API. So what is your suggestion for my use case? Shouldn't something like this be added to the API?

@mikehaertl see :help nvim-tree.api.config.mappings.default_on_attach()

Alternative Default Mappings

mikehaertl commented 1 year ago

@alex-courtis Works fine, thanks.

alex-courtis commented 1 year ago

@mrjones2014 @hinell following up:

What is still needed to achieve the desired legendary functionality?

mrjones2014 commented 1 year ago

Everything described here: https://github.com/nvim-tree/nvim-tree.lua/issues/1869

As well as a function to get the currently applied keymaps considering user config.

hinell commented 1 year ago

@alex-courtis Just give an example, like in this reply over here

alex-courtis commented 1 year ago

We can reactively enumerate the mappings via event: https://gist.github.com/alex-courtis/984e5b97cc2148778f5353305b911308

The interesting bit is the callback: it's an API function defined in the defaults / by the user OR a custom function mapped by the user.

I did just commit a fix f0a1c6a to ensure that the event is fired. You'll need to pull master.

However...

alex-courtis commented 1 year ago

... a better solution might be to proactively retrieve the mappings (default or user) via a scratch buffer: https://gist.github.com/alex-courtis/de9f5cdda08129e3da9127817d7ffe28

The example is mapped for ease of testing.

You'll obviously need to wait until nvim-tree setup has been called, however you have :help nvim_tree_events_startup to deal with that.