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

feat!: massive keybinds refactor #1509

Merged
merged 29 commits into from
Jul 13, 2024
Merged

feat!: massive keybinds refactor #1509

merged 29 commits into from
Jul 13, 2024

Conversation

vhyrro
Copy link
Member

@vhyrro vhyrro commented Jul 8, 2024

Modernizing Keybinds

This pull request is a long-awaited refactor of the internal keybind system used by Neorg. In this refactor I seek to:

  • Make changing keybinds as convenient as possible
  • Introduce keybind presets (including a "vim" preset which makes full use of the vim keys for all operations this will require a different pull request to consider all of the vim-available keys)
  • Strip out large portions of the core.keybinds code, making startup faster
  • Make proper use of <Plug> mappings

<Plug> may seem like a weird choice, but it is the defacto way of mapping keys. This way, various modules can map their behaviours to a <Plug> object without worrying about who binds it and where. Plug mappings also allow us to check for keybind conflicts reliably. When you map your own keybind to a Neorg mapping, Neorg will no longer forcefully create its own keybind, completely eliminating conflicts.

All in all, this system will be much more ergonomic and will only require a vim.keymap.set from the user's end, everything else will just work!

TODOs

  • Refactor all modules to use the new system
  • Check all modules to ensure they still function as intended
  • Ensure that no other modules require core.keybinds
  • Reimplement mode support (like traverse-headings)
  • Fix user-overwritten keybinds not rebinding
  • Reimplement keybind hook (required for mode interoperability)
  • Allow external modules to extend the default keybind list, thereby respecting the default_keybinds option
  • Implement keybind presets
  • Verify that dot repeat works
  • Update all documentation to no longer reference core.keybinds
  • Make docgen work with keybind system for discoverability

@vhyrro vhyrro force-pushed the keybinds-refactor branch from 003a2ce to a1d7bb8 Compare July 8, 2024 21:02
@max397574
Copy link
Contributor

I think modes would be quite similar to what this plugin does https://github.com/shmerl/session-keys

Of course the optimal solution would be namespaced mappings but we don't know when will get those (if ever 😢 )

@vhyrro vhyrro force-pushed the keybinds-refactor branch from e71dfc4 to bd77711 Compare July 9, 2024 21:07
@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

Mode support is much more sophisticated now and should fix a lot of issues found during day to day usage. This is because modes can now have parents which they inherit behaviours like keybinds from.

@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

Note: general keybinds that operate outside of norg files will not trigger because neorg lazy loads itself. This should be mitigated by moving the core startup code to plugin/ and optimizing Neorg's startup time.

@max397574
Copy link
Contributor

just a thought I had
somehow hook into this for more default keybindings?
Thinking about neorg-telescope right now. Because currently it can't define keymaps for itself afaict
And we shouldn't just add them when telescope isn't available

@max397574
Copy link
Contributor

another thing is custom keybinds
so the user can just assign custom functions which aren't neorg keybinds to things (even if they don't define plug mappings)

you can just use normal keybinds because of the different modes

@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

I earlier alluded to reimplementing the keybind hook for interoperability, but instead settled on (in my opinion) a much better system. Keybind hooks are very limiting and unnecessary DSLs for our keybind system.

Users will have to set their keymaps in a buffer-local fashion either:

  • using .config/nvim/ftplugin/norg.lua, where they can freely use vim.keymap.set to override any key they'd like
  • using an autocommand defined somewhere in their configuration, inside of which they can also run vim.keymap.set

This will, by default, override that key for all Neorg modes. To remap a key in a specific mode only, one can simply query the current mode using require("neorg.modules").get_module("core.mode").get_mode() and conditionally run their mappings depending on the return value.

This approach is much more inline with the vim philosophy and much easier to adapt to any configuration style.

@max397574
Copy link
Contributor

the mode thing doesn't work though
because how would they know when the mode changes? afaik there currently is no way to listen to these events
perhaps trigger a user autocmd NeorgModeChanged with the new (and perhaps also old) mode as data?
would perhaps also be interesting for other future usecases

@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

If a mode change happens dynamically then yes there may be a problem, you're right. A user autocommand generated by the core.mode module sounds like a good approach.

(PS: there is already a way to listen to these events through neorg.callbacks, but a user autocommand is much more ergonomic)

@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

Actually, I'm highly considering nuking mode support. Shouldn't we integrate with e.g. hydra instead?

@max397574
Copy link
Contributor

max397574 commented Jul 10, 2024

hydra does much more than just different modes and seems overkill imo and not worth to add as a dependency
and of course the hope for mappings namespaces never dies

also hydra is not too well maintained it seems

as long as it's just optional I'm fine with this

@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

I never wanted to add it as a direct dependency. But integration with existing tools is much wiser than trying to reinvent the wheel in my eyes. It simplifies the existing code so much that I'd say it's worth pursuing.

@vhyrro vhyrro force-pushed the keybinds-refactor branch from 86c977b to f297ccd Compare July 10, 2024 12:56
@vhyrro
Copy link
Member Author

vhyrro commented Jul 10, 2024

Just documentation left now!

(and the docgen)

@vhyrro vhyrro marked this pull request as ready for review July 13, 2024 21:28
@vhyrro vhyrro requested review from mrossinek and danymat as code owners July 13, 2024 21:28
@vhyrro
Copy link
Member Author

vhyrro commented Jul 13, 2024

I don't believe I'm missing anything here. Should be ready to merge into main soon as a first test wave. If any of you are not pinned to the latest stable release then I encourage you to do so asap :p

@vhyrro vhyrro merged commit bfbe7b9 into main Jul 13, 2024
9 of 11 checks passed
@vhyrro vhyrro deleted the keybinds-refactor branch July 14, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants