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

About improvement of user customization experience. #888

Closed
7 tasks done
misumisumi opened this issue Jul 22, 2023 · 20 comments · Fixed by #931
Closed
7 tasks done

About improvement of user customization experience. #888

misumisumi opened this issue Jul 22, 2023 · 20 comments · Fixed by #931
Labels
complexity:high High-risk, potential for delicate/cascading effects enhancement New feature or request

Comments

@misumisumi
Copy link
Collaborator

misumisumi commented Jul 22, 2023

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.

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

Additional information

No response

@misumisumi misumisumi added the enhancement New feature or request label Jul 22, 2023
@Jint-lzxy Jint-lzxy added the complexity:high High-risk, potential for delicate/cascading effects label Jul 22, 2023
@misumisumi
Copy link
Collaborator Author

misumisumi commented Jul 22, 2023

ccording to #885 (review) , an override that does not rely on LazyNvim is likely necessary.

  • Consider using the merge function or better

@fecet
Copy link
Contributor

fecet commented Jul 23, 2023

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

@Jint-lzxy
Copy link
Collaborator

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 #885 (review) 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
Copy link
Contributor

fecet commented Jul 23, 2023

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

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 #885 (review):

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

fecet commented Jul 24, 2023

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

Jint-lzxy commented Jul 24, 2023

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:)

  1. 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
Copy link
Collaborator

CharlesChiuGit commented Jul 24, 2023

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

fecet commented Jul 24, 2023

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

Jint-lzxy commented Jul 24, 2023

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

@Jint-lzxy
Could you tell us a little more about the following #885 (review).

this structure in #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
Copy link
Collaborator

Jint-lzxy commented Jul 25, 2023

Could you tell us a little more about the following #885 (review).

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

fecet commented Jul 25, 2023

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

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

Yeah it's possible.

@misumisumi
Copy link
Collaborator Author

misumisumi commented Jul 31, 2023

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

misumisumi commented Jul 31, 2023

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

misumisumi commented Aug 1, 2023

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

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

IMO we have to think a bit about overriding keymap.

In 372d3c9 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
Copy link
Collaborator Author

misumisumi commented Aug 6, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity:high High-risk, potential for delicate/cascading effects enhancement New feature or request
Projects
None yet
4 participants