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

Avoid creating default keymaps #39

Open
naranj4 opened this issue Jun 26, 2022 · 6 comments
Open

Avoid creating default keymaps #39

naranj4 opened this issue Jun 26, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@naranj4
Copy link

naranj4 commented Jun 26, 2022

Personally, I tend to like to configure keymaps on my own (or have the ability to override defaults). The buffer-specific keymaps are fine, but the global mapping on - sometimes interferes (rarely) with my use of macros. I can contribute a PR, but I'll wait for feedback/discussion, since changing defaults might break other users' configs.

Possible solutions:

  • Adding a mapping table that gets used to setup mappings in dirbuf. To override defaults, simply set the default values to nil. This is similar to how many other neovim plugins handle mappings (see telescope, etc).
  • [PREFERRED] Add a config option to disable default mappings. Use the mapping table described above or an on_attach function (similar to how lspconfig, etc handles mappings) as mentioned in Feature: Ability to pass an on_attach function to set custom dirbuf buffer keymaps #38. This feels simpler? And I like the flexibility of an on_attach hook in general.
@elihunter173 elihunter173 added the enhancement New feature or request label Jun 29, 2022
@elihunter173
Copy link
Owner

I can see adding a default_mappings option to dirbuf.setup() as mentioned in your preferred solution.

But before changing anything, what are your thoughts on the current behavior where default mappings are only created if no mapping to the <Plug> mapping exists and they don't conflict with other mappings? (The second condition is only relevant for global mappings.)

@naranj4
Copy link
Author

naranj4 commented Jun 29, 2022

I’d be okay with that solution for buffer specific mappings. That sounds reasonable, but I’m not quite as sure for global mappings. If the user doesn’t want the mapping at all, and would prefer to just run :Dirbuf % instead, they’d want the global mapping to be completely disabled.

Admittedly, I’m biased towards letting the user configure mappings themselves since that’s always felt more “vim” to me 🤷

Maybe separate options for no_global_mappings and no_buffer_mappings?

@elihunter173
Copy link
Owner

If the user doesn’t want the mapping at all, and would prefer to just run :Dirbuf % instead, they’d want the global mapping to be completely disabled.

With the current behavior, a global keybindings can be disabled by remapping a key to itself, e.g. nnoremap - - to disable dirbuf.nvim's <Plug>(dirbuf_up) global mapping. This behavior was taken from vim-dirvish.

I do think this is a bit unintuitive and I'm currently leaning towards adding a default_mappings option which could be one of "all", "none", "global-only", and "buffer-only" with a default of "all". However, I'm torn then with this setting if default keybindings should always be created even if they clobber other keybindings or if the default keybindings should still be "polite" and only set themselves up if that mapping hasn't been taken yet (which global mappings currently do). I'm also not sure how I feel about changing the current behavior at all because currently keybindings can be disabled, even if it's somewhat unintuitive, and I want to avoid breaking configs in general.

@naranj4
Copy link
Author

naranj4 commented Jun 30, 2022

Yeah, I think that's fair and mainly why I wanted to discuss options. How about the following:

default_mappings = {
    global = "none" | "respect_user_mappings" | "all",
    buffer = "none" | "respect_user_mappings" | "all",
}

I think respect_user_mappings name needs to be massaged a bit and would need clear documentation for each of the options in the README, but perhaps this offers enough granularity? And you can use respect_user_mappings as the default for both fields?

@andrewferrier
Copy link

andrewferrier commented Jul 14, 2022

I can see adding a default_mappings option to dirbuf.setup() as mentioned in your preferred solution.

But before changing anything, what are your thoughts on the current behavior where default mappings are only created if no mapping to the <Plug> mapping exists and they don't conflict with other mappings? (The second condition is only relevant for global mappings.)

The problem with this approach is that it relies on the order in which plugins load, if two of them are trying to grab the same mapping. And that's not always predictable...

FWIW I would prefer a simple boolean of disable_default_mappings or similar.

By the way, I'm not sureon_attach is needed. A dirbuf.lua in after/ftplugins has worked well for me so far for setting other dirbuf-specific stuff. on_attach is most helpful for things like LSP/Treesitter which span many types of files, but the filetype for dirbuf is always predictable.

@textzenith
Copy link

textzenith commented May 17, 2023

The - keymapping is violence — and I say this as someone who loves dirbuf!

Global keymaps should be configured only by request, in a config option, never by default. I'd only make an excuse for plugins where the keymap is the entire point of the plugin — i.e. something like Surround or Sneak (or Unimpaired, or something that provide extra text objects).

When I first discovered this, I just disabled dirbuf for a while instead of trying to figure how to change the keymap. I mean, all it was doing was exploding my current buffer and producing a ton of error messages, anyway. Users who do this might not stick around to find all the good that the plugin has to offer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants