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

Keybinding improvement proposal #1023

Open
1 task done
nicolasdumitru opened this issue Aug 11, 2023 · 22 comments
Open
1 task done

Keybinding improvement proposal #1023

nicolasdumitru opened this issue Aug 11, 2023 · 22 comments
Labels
feature Issues related to feature proposals. Please attach a module.

Comments

@nicolasdumitru
Copy link

Issues

  • I have checked existing issues and there are no existing ones with the same request.

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:

  • Neorg makes heavy use of the key, which is great because Neorg keybindings only work in .norg files. It simply makes a lot of sense.

  • Neorg keybindings seem to follow very sane mnemonics (I haven't learned all of them by heart yet, but looking at the file where they are defined, they certainly look good).

  • The set of default keybindings can be imported by using the core.default module.

The suboptimal:

  • Keybindings are poorly documented. You have to read and understand the source code to get a good grasp of what the keybindings are. This could be problematic for some people. The situation is understandable, as Neorg is new software and things could change shortly, but it's worth noting. There are tools like Telescope, which can show you all of your active keybindings, but ideally, it shouldn't be necessary to use anything extra.

  • Some of the keybindings use Alt or Control. This is suboptimal because, while these keybindings are only available when the currently edited buffer contains a .norg file, they can easily interfere with someone's other custom, maybe more generic or even file-agnostic keybindings. Personal example: I have Alt + Enter set to toggle a terminal with Toggleterm. It doesn't work when I edit .norg files.

  • Some of the keybindings are redundant. Example: normal mode Enter opens links. Neovim already had two keybindings that did this: gf for file links and gx for web links. In all fairness, the normal mode Enter works a little better (I can explain why, if necessary, but let's not get lost in details for the moment). Why not improve the already existing bindings when editing .norg files and instead make use of those for what they are already meant to do? Users would have to memorize fewer keybindings.

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.

@nicolasdumitru nicolasdumitru added the feature Issues related to feature proposals. Please attach a module. label Aug 11, 2023
@vhyrro
Copy link
Member

vhyrro commented Aug 11, 2023

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

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

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

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

vhyrro commented Sep 4, 2023

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

vhyrro commented Sep 4, 2023

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

champignoom commented Sep 4, 2023

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

nicolasdumitru commented Sep 5, 2023

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

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

vhyrro commented Sep 5, 2023

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

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

nicolasdumitru commented Sep 5, 2023

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

vhyrro commented Sep 5, 2023

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

vhyrro commented Sep 5, 2023

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

champignoom commented Sep 5, 2023

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

vhyrro commented Sep 5, 2023

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

@nicolasdumitru
Copy link
Author

nicolasdumitru commented Sep 6, 2023

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

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

champignoom commented Sep 6, 2023

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

nicolasdumitru commented Sep 6, 2023

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

@vhyrro
Copy link
Member

vhyrro commented Sep 6, 2023

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

@glyh
Copy link

glyh commented May 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Issues related to feature proposals. Please attach a module.
Projects
None yet
Development

No branches or pull requests

5 participants