nvim-neorg / neorg

Modernity meets insane extensibility. The future of organizing your life in Neovim.
GNU General Public License v3.0
6.51k stars 214 forks source link

Keybinding improvement proposal #1023

Open nicolasdumitru opened 1 year ago

nicolasdumitru commented 1 year ago

Issues

Feature description

Goal:

Improving the default set of keybindings to better follow the project's philosophy, namely goal 2, "Keybinds that make sense".

Why:

Because it could turn Neorg into a plugin that can better adapt to user needs and integrate into Neovim not only when it is otherwise vanilla, but also when it is heavily customized. It could save many users a lot of headaches. People who add Org mode to Neovim probably configure a lot of other things. Everyone likes using a plugin that plays nicely with the other ones, so this could potentially help drive Neorg adoption in the community.

Problem:

Neorg is great and provides a really powerful note-taking workflow, BUT some of the keybindings are suboptimal.

What I mean by all of the above: The good:

The suboptimal:

Something worth considering: Many plugins set all of their keybindings in the config and use the usual vim.keymap.set to do so. This has two advantages: users already know how to interpret this - it's unambiguous - and you can get a good look at your keybindings by just opening your config, instead of searching through a git repo online or a git repo that you technically have on your machine, but have no idea where it is because your plugin manager just hid it somewhere.

There is likely a very good rationale for choosing the current default keybindings. I don't know exactly what it is. It may be emulating Emacs Org mode, which is completely fine, or some other reason, but they are currently imperfect. In case the reason is emulating Emacs, I think an alternative, more transparent and easier-to-customize set of keybindings that users can opt for would be ideal for Neorg. Flexibility is one of Neovim's greatest strengths and it should be reflected in the way its plugins work.

Proposed solution:

Create an alternative set of keybinding presets - not necessarily a replacement, but something a user can easily opt-in to. Sticking to the project philosophy, are a few guideline ideas for this new set of keybindings:

  1. They should leverage already existing keybindings and improve them (gf or gx are among the prime candidates).
  2. As a general rule, they should only use , not Alt or Control.
  3. Ideally, Alt should never be used. Neovim doesn't have Alt keybindings by default, so people will likely choose to use it for their own; also, some GUIs have Alt bindings if I'm not mistaken. As an exception, Alt should only be used for actions that involve going back and forth and only when it is the only good option (i.e. no (sane) defaults exist and other options wouldn't make as much sense - I know this is vague, I can elaborate on it). This could include toggling stuff on and off (when it is imperative that this can be done quickly or when it would greatly improve the user experience). A personal example of this: my Toggleterm binding that I mentioned above and or Alt + hjkl for quickly navigating splits.
  4. Ideally, Control should never be used. Neovim binds some functionality to control, so this runs the (fairly high) risk of keymap collision. If it's not a problem now, future updates could create the problem.
  5. If the use of Control is unavoidable, Alt + Control is a better option, as it is much more uncommon. when Alt + Control should be used (for Alt binding variations).

Help

Yes, but I don't know how to start. I would need guidance

Implementation help

I could elaborate a specification for this new keybinding set, but I would likely need help implementing it. I don't know exactly what I'd need, as I've never contributed to any projects. I know that there is a section about contributing modules in the documentation, but from what I understand, it is a work in progress. I think I could manage this if I had a fairly solid tutorial and I could occasionally ask questions.

vhyrro commented 1 year ago

Hey! The points you raise are very solid and also essentially the same as what I would do to make the keybind system "perfect". In fact the keybinds module has been long due for a rewrite but has too low of a priority for anyone to currently bother.

Control is never used in Neorg, and never will be precisely because almost every key has a default vim binding with control pressed. The local leader key is always prioritized, alt is currently used only used for Alt + Enter which is an insert mode binding - a local leader would be incredibly inconvenient there. From what I recall a few months ago there has been a PR to Neovim which made Alt keybinds completely okay to use in the TUI context.

The idea of keybind presets is still on the TODO list for the module.

About how keybinds are chosen, first a mnemonic is determined and I try to fit it with the rest of the existing keybinds. If there are conflicts, generalize the mnemonics to allow for both to coexist. In cases like jumping to a link location, enter is a natural choice given the name of the key is literally called "enter". A keybind preset would allow users to choose vim-style keys or "neorg" keys. gx and gf might make sense to experienced neovim users, however they are not mnemonic and aren't a simple one click ordeal, which is why neorg currently binds both enter and gx/gf to make everyone happy. Unlike some tools like org-mode, anything that is part of any kind of opinionated workflow is put under the leader key, however anything that is considered "universal" (like continuing a list item, entering a link) are given faster, one-click keybindings. This way there is no clutter in the global keybind namespace.

Hope that answers some questions! I'll keep this issue open until I have time to improve the module :)

nicolasdumitru commented 1 year ago

I must say that I have thought about this and I have a few objections.

alt is currently used only used for Alt + Enter which is an insert mode binding This is true, but incomplete. Alt + Enter is also a normal mode binding - a different one too! You don't have to believe me, this can be easily checked.

gx and gf might make sense to experienced neovim users, however they are not mnemonic and aren't a simple one click ordeal Ok, maybe they aren't a single click ordeal, but they are mnemonic (Go to File/Go eXternal (or something like this)).

Also, how do you even set these keybindings? I tried to unmap the Alt + Enter one in my after directory with vim.keymap.del and it didn't work. I'd like to just make every keymap that I set override anything that plugins set by default in case of a collision.

max397574 commented 1 year ago

I'd like to just make every keymap that I set override anything that plugins set by default in case of a collision.

that's not possible here because of how the neorg keybindings work check out the wiki to see how to disable mappings https://github.com/nvim-neorg/neorg/wiki/User-Keybinds

nicolasdumitru commented 1 year ago

@max397574 first of all, that's invasive to say the least. Second of all, I find it hard to believe that it is actually "impossible" (they must have been set using some built-in Neovim functionality). Third, I don't want to disable mappings manually, as that either implies/means one of three things:

  1. a ton of work (if I want to only keep some of them)
  2. I get rid of all of them at once
  3. or I have to wait until something conflicts and interrupt my workflow to fix the conflict.

It's a poor solution at best. I'd like to know how to override them in a Neorg-agnostic way. Ideally, the keybinding's shouldn't even get set in the first place if they conflict with something that a user sets (everyone hates it when they install a plugin and it breaks some other functionality that they set themselves).

vhyrro commented 1 year ago

The reason they are being overwritten is likely because the keybinding module kicks in after you have deleted the keybind, reapplying it again. As mentioned the keybind system is currently icky because of how things had to be structured in the Neovim 0.5 era. They can be removed through the Neorg keybind hook (our specific syntax), or you can opt to completely stop Neorg from setting keybinds in case you want to go for a fully manual approach.

About the previous message!

This is true, but incomplete. Alt + Enter is also a normal mode binding - a different one too!

Right, I somehow forgot. That alt is a modifier for the "Enter" key, opening up a link within a split. Also a natural choice, additionally given that Alt + Enter in normal mode does not do anything within Neovim (or at least I see no mention in the help pages).

Ok, maybe they aren't a single click ordeal, but they are mnemonic

I strongly believe this is where presets will come in to save the day. Seasoned Neovim users want their keybinds in check, whereas new users want simplicity and easy-to-memorize shortcuts. There is no one-size-fits-all here unfortunately. As this part of the codebase gets rewritten and therefore matures and receives "neovim-idiomatic" and "simple" presets I'll be sure to include README snippets with more explicit information on how to tune the keybinds for whoever feels like doing so.

Alongside the rewrite we can also add a system that detects keybind clashes and informs of them, that's a good idea! Don't know how that never crossed my mind.

vhyrro commented 1 year ago

In the end, I'm trying to find a balance with the keybinds, which is already a very difficult task given the variety of users. Default keybinds that are more niche/"advanced" Neovim features to the average person may cause them to see the keybinds as deterring and worst case inconsistent (given that currently we have a distinction of optional modifiers (alt) + one key in the global keybind namespace or <LocalLeader> + key combination, this would create yet another category "a key prefixed by g").

This is why I really think the best way to go forward is to refine the current default keybinds (but keep simple things like Enter for links if they do not clash with user defined keybinds) and then provide a "very truthful to neovim" implementation for anyone who really requires it. Any thoughts on the approach?

champignoom commented 1 year ago

Maybe we could make the keybind config declarative. So the example on the wiki

["core.keybinds"] = {
    config = {
        hook = function(keybinds)
            -- Unmaps any Neorg key from the `norg` mode
            keybinds.unmap("norg", "n", "gtd")

            -- Binds the `gtd` key in `norg` mode to execute `:echo 'Hello'`
            keybinds.map("norg", "n", "gtd", "<cmd>echo 'Hello!'<CR>")

            -- Remap unbinds the current key then rebinds it to have a different action
            -- associated with it.
            -- The following is the equivalent of the `unmap` and `map` calls you saw above:
            keybinds.remap("norg", "n", "gtd", "<cmd>echo 'Hello!'<CR>")

            -- Sometimes you may simply want to rebind the Neorg action something is bound to
            -- versus remapping the entire keybind. This remap is essentially the same as if you
            -- did `keybinds.remap("norg", "n", "<C-Space>, "<cmd>Neorg keybind norg core.qol.todo_items.todo.task_done<CR>")
            keybinds.remap_event("norg", "n", "<C-Space>", "core.qol.todo_items.todo.task_done")

            -- Want to move one keybind into the other? `remap_key` moves the data of the
            -- first keybind to the second keybind, then unbinds the first keybind.
            keybinds.remap_key("norg", "n", "<C-Space>", "<Leader>t")
        end,
    }
}

would become something like:

["core.keybinds"] = {
    config = {
        normal_mode = {
            ["gtd"] = "<cmd>echo 'Hello!'<CR>",
            ["<C-Space>"] = function(modules) modules.broadcast_event("core.qol.todo_items.todo.task_done") end,
            ["undesired-binding"] = false,  -- disabled
        },
    }
}

In this way, keybinding conflicts can be detected easily, and one can disable undesired bindings easily as well.

EDIT: nvm I just realized that this is not where the keybindings are first registered.

nicolasdumitru commented 1 year ago

The reason they are being overwritten is likely because the keybinding module kicks in after you have deleted the keybind, reapplying it again

@vhyrro no, it doesn't. Indeed it kicks in even after the 'after' of the neovim config in some situations, but I have even tried manually unmapping it with :unmap <M-CR> while in a '.norg' file buffer. It doesn't work. This is why I am asking you about it. My best guess is that the module uses a non-standard (vim.keymap.set) underlying way of setting keymaps. Am I right?

you can opt to completely stop Neorg from setting keybinds in case you want to go for a fully manual approach.

A fully manual approach means that you are basically doing what the 'core.keybinds' module is supposed to elegantly do for you. At that point, why not just rewrite the thing and submit a pull request? Also, the fully manual approach doesn't always work, as I mentioned in #1051

Alongside the rewrite we can also add a system that detects keybind clashes and informs of them, that's a good idea!

Why write a system that detects keybind clashes when you can just let people's configs override them? Probably the only thing that would be more annoying than not having your keybinds work would be getting notified that something's wrong and having to use plugin-specific syntax to fix the conflict. Just imagine how unusable Neovim would be if every plugin did that.

I'd just like to say that the reason why I take the time to tell you about all of this is that I genuinely care about Neorg. It's the only desktop note-taking software that I've ever felt is worth using, but it's plagued by these weird quirks, not to mention that it's downright slow when loading. It takes ~1/3 of what lsp-zero takes and that's literally ~8-9 plugins.

max397574 commented 1 year ago

Why write a system that detects keybind clashes when you can just let people's configs override them?

because neorg has different modes where different keybindings exist they get created and changed while you edit files so if you'd just overwrite them they would just be overwritten again

vhyrro commented 1 year ago

@nicolasdumitru

The fact that unmap does nothing is particularly odd - I'll have to also test that on my own system. The keybinds module does in fact set keybindings via vim.keymap.set, however this is far from non-standard, in fact it's the preferred way of keybind management in lua, if that's what you meant in your message. The fact that it runs after any unmapping code in your init.lua (unless it's in after/) still applies :)

About clash detection - it's easy to say that we should simply allow user overrides, but this is far from under my control. It's up to the user's configuration and plugin manager to execute their keymaps after neorg is loaded. It's not possible for me to execute code before the user's configuration. About fixing with a specific syntax, I believed my message inplied that such a system would be put in place after the rewrite, which would already include many of the proposed fixes. Consider it a cherry on top, it would simply issue a warning. They may be annoying, but they are much better than ignoring the clash entirely and emulating what is basically a silent undefined behaviour imo!

I'd just like to say that the reason why I take the time to tell you about all of this is that I genuinely care about Neorg

I appreciate it! The biggest blocker for fixing these things immediately is time. As I mentioned in either this thread or another keybind related one, the issues here are simply lower priority than others that I have to spent time fixing/implementing. It will take a while before the keybind module gets fleshed out, but it will eventually. If anyone reading has the spare time to improve or rewrite the keybinds module to be more minimal amd modern then feel free to do so!

nicolasdumitru commented 1 year ago

The fact that unmap does nothing is particularly odd - I'll have to also test that on my own system

Please do, and please tell me if you can reproduce the weird behavior. I'm really curious to see what results you get.

The keybinds module does in fact set keybindings via vim.keymap.set, however this is far from non-standard

I don't think I've made myself clear. I was trying to say that vim.keymap.set is the standard/preferred way to set keybindings and that I had thought Neorg set keymaps with some other method. Considering that it sets keymaps in this standard way, how could it be that :unmap <M-CR> can unmap bindings that I set in my config, but not Neorg bindings?

The fact that it runs after any unmapping code in your init.lua (unless it's in after/) still applies :)

Again, I've tried unsetting it in after/ but it had no effect, which is very odd. At first I thought that the problem was that I am lazy-loading Neorg, but it wasn't. I've even tried sourcing the particular file in the function that I use to configure Neorg (to no avail).

If anyone reading has the spare time to improve or rewrite the keybinds module to be more minimal amd modern then feel free to do so!

I'd try to do it myself, but I haven't used lua for anything other than configuring programs like Neovim and I'm not familiar with Neovim's api. I'd need some guidance, but I'd be down to do it.

nicolasdumitru commented 1 year ago

screenshot-2023-09-05-15-38-27 screenshot-2023-09-05-15-39-09 @vhyrro just so that we are on the same page, this is how I tried to manually unmap it.

And I've even tried this: screenshot-2023-09-05-15-51-58

vhyrro commented 1 year ago

Just booted up my PC and oh, it does not see the commands with unmap on my machine either. This could be maybe because it's a buffer-local mapping? Give me a few minutes let me check out what's happening exactly.

Edit: It does indeed have to do with it being buffer local. Running :mapclear <buffer> clears them correctly, but of course also clears all other mappings along the way. To delete the specific keybind, running vim.keymap.del("n", "<M-CR>", { buffer = 0 }) works as intended!

vhyrro commented 1 year ago

It's quite weird that Neovim does not catch the mapping - logically it should also fall back to searching buffer-local ones in case it can't find the respective mapping in the global keybind namespace, but I assume the Neovim developers didn't want "magic" to occur within the function (i.e. taking the currently open buffer and looking through its local keybinds). The more you know! Also during the reimplementation it would be great to mention this in the wiki.

champignoom commented 1 year ago

@vhyrro you need to use the buffer argument for unmap as well: :unmap <buffer> <M-CR>

tested with NVIM v0.10.0-dev-1013+gbc43575c5

vhyrro commented 1 year ago

Awesome thanks, didn't know that was a thing, help pages are pretty weird for those commands :p

nicolasdumitru commented 1 year ago

@vhyrro @champignoom I'm back with some new bad news. :unmap <buffer> <M-CR> works, but there's another problem. It only works once and the Neorg sets the same keybinding again. In other words: if you run :unmap <buffer> <M-CR> and then press it will do what does and then Neorg will somehow set the bindings again (even if there was no link under the cursor to follow) and will become the "Jump To Link (Vertical Split)" keybinding again, leaving you back at square 1. I'd like to reiterate and say that this behavior is invasive. Not only does it override the after\ directory (which is, as far as I know, supposed to trump everything), it also overrides the commands that a user runs in a particular buffer BEFORE the user manually reloads the buffer.

max397574 commented 1 year ago

that's exactly what I meant when I told you that you can't simply unmap the keys

here

that's not possible here because of how the neorg keybindings work check out the wiki to see how to disable mappings https://github.com/nvim-neorg/neorg/wiki/User-Keybinds

and here

because neorg has different modes where different keybindings exist they get created and changed while you edit files so if you'd just overwrite them they would just be overwritten again

champignoom commented 1 year ago

The keybind system could indeed be more declarative tho, because if one has to use keybinds.unmap() to disable a keybind, the same keybind must have already been (destructively re-)mapped, without confirmation from the user.

nicolasdumitru commented 1 year ago

@max397574 I love this plugin but I'm speechless. Does that not slow Neorg down... a lot?

vhyrro commented 1 year ago

It does slow Neorg down a lot, which is exactly why a rewrite is due :D

glyh commented 6 months ago

I closed #1023 in favor of this. What I propose is to delete the Ctrl + Space binding and replace it with something else. This is because Ctrl+Space is used by most IMEs to call out the popup window. That is an essential feature for most non-europe/american users whose language has an alphabet that can't be mapped to 26 english alphas.