Rewrite Layer

[anuvyklack]

I needed the like Layer functionality and was about to white it myself, but found your plugin. But it wasn't working. I tried to amend it, but ended up completely rewriting it.

It was full of lacks:

• It didn't take into account, that api.nvim_get_keymap returns the output in the form api.nvim_set_keymap accepts. So if key is mapped to Lua function, in the table that api.nvim_get_keymap returns the rhs key will be absent, and desired Lua function will be under callback key. So I get an error, that rhs is nil.

• Your take into account only global key mappings forgot about buffer ones.

• Your check if layer is active was implicit, better make a boolean flag: it is easier to operate and harder to get confused.

• Your forbid to call Layer:unmap method if Layer is not entered and suggested to manually edit layer_keymaps property. It is ridiculous from the OOP side: your marked it as private and after that suggested edit it manually.

Also I replaced the keybindings style from:

{
    mode = {
        lhs = {
            rhs = rhs,
            silent = true,
            expr = true
        }
    }
}

to (just like in vim.keymap.set)

{
    mode = {
        lhs = { rhs, { silent = true, expr = true } }
    }
}

or (if no options needed)

{
    mode = {
        lhs1 = rhs1,
        lhs2 = rhs2,
    }
}

vim.keymap.set is now a standart and for users it will be more convenient.

And you use tabs instead of spaces, so I added the project config file with noexpandtab for those who use spaces by default.

P.S.: I advise you to drop vimscript support. You require Neovim 7.0, and I doubt very much that anyone comes to Neovim 7.0 to continue use vimscript. And it will be easier to maintain.

[Iron-E]

Thank you for taking the time to make changes in places you thought needed improvement! I'm happy this project interested you enough to contribute.

It didn't take into account, that api.nvim_get_keymap returns the output in the form api.nvim_set_keymap accepts. So if key is mapped to Lua function, in the table that api.nvim_get_keymap returns the rhs key will be absent, and desired Lua function will be under callback key. So I get an error, that rhs is nil.

Can you give a specific example for this?

Your take into account only global key mappings forgot about buffer ones.

Good point. I just pushed v3.1.1 to address this. I understand this probably creates a merge conflict (sorry!) but I figured while we work on these other issues I could at least publish a quick fix for buffer mappings now that I am aware of the issue.

Your check if layer is active was implicit, better make a boolean flag: it is easier to operate and harder to get confused.

Could you be more specific?

Your forbid to call Layer:unmap method if Layer is not entered and suggested to manually edit layer_keymaps property. It is ridiculous from the OOP side: your marked it as private and after that suggested edit it manually.

It is not recommended to edit the layer_keymaps property, and layer_keymaps is marked private for that reason. The error specifically states:

error("Don't call this function before activating the layer; just remove from the keymap passed to `Layer.new` instead.")

What this means is, instead of doing this:

local libmodal = require 'libmodal'
local layer = libmodal.layer.new(
{
	n =
	{ -- normal mode mappings
		gg = -- remap `gg`
		{
			rhs = 'G', -- map it to `G`
			-- other options such as `noremap` and `silent` can be set to `true` here
		},
		G = -- remap `G`
		{
			rhs = 'gg', -- map it to `gg`
			-- other options such as `noremap` and `silent` can be set to `true` here
		},
	}
})

layer:unmap('n', 'gg')

layer:enter()

Do this:

local libmodal = require 'libmodal'
local layer = libmodal.layer.new(
{
	n =
	{ -- normal mode mappings
		G = -- remap `G`
		{
			rhs = 'gg', -- map it to `gg`
			-- other options such as `noremap` and `silent` can be set to `true` here
		},
	}
})

layer:enter()

This is acceptable because you control what gets passed to libmodal.layer.new(). You lose control of the input after calling layer:enter(), at which point it would be reasonable to call layer:unmap().

Also I replaced the keybindings style from

Even though I like this change (and actually considered it for v3.0.0) it would be a breaking change for users who have been using libmodal.Layer since f84b0288e74fdc55e6e412744f1d07a9419b7ebf released in 2020. I will have to request that we revert this portion of the PR to maintain backwards compatibility. The same goes for the Vimscript— it's mostly just an autoload file which wraps around the Lua implementation to provide backwards compatibility with vim-libmodal.

---

I hope that I don't come off too critical here! I am in favor of some of the work you've done, particularly with the documentation and the project config. I would like to go point-by-point on the other things I mentioned previously to understand the motivation behind the changes before reviewing.

[anuvyklack]

It didn't take into account, that api.nvim_get_keymap returns the output in the form api.nvim_set_keymap accepts. So if key is mapped to Lua function, in the table that api.nvim_get_keymap returns the rhs key will be absent, and desired Lua function will be under callback key. So I get an error, that rhs is nil.

Can you give a specific example for this?

With use of plugin nvim-keymap-amend I have the next keymapping:

use 'anuvyklack/nvim-keymap-amend'

local keymap = vim.keymap
keymap.amend = require('keymap-amend')

keymap.amend('n', '<Esc>', function(original)
   if vim.v.hlsearch and vim.v.hlsearch == 1 then
      vim.cmd('nohlsearch')
   end
   original()
end)

try to set it and run the layer-simple example and press <Esc>. Even on 3.1.1

[anuvyklack]

Your check if layer is active was implicit, better make a boolean flag: it is easier to operate and harder to get confused.

Could you be more specific?

You check if layer is active with help of existence of existing_keymaps table instead of creating explicit self.layer_is_active flag.

"Explicit is better than implicit", you know.

[anuvyklack]

Even though I like this change (and actually considered it for v3.0.0) it would be a breaking change for users who have been using libmodal.Layer since f84b028 released in 2020. I will have to request that we revert this portion of the PR to maintain backwards compatibility. The same goes for the Vimscript— it's mostly just an autoload file which wraps around the Lua implementation to provide backwards compatibility with vim-libmodal.

I already implemented backward compatibility (for Layers). This pull request doesn't break anything.

[anuvyklack]

About Layer:unmap. I get everything your wrote, but it is a bad design decision. User creates an object and want to manipulate it. And you give him this possibility with Layer:unmap method. But you actually not. The Layer:unpam method instead of doing what it should: delete the key mapping form the layer object despite activated it or not, begins to teach user to create another object.

[anuvyklack]

And why do you try to invent the wheel with next snippet:

if self.existing_keymaps_by_mode then
    vim.notify(
        'nvim-libmodal layer: This layer has already been entered. `:exit()` before entering again.',
        vim.log.levels.ERROR,
        {title = 'nvim-libmodal'}
    )
    return
end

Lua is already has assert function.

[anuvyklack]

Your current version also does not working. You check the buffer keymappings only if user pass buffer option. But he might not do it. And in this case layer will overwrite the global key mapping. But if buffer key mapping exists if will overlap the layers keymap.

[anuvyklack]

And in general: you get existing_keymaps from api.nvim_get_keymap and try to restore them with vim.keymap.set. This simply does not work. They are incompatible between each other.

[anuvyklack]

My code is simply better. I have already thought about everything you are only going to.

[Iron-E]

You check if layer is active with help of existence of existing_keymaps table instead of creating explicit self.layer_is_active flag.

"Explicit is better than implicit", you know.

I suppose in that case we should add a Layer:is_active() method which just looks like this:

--- Check whether the layer has been `:enter`ed previously but not `:exit`ed.
--- @return bool
function Layer:is_active()
	return self.existing_keymaps_by_mode ~= nil
end

If we use a flag like self.active it has potential to go out of sync with what actually determines whether the layer is active.

I already implemented backward compatibility (for Layers). This pull request doesn't break anything.

Great! We won't have to worry about removing that part, then.

About Layer:unmap. I get everything your wrote, but it is a bad design decision. User creates an object and want to manipulate it. And you give him this possibility with Layer:unmap method. But you actually not. The Layer:unmap method instead of doing what it should: delete the key mapping form the layer object despite activated it or not, begins to teach user to create another object.

I still don't think it's necessarily a bad design decision, I just think it might be confusing given the fact :map allows mapping before :enter but :unmap doesn't. For that reason I'm inclined to agree that :unmap should allow unmapping regardless of state.

And why do you try to invent the wheel with next snippet:

if self.existing_keymaps_by_mode then
    vim.notify(
        'nvim-libmodal layer: This layer has already been entered. `:exit()` before entering again.',
        vim.log.levels.ERROR,
        {title = 'nvim-libmodal'}
    )
    return
end

Lua is already has assert function.

asserts are not generally for use within production code— they are mostly for unit tests (there are exceptions, example). In this case we don't want to assert because we are trying to communicate the state of the Layer to the user, and the assertion's error messages are unhelpful. For example:

assert(not self.existing_keymaps_by_mode, 'nvim-libmodal layer: This layer has already been entered. `:exit()` before entering again.')

Prints the following error:

E5108: Error executing lua [string ":lua"]:1: nvim-libmodal layer: This layer has already been entered. `:exit()` before entering again.
stack traceback:
	(the stacktrace)

While it gives a lot of helpful information to us as developers, this message is for the user, and what's really important for the user to see is buried in the middle of a lot of technical jargon. Compare that to what happens when I do vim.notify as above:

nvim-libmodal layer: This layer has already been entered. `:exit()` before entering again.

There's nothing here but what the user needs to see. This is a unique string too, so we could just :grep it if we really need to find where it came from in the project. Combine that with a plugin such as nvim-notify and you have a very nice-looking and simple message.

Your current version also does not working. You check the buffer keymappings only if user pass buffer option. But he might not do it. And in this case layer will overwrite the global key mapping. But if buffer key mapping exists if will overlap the layers keymap.

This is working as intended. See :h :map-buffer: "local buffer mappings are used before the global ones." This means that if we apply a global mapping through the Layer, we need buffer-local mappings that previously exist to still override those which were set by the Layer. This is the behavior of all mapping functions within vim (:map, vim.keymap.set, vim.api.nvim_set_keymap, etc) so I want Layer to continue to work the same way.

And in general: you get existing_keymaps from api.nvim_get_keymap and try to restore them with vim.keymap.set. This simply does not work. They are incompatible between each other.

They are partially compatible. It's just that nvim_get_keymap has more keys than vim.keymap.set so some need to get filtered out (which I forgot to do, thank you for pointing that out!) and certain key-codes are evaluated

[anuvyklack]

But all your words don't change the fact that the code just doesn't working.

Try this case

[Iron-E]

But all your words don't change the fact that the code just doesn't working.

Try this case

I installed nvim-keymap-amend to test but get no error running that :confused:

---

Here are the things that I am interested in merging from this PR:

• The documentation layout changes (I quite like these, it makes everything easier to read)
• The project .vimrc, since it enforces the whitespace style I prefer.
• The new {n = {gg = {'G', {silent = true}}}} style of mapping since it is in-line with vim.keymap.set/vim.api.nvim_set_keymap

These are things I don't want to merge:

• The solution for buffer-local mappings. I have already implemented fixes for this, and I don't want to introduce new bugs (e.g. buffer-local mappings being wiped while creating global mappings) or introducing a regression (e.g. that vim.schedule is required while unmapping, which I discovered while checking out this PR and have already addressed in v3.2.3). I appreciate you bringing the problems with my previous implementation to my attention, however!
• Moving the VimScript Examples, as I have linked to them online and don't want those links to go stale

If you discover further issues with my buffer-local implementation I welcome issues or follow-up PRs, I just want to focus the scope of this one.

[Iron-E]

Closing as stale, feel free to reopen.