mvllow / modes.nvim

Prismatic line decorations for the adventurous vim user
567 stars 14 forks source link

feat!: support replace mode, new colors and new improved scripting #59

Open mvllow opened 4 months ago

mvllow commented 4 months ago

This pull request is an internal refactor of modes, including support for replace mode, more predictable mode/operator detection, a shiny new palette, and builtin documentation.

These changes should not be breaking for most, but we have removed default ignored_filetypes as the detection of unlisted buffers seems to suffice.

Todo

Fixes #57

adriankarlen commented 4 months ago

I was playing around using the canary branch but for me this update breaks custom colors.

image

However it seems to always use the hl-group. so if the hl group is set it will overwrite default colors. but custom colors will still not be used

image

fitrh commented 4 months ago

I am currently in the middle of migrating to a new machine and I don't have any neovim setup at the moment so I can't test it.

By skimming the changes, everything looks good, regardless of my personal preferences to avoid using:

  1. The _G global variable
  2. The plugin/ dir pattern
  3. The vim.g.*_loaded pattern, in lua we have package.loaded[mod]
  4. The vim.validate usage, is known for its poor performance, lately in neovim core there have been several refactor to avoid vim.validate for simple type checking

Of course all of them are personal preference and not a bad thing, so feel free to ignore them

fitrh commented 4 months ago

Regarding documentation, I think it would be better if we could document the available highlight groups (Modes{Scene}{Part}) and which parts of the editor they affect, in my personal setup, the highlight groups are a powerful way to customize the appearance, but this can be done in a separate PR

mvllow commented 4 months ago

Really appreciate the feedback. I'll look into the custom colors not applying, that's something I simply haven't tried manually so thanks for bringing it up.

As far as your feedback, @fitrh, the global shenanigans were inspired by mini.nvim but I'm not attached to anything. I'll definitely remove vim.validate as that doesn't add much value after initial setup.

A large goal of this refactor was to help me regain a grasp of how modes even works because it's been a while since I've looked at it in depth and to allow easier disabling/enabling (especially the autocmds)

mvllow commented 4 months ago

& maybe we don't need to be able to configure colors via modes. We could have an example autocmd in the readme to modify the colors? Less abstraction is always my preferred way but I don't want to take away from UX either.

adriankarlen commented 4 months ago

I agree that using hl-groups is better. Perhaps something like this could be added to the readme image

mvllow commented 4 months ago

I think we will end up removing colours then. The only question is what the API should look like.

vim.api.nvim_create_autocmd("ColorScheme", {
  callback = function()
    -- With full control of fg/bg, using blend for the bg to replace `line_opacity`
    vim.api.nvim_set_hl(0, "ModesInsert", { fg = "blue", bg = "blue", blend = 15 })
    -- or only caring about the bg, leaving `line_opacity` as an option
    vim.api.nvim_set_hl(0, "ModesInsert", { bg = "blue" })
  end
})

Also, there seems to be a pretty bad performance regression—probably related to the autocmds. Haven't had time to debug but opening mini.files (presumably any ignored buffer) and navigating around brings nvim to a halt after a few seconds.

mvllow commented 3 months ago

Would package.loaded be a drop-in replacement @fitrh?

diff --git a/plugin/modes.lua b/plugin/modes.lua
index cc97d14..8e95b79 100644
--- a/plugin/modes.lua
+++ b/plugin/modes.lua
@@ -1,7 +1,7 @@
-if vim.g.modes_loaded then
+if package.loaded["modes"] then
    return
 end

-vim.g.modes_loaded = true
+package.loaded["modes"] = true

 require("modes").setup()

Edit: after reading :h package.loaded it seems that package.loaded is automatically set

Also, I'm curious why you are against enabling the plugin without explicitly calling setup? I quite like this pattern from standard vim and aside from convention I don't see any downsides—especially with package managers like lazy.nvim having an enable field.

Finally, once the colour handling is addressed/fixed the only remaining point I see is the use of _G. I can understand not wanting to pollute the global namespace and am not set on keeping this.

fitrh commented 3 months ago

Also, I'm curious why you are against enabling the plugin without explicitly calling setup?

There is no technical reason here, I just feel like that kind of behavior limits the user's freedom in how they want to load plugins (personally, for modes.nvim I would load it inside the ModeChanged autocmd and I don't need any plugin manager for that)

especially with package managers like lazy.nvim having an enable field

The dependency on package managers/package manager-oriented stuff is one thing I really don't like in this day neovim plugin ecosystem

It should be noted that I'm fine with all the changes, not against them, it's a matter of personal preference ❤️

fitrh commented 3 months ago

after reading :h package.loaded it seems that package.loaded is automatically set

Yes, we just need to check package.loaded

diff --git a/plugin/modes.lua b/plugin/modes.lua
index cc97d14..8e95b79 100644
--- a/plugin/modes.lua
+++ b/plugin/modes.lua
@@ -1,7 +1,7 @@
-if vim.g.modes_loaded then
+if package.loaded["modes"] then
    return
 end

-vim.g.modes_loaded = true

 require("modes").setup()
mvllow commented 3 months ago

There is no technical reason here, I just feel like that kind of behavior limits the user's freedom in how they want to load plugins (personally, for modes.nvim I would load it inside the ModeChanged autocmd and I don't need any plugin manager for that)

That’s fair. This is why I asked you—I only know my workflow so it’s good to hear more/different preferences and I respect yours :)

The dependency on package managers/package manager-oriented stuff is one thing I really don't like in this day neovim plugin ecosystem

Yea I definitely wouldn’t want to rely on a specific package manager. I assume setting package.loaded['modes'] = true could be a workaround to load modes on your own terms? I’m not sure on the load order there.

Thanks again for your input, I really do appreciate it