Skip to content

Expose DEFAULT_KEYMAPS And Current Mappings #1858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hinell opened this issue Dec 24, 2022 · 56 comments · Fixed by #1876 or #2056
Closed

Expose DEFAULT_KEYMAPS And Current Mappings #1858

hinell opened this issue Dec 24, 2022 · 56 comments · Fixed by #1876 or #2056
Labels
API API or Event

Comments

@hinell
Copy link
Contributor

hinell commented Dec 24, 2022

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?

  • Well, it can, but the following is not guaranteed to be stable:
require("nvim-tree.keymap").keymaps

Note The callbacks should be exported explicitly!

@mrjones2014
Copy link
Collaborator

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)

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
Copy link
Contributor Author

hinell commented Dec 25, 2022

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

@mrjones2014
Copy link
Collaborator

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

@alex-courtis
Copy link
Member

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: mrjones2014/legendary.nvim#289 (comment)

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
Copy link
Collaborator

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
Copy link
Member

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.

@alex-courtis alex-courtis added API API or Event and removed awaiting feedback labels Dec 30, 2022
@alex-courtis alex-courtis changed the title Export default keymappings Expose DEFAULT_KEYMAPS And Current Mappings Dec 30, 2022
@mrjones2014
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

    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
Copy link
Member

This change breaks down into three parts:

  • expose default mappings
  • expose user mappings (useful only if the user called setup again)
  • mechanism for on_attach wrapping

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

@mrjones2014
Copy link
Collaborator

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

@alex-courtis
Copy link
Member

#1869

@alex-courtis
Copy link
Member

@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
Copy link
Collaborator

Working on testing this today.

@mrjones2014
Copy link
Collaborator

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

@hinell
Copy link
Contributor Author

hinell commented Jan 3, 2023

@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
Copy link
Collaborator

mrjones2014 commented Jan 3, 2023

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.

@alex-courtis
Copy link
Member

alex-courtis commented Jan 10, 2023

@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
Copy link
Contributor Author

hinell commented Jan 10, 2023

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
Copy link
Member

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
Copy link
Contributor

@alex-courtis I see that the methods to fetch the default mappings that were only added recently (bac962c) are now already deprectated again (7495975).

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
Copy link
Contributor

@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
Copy link
Collaborator

I see that the methods to fetch the default mappings that were only added recently (bac962c) are now already deprectated again (7495975).

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

@alex-courtis
Copy link
Member

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
Copy link
Member

alex-courtis commented Feb 27, 2023

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
Copy link
Member

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

  • create a noname new or scratch buffer
  • execute on_attach inside that buffer
  • grok the mappings
  • delete the scratch buffer

I believe that telescope does something similar.

@alex-courtis
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

@alex-courtis Works fine, thanks.

@alex-courtis
Copy link
Member

@mrjones2014 @hinell following up:

What is still needed to achieve the desired legendary functionality?

@mrjones2014
Copy link
Collaborator

Everything described here: #1869

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

@hinell
Copy link
Contributor Author

hinell commented Mar 7, 2023

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

@alex-courtis
Copy link
Member

alex-courtis commented Mar 13, 2023

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 alex-courtis reopened this Mar 13, 2023
@alex-courtis
Copy link
Member

... 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.

@hinell
Copy link
Contributor Author

hinell commented Mar 13, 2023

The best solution is just to export it as an ordinary piece of data. The most cheap and quick. Complexity isn't be justified over here.

@alex-courtis
Copy link
Member

The best solution is just to export it as an ordinary data piece. The most cheap and quick. Complexity wouldn't be justified over here.

That could be possible. What would you expect, in addition to nvim_buf_get_keymap ?

@alex-courtis
Copy link
Member

@hinell @mrjones2014

I would be most grateful if you tested an API addition:

cd /path/to/nvim-tree.lua
git pull
git checkout 1858-api-on-attach-mappings

:help nvim-tree.api.config.mappings.get_keymap()
:help nvim-tree.api.config.mappings.get_keymap_default()

This uses the scratch buffer approach and returns all the buffer locals applied by nvim-tree.

@mrjones2014
Copy link
Collaborator

I left one comment on the PR with a small implementation suggestion. But this works just fine.

Combined with this PR on legendary.nvim: mrjones2014/legendary.nvim#317

It should allow us to make a really nice integration with very little work on the user config side. We will be able to just grab the keymaps from nvim-tree and set them in legendary.nvim to be filtered by filetype = 'NvimTree'

@alex-courtis
Copy link
Member

It should allow us to make a really nice integration with very little work on the user config side. We will be able to just grab the keymaps from nvim-tree and set them in legendary.nvim to be filtered by filetype = 'NvimTree'

Great news! I look forward to using legendary nvim-tree on release.

@hinell
Copy link
Contributor Author

hinell commented Mar 24, 2023

Sorry, been busy in a while.

@alex-courtis Good job as always. Thanks. Please, make deprecation notice upon calls to default() and active() so we can migrate in the next tag:

print(vim.inspect(require("nvim-tree.api").config.mappings.default()))
-- => WARNING: this api is deprecated in favor of get_keymap_default()

I also think the way you rename the API is a worrysome. Renaming key to lhs in keymaps tables was rather a BREAKING change. Please, don't do this. Otherwise you break plugin too easily.

@mikehaertl Please document the way we implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API or Event
Projects
None yet
5 participants