Skip to content
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

[Feature]: plugin integrations #183

Closed
1 task done
mrjones2014 opened this issue Oct 27, 2022 · 23 comments · Fixed by #320
Closed
1 task done

[Feature]: plugin integrations #183

mrjones2014 opened this issue Oct 27, 2022 · 23 comments · Fixed by #320
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@mrjones2014
Copy link
Owner

Similar Issues

  • Before filing, I have searched for similar issues.

Description

Just toying with an idea, what if we could integrate with various other plugins that add default keymaps? For example diffview.nvim adds a bunch of keymaps when diffview is open, we could built an integration that would automatically show them in the legendary finder.

@mrjones2014 mrjones2014 added the enhancement New feature or request label Oct 27, 2022
@mrjones2014 mrjones2014 self-assigned this Oct 27, 2022
@mrjones2014 mrjones2014 added the help wanted Extra attention is needed label Oct 31, 2022
@doctoromer
Copy link

I tried to integrate it with packer.nvim and failed. My goal is to have keymaps of lazy-loaded plugins displayed in the legendary API before the plugin is actually loaded.
I tried to use the helpers module, but it clashes with packer's bindings for lazy loading.

@mrjones2014
Copy link
Owner Author

For now you should be able to manually add the keymaps to legendary. There isn’t a great way to do it automatically at the moment.

I need to put a lot more thought into this.

@doctoromer

This comment was marked as off-topic.

@mrjones2014

This comment was marked as off-topic.

@doctoromer

This comment was marked as off-topic.

@mrjones2014
Copy link
Owner Author

We’ll be starting with nvim-tree. My comment from #289:

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

hinell commented Jan 9, 2023

@mrjones2014 You may also want to take a look at how persisted.nvim deals with Telescope integration over here.

@hinell
Copy link
Contributor

hinell commented Jan 12, 2023

@mrjones2014 Minor feedback here. I think Legendary dont' needs a sophisticated module-resolving system akin to Telescope one. The best way to handle plugins IMO by registering a plugin table (object) with a meta information and init function that is provided by api instance e.g.:

   local legendary = require("legendary")
   legendary.plugins:add({
      name = "FooPlugin",
      init = function(api)
         api.register({ keymaps = { ... } })
      end
)

I have seen this approach many times in Web development tools.

@mrjones2014
Copy link
Owner Author

I disagree.

This doesn't allow the user to easily load only the plugins they want, unless you mean the user will be writing the snippet you gave above themselves, in which case I think that's just really un-ergonomic. This also doesn't easily allow other plugin authors to provide their own legendary.nvim plugin integrations.

I have seen this approach many times in Web development tools.

That's an extremely different use case.

@hinell
Copy link
Contributor

hinell commented Jan 12, 2023

@mrjones2014 Flexibility may be achieved both by consumers and plugin-writers:

    -- init.lua
    local PluginA = require("cool/legendary/extensionA")
    local PluginB = require("cool/legendary/extensionB")
    local PluginC = require("myExtensions/legendary/keymaps")
             
    legendary.plugins:add(PluginA)
    legendary.plugins:add(PluginB)
    legendary.plugins:add(PluginC)
    legendary.plugins:add({
        name = "foo-keymaps"
        init = function(legendaryApi) ... end
    })

@mrjones2014
Copy link
Owner Author

That's a lot less ergonomic than just being able to do:

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

It also potentially breaks lazy-loading setups.

Is there any actual downside to a module resolution system like I described above? I see only benefits, and I see many downsides with your suggested approach, with no benefit over the module resolution system.

@hinell
Copy link
Contributor

hinell commented Jan 12, 2023

@mrjones2014 Well this way it's too simplified. It's going to be a nightmare if two different modules use the same name. It also deprives user from a control over module system.

This way you also can't add plugins on the fly.

Upd: and I don't even know how you are going to sanely debug plugins cause this way of configuration is basically a black box

@hinell
Copy link
Contributor

hinell commented Feb 2, 2023

Any progress on this?

I have a question about package manager status intergration. Like, how would a third-party plugin check that Legendary.nvim is installed? In packer we can do:

local L = packer_plugins["legendary.nvim"]
if L and L.loaded) then
 ...
end

UPD: Turns out we can use pcall() like:

local ok, mod = pcall(require, "Legendary")

@mrjones2014
Copy link
Owner Author

Haven’t made any progress yet, I have been abroad in the Netherlands, so I haven’t had time to work on open source lately.

@mrjones2014
Copy link
Owner Author

Looking into this again, we also need: nvim-tree/nvim-tree.lua#1895

@mrjones2014
Copy link
Owner Author

We could technically do it by re-binding the keymaps in nvim-tree's on_attach but then we'd be creating duplicate keymaps every time you open nvim-tree.

@mrjones2014
Copy link
Owner Author

mrjones2014 commented Mar 20, 2023

This should be possible now, by setting the keymaps generated to have filters = { filetype = 'NvimTree' }

@hinell
Copy link
Contributor

hinell commented Mar 24, 2023

@mrjones2014 Please add docs & examples on how to implement extensions. Thanks.

Upd, seems like it's done via filters. Not very obvious....

@mrjones2014
Copy link
Owner Author

Filters are one of a few tools you can use. I consider the API unstable currently, as is noted in the docs. I’m still iterating in it, I will add thorough docs once I feel like it’s in a good place.

@airtonix
Copy link

airtonix commented Apr 23, 2023

@mrjones2014 Flexibility may be achieved both by consumers and plugin-writers:

    -- init.lua
    local PluginA = require("cool/legendary/extensionA")
    local PluginB = require("cool/legendary/extensionB")
    local PluginC = require("myExtensions/legendary/keymaps")
             
    legendary.plugins:add(PluginA)
    legendary.plugins:add(PluginB)
    legendary.plugins:add(PluginC)
    legendary.plugins:add({
        name = "foo-keymaps"
        init = function(legendaryApi) ... end
    })

That's a lot less ergonomic than just being able to do:

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

It also potentially breaks lazy-loading setups.

Is there any actual downside to a module resolution system like I described above? I see only benefits, and I see many downsides with your suggested approach, with no benefit over the module resolution system.

This is the composition vs configuration as convetion approach.

ruby on rails went down the latter. you'll end spending the rest of your life tweaking offical default configs due to n+inifity requests in tickets.

I mean, you should step back and rexamine what it really is you're trying to achieve:

  • cool plugin system?
  • a super god like config convention that's tricky: "lol i anticipated every possible edge case, so just put yourpluginname = true"
  • registering other plugins keymaps so the user has superior UX?

I dare say the actual goal here is :

registering other plugins keymaps so the user has superior UX?

In which case, have you discussed monkey patching the nvim keymap functions so you catch all and any keymaps? This seems entirely relevant to: #258

@mrjones2014
Copy link
Owner Author

a super god like config convention that's tricky: "lol i anticipated every possible edge case, so just put yourpluginname = true"

Tells me you have not bothered reading the documentation or look at the actual implementation that was merged, so that's super cool. Only a moron could think they've anticipated every possible edge case.

registering other plugins keymaps so the user has superior UX?

Yes that's the goal.

have you discussed monkey patching the nvim keymap functions so you catch all and any keymaps

This decreases control, it doesn't tell us the source of a keymap, and it doesn't allow users to have some but not all keymaps in their legendary.nvim config. Sometimes you don't want everything in the finder, just the stuff you don't use all the time or don't always remember the keymap or command.

Finally, don't be a dick 🙂

@hinell
Copy link
Contributor

hinell commented Apr 23, 2023

Sorry for bothering, but it if ever legendary should not be responsible for configuring plugins. this should always be done at require("plugin").setup(). I just came from Telescope.nvim and can already tell you how bad it is. It would be a major design flaw.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
4 participants