About improvement of user customization experience.

misumisumi commented 1 year ago

Feature description

This is a suggestion regarding #885. It contains major changes across the board. Other suggestions welcome..

I'm looking for a way that doesn't change the original configuration too much. Work is done at misumisumi/nvimdots/add-user-custom-function.

[x] Implementing the load_plugin function
- [x] Extend default value when given value is a table
- [x] Replace completely when the given value is a function
[x] Support override for event.lua
[x] Support override for options.lua
[x] Support override for settings.lua
[x] Support override for keymap.lua

Additional information

No response

misumisumi commented 1 year ago

ccording to https://github.com/ayamir/nvimdots/pull/885#pullrequestreview-1542119001 , an override that does not rely on LazyNvim is likely necessary.

[ ] Consider using the merge function or better

fecet commented 1 year ago

Maybe we can take a look at https://github.com/LazyVim/LazyVim/tree/main, imitate its behavior or develop based on it.

Jint-lzxy commented 1 year ago

Maybe we can take a look at https://github.com/LazyVim/LazyVim/tree/main, imitate its behavior or develop based on it.

Hmm I don't feel like things are gonna work this way. Cause compared to LazyVim, we used a completely different loading strategy & directory hierarchy (implemented in #458, that's why we added a custom loader instead of using lazy.nvim's). Following LazyVim will definitely require a complete rewrite - but (at least in the current state) we don't want this config to become some sort of "Neovim distribution", where the base config and user config are completely separated (or are connected via settings entries) - which means incremental updates to plugins' specs are almost impossible (well, this is the case if the user did follow the instructions to customize their experience). See https://github.com/ayamir/nvimdots/pull/885#pullrequestreview-1542119001 for some thoughts on this.

IMO this is not that viable regarding our current situation, but we can further discuss its feasibility as well 😄

Quoting @CharlesChiuGit:

I once tried Lunarvim, but it also has it's own issue. One day u will still need to know how to fix bugs like this. The donw side of a highly pre-config nvimdot is that u can't be sure about wether the bug is from lunarvim, plugins or nvim itself.

cc @fecet

fecet commented 1 year ago

Maybe we can discuss what's the best way to do custom config based on current setup. For me I forked this and do rebase from time to time，I don't really like this as I often have to deal with various conflicts.

If would be better IMO if adding or removing plugins are happen at files level (not tables), but I guess there is some better approach

Jint-lzxy commented 1 year ago

If would be better IMO if adding or removing plugins are happen at files level (not tables), but I guess there is some better approach.

Yeah that's sort of what we're going to implement. Quoting https://github.com/ayamir/nvimdots/pull/885#pullrequestreview-1542119001:

\<We could...> wrap this as an API call like require("...").load_plugin("<name-used-for-require>", current_spec) where user configs will be automatically merged with parent specs, and if the passed user config is a function, it will completely replace its parent spec.

fecet commented 1 year ago

My point is, if we have to refactor the code anyway, we can choose to adopt the approach of lazyvim, which seems more user-friendly (for regular users) and easier to develop and we can get free upstrem update(they appear to be very activte). Otherwise, I would rather keep the current configuration.

One significant advantage of lazyvim I would like to mention is that it allows us to include some niche plugins as extras, and users can choose whether to import them or not.

I dont really understand why

incremental updates to plugins' specs are almost impossible

for neovim distributionn as I never use any of them, but lazyvim seems possible to do so?

Jint-lzxy commented 1 year ago

My point is, if we have to refactor the code anyway, we can choose to adopt the approach of lazyvim, which seems more user-friendly (for regular users) and easier to develop and we can get free upstrem update(they appear to be very activte). Otherwise, I would rather keep the current configuration.

Yeah FWIW we are planning to use similar logic compared with LazyVim. That is, "to separate user config from the base config to avoid merge conflicts (and users can get free upstrem update)". The thing is, our structure is different from LazyVim's. LazyVim is somehow similar to our previous structure ('functionality-oriented', a file may contain multiple plugins and everything is placed there), while we currently use 'plugins-oriented' approach (a file is treated as one basic unit for a single plugin, everything about that plugin is placed there) - configuring those plugins is roughly as follows:

If this is an existing plugin:

Create a new file with the same name as the parent spec in user/config, and add the options that should be passed to the setup() function there. If the user want to completely replace the parent spec, simply pass a function instead.

Example:

I want to modify the settings of specs.nvim (ui/specs.lua):

(Parent spec [in the future(?)]:)

return function()
    local opts = {
        show_jumps = true,
        min_jump = 10,
        popup = {
            delay_ms = 0, -- delay before popup displays
            inc_ms = 10, -- time increments used for fade/resize effects
            blend = 10, -- starting blend, between 0-100 (fully transparent), see :h winblend
            width = 10,
            winhl = "PmenuSbar",
            fader = require("specs").pulse_fader,
            resizer = require("specs").shrink_resizer,
        },
        ignore_filetypes = {},
        ignore_buftypes = { nofile = true },
    }

    require("...").load_plugin("specs", opts)
end

(Steps:)

Create user/config/specs.lua;

2.1. I want to set popup.delay_ms to 20:

-- Other configs... (new autocmds, usercmds, etc.)

return {
    popup = {
        delay_ms = 20,
    }
}

load_plugin will merge this spec with the parent spec, and then call require("specs").setup({final_spec})

2.2. I want to customize this plugin myself:

return function(opts) -- This is the parent spec in case the user want to have some references
    -- Other configs... (new autocmds, usercmds, etc.)

    opts.show_jumps = true
    -- OR (complete replacement) --
    opts = { show_jumps = true }
end

And load_plugin would instead do: require("specs").setup({opts})

2.3 I want to disable this plugin:

return nil

Nothing will happen then.

If this is a new plugin:

Similar to LazyVim:

Adding a plugin is as simple as adding the plugin spec to one of the files under user/plugins/*.lua. You can create as many files there as you want.

You can structure your lua/plugins folder with a file per plugin, or a separate file containing all the plugin specs for some functionality.
return {
  -- add symbols-outline
  ["simrat39/symbols-outline.nvim"] = {
    lazy = true,
    cmd = "SymbolsOutline",
    opts = {
      -- add your options that should be passed to the setup() function here
      position = "right",
    },
    -- OR --
    config = function()
     -- setup the plugin urself
    end
    -- OR --
    config = require("user.config.outlinexxx") -- This works as well
  },
}

@fecet Does this make things a bit clearer?

CharlesChiuGit commented 1 year ago

Yeah FWIW we are planning to use similar logic compared with LazyVim. That is, "to separate user config from the base config to avoid merging conflicts (and users can get free upstrem update)". ....

I think this would be really nice, I'm buying it lol

fecet commented 1 year ago

Yes I like this way. But I'm wondering why our config is plugins-oriented, doesn't our config also use a single file contains many configs, https://github.com/ayamir/nvimdots/blob/main/lua/modules/plugins/editor.lua looks very similar to https://github.com/LazyVim/LazyVim/blob/main/lua/lazyvim/plugins/editor.lua and can be easily migrated from one to another one.

I think the correct way is use functionality-oriented for some complicated and basic part like treesitter, lsp, dap and telescope as they are hard to seperate and doesn't need much modified and use plugins-oriented for other plugins so they are easier to manager for users and developers.

Jint-lzxy commented 1 year ago

I think the correct way is use functionality-oriented for some complicated and basic part like treesitter, lsp, dap and telescope as they are hard to seperate and doesn't need much modified and use plugins-oriented for other plugins so they are easier to manager for users and developers.

Sorry for the misleading wording - the main focus here is configs. LazyVim put the spec and its config together (making that a "comprehensive" file), but we separated them (each "config" file corresponds to exactly one specification (main plugin + related dependencies)). The thing is, that (comprehensive file) makes examining/editing the change history in the version control system cumbersome. It may happen that the same file is being edited all the time and consequently more challenging to understand what and when was fixed (+possible merge conflicts). We chose this structure b/c most of the time we work w/ configs rather than w/ plugin specifications 😄

misumisumi commented 1 year ago

@Jint-lzxy Could you tell us a little more about the following https://github.com/ayamir/nvimdots/pull/885#pullrequestreview-1542119001.

this structure in https://github.com/ayamir/nvimdots/pull/458 is the opts key would completely replace parent specs when passed as a function.

@fecet My PR is inspired by LazyNvim. Some changes made by the user are as follows. For example. override alpha.lua step.1 user/modules/plugins/ui.lua
local ui = {}
ui["goolord/alpha-nvim"] = {
enabled = false, # If you do not use plugin.
opts = require("user.modules.configs.ui.alpha"),
}
return ui
step.2 user/modules/configs/ui/alpha.lua
return function()
local alpha = require("alpha")
local dashboard = require("alpha.themes.dashboard")
require("modules.utils").gen_alpha_hl()

dashboard.section.header.val = {
    [[ your ascii art ]],
}

end


The current PR does not support full overrides.
You can pseudo-completely override by defining your own config.

Jint-lzxy commented 1 year ago

Could you tell us a little more about the following https://github.com/ayamir/nvimdots/pull/885#pullrequestreview-1542119001.

The key point is here (lazy.nvim/lua/lazy/core/plugin.lua):

elseif type(values) == "function" then
  ret = values(plugin, ret) or ret
  return type(ret) == "table" and ret or { ret }
end

Because we are using a custom loader, the relative order in which plugins appear internally (inside lazy.nvim) is not specified. That is to say, it is possible that the parent spec appears after its children, which means this would block out any user configs (if the parent spec is a function and that is indeed the case). If we are sure to adopt this PR's structure, we need to rewrite the loader (using lazy.nvim's preset).

fecet commented 1 year ago

Great! I feel like we are moving in the right direction. I still haven't find time to dig into lazyvim, is it possible to use our custom loader and import extras from lazyvim? So the config will look like

require("lazy").setup({
  spec = {
    -- add LazyVim and import its plugins
    { "ayamir/nvimdots", import = "nvim.plugins" },
    -- import any extras modules here
   { import = "lazyvim.plugins.extras.lang.typescript" },
    -- { import = "lazyvim.plugins.extras.lang.json" },
    -- { import = "lazyvim.plugins.extras.ui.mini-animate" },
    -- import/override with your plugins
    { import = "plugins" },
  },

I have many plugins that not suitable to add in main config but could be helpful for someone so it's good to have a extra part.

Jint-lzxy commented 1 year ago

is it possible to use our custom loader and import extras from lazyvim

Yeah it's possible.

misumisumi commented 1 year ago

I thought of a function that would allow the user to override the plugin's options without making major changes to the current configuration. If nil is given disable the plugin, if a table is given it will extend the default table, if a function is given it will replace it completely. I think this works fine for many plugins. However, like wilder.nvim, it doesn't work well if you set options other than opts after calling setup. load_plugin must be called last. The problem with this method is that you cannot disable plugins written in vimscript.

load_plugin()

---@param plugin_name string @Plugin name
---@param opts table @Default options for plugin
---@param isVimPlugin? boolean @If plugin is made by vimscript
---@param extraSetupFunc? function @When the plugin name that calls setup is different from `modules/configs/*.lua`
function M.load_plugin(plugin_name, opts, isVimPlugin, extraSetupFunc)
isVimPlugin = isVimPlugin or false
local ok, custom = pcall(require, "user.modules." .. plugin_name)
if ok then
    local setupFunc = extraSetupFunc or require(plugin_name).setup
    if custom == nil then
        setupFunc(false)
    elseif type(custom) == "table" then
        assert(
            not isVimPlugin,
            "This plugin is not made by lua, so define options in functions (probably using `vim.g.*`)"
        )
        opts = vim.tbl_deep_extend("force", opts, custom)
        setupFunc(opts)
    elseif type(custom) == "function" then
        local user_opts = custom()
        if type(user_opts) == "table" and not isVimPlugin then
            setupFunc(user_opts)
        end
    else
        error(
            "Please return `nil` if you disable plugin or `table` if you override config or `function` if you replace config completely "
        )
    end
elseif isVimPlugin then
else
    local setupFunc = extraSetupFunc or require(plugin_name).setup
    setupFunc(opts)
end
end

misumisumi commented 1 year ago

You can isolate your own plugins by adding two lines to pack.lua. If a user wants to include additional plugins, simply add them to user/modules/plugins.

    local get_plugins_list = function()
        local list = {}
        local plugins_list = vim.split(fn.glob(modules_dir .. "/plugins/*.lua"), "\n")
        local user_plugins_list = vim.split(fn.glob(user_modules_dir .. "/plugins/*.lua"), "\n")
        plugins_list = vim.list_extend(plugins_list, user_plugins_list)

Since the user definition is added to the end of the list, you can also disable the plugin using the following method. nil conditionals in load_plugin may not be necessary. You can also include it in the table below if you want to replace the config function entirely.

local ui = {}
ui["goolord/alpha-nvim"] = {
    enabled = false,
}
return ui

misumisumi commented 1 year ago

IMO we have to think a bit about overriding keymap. I simply implemented a method of overriding each item, but there is actually an order to load the keymap, so if you want to assign the key in another item to another command, you must include it in the file called later. plug. Want, all item files are combined in keymap/init.lua to return a table. Then load it with neovim after overriding the user settings.

misumisumi commented 1 year ago

I have implemented an override that does not significantly change the current file and directory structure. Could you please give me a review if possible? Details are misumisumi/nvimdots/add-user-custom-function. An example of actually overriding using this is misumisumi/nvimdots/my-config).

Jint-lzxy commented 1 year ago

IMO we have to think a bit about overriding keymap.

In https://github.com/ayamir/nvimdots/commit/372d3c9f7dab8db15b5b7268442b2b3584801515 I introduced a keymap module that can help overwrite existing keymaps. Maybe something can be done there? Since we could use user.keymap to overwrite those base keymaps after they are loaded.

misumisumi commented 1 year ago

In newly created branches, we have implemented a setting to isolate user overrides that do not make significant modifications to the current configuration. Therefore, I closed the previous PR (#885) and created a new PR (#931).

ayamir / nvimdots