ldelossa / litee.nvim

A framework for building Neovim plugins
409 stars 14 forks source link

Overriding nvim-web-devicon configuration #79

Closed levouh closed 2 years ago

levouh commented 2 years ago

The current icon configuration will override whatever the user has configured (at least in terms of highlights) for icons via what happens here. The highlight group names should be changed to not conflict with what users have already configured for icons (as DevIcon* is already "taken" by nvim-web-devicons itself). All of these would perhaps "ideally" be linked to already existing highlight groups setup by this plugin, however the list starts to get a bit ridiculous at that point with how many highlights the user has to setup. Perhaps this list could be trimmed a bit? Or, given that users have nvim-web-devicons setup, the configuration here could highlight def link LiteeDevIconX DevIconX to ensure that the groups get the same highlights. Another solution would just be to just have users call require("litee.nvim-web-devicons").setup directly, however that does not seem like the intended (nor ideal) behavior.

Either way, this seems like something that needs a fix. If the PR were simple, I'd be happy to take a stab, though I'm not entirely sure that it is.


As a side note, the cterm_color and name for this icon need to be fixed as well.

ldelossa commented 2 years ago

Setting up links like youre mentioning sounds like a good way to go.

The filetree stuff is new so admittedly the highlight stuff was just thrown in there with minimal effort to make it work.

Do you see defining links fixing all the issues you are currently mentioning?

levouh commented 2 years ago
local function get_highlight_name(data)
  return data.name and "DevIcon" .. data.name
end

local function set_up_highlight(icon_data)
  local hl_group = get_highlight_name(icon_data)
  if hl_group then
    vim.cmd(string.format("highlight def link Litee%s %s", hl_group, hl_group))
  end
end

That should work, however I'm not sure what happens in the scenario where the icons aren't loaded by the time that this plugin is loaded. Perhaps this plugin already enforces loading/setup of nvim-web-devicons implicitly, but just something to consider.

ldelossa commented 2 years ago

You raise a good point.

Ideally, litee has no dependency on having nvim-web-devicons installed.

Is it enough to just rename and remap all the "DevIcon" highlights litee.nvim sets to "LIDevIcon*" ?

You would not need to define each highlight, as I can loop thru the hacked-up local nvim-web-devicons.lua and set the highlights accordingly.

While doing this setup, we can also add a quick check, just to see if "DevIcon" highlight is defined, and if it is, perform a link. This would mean if someone did go ahead and set custom highlights for a bunch of "DevIcon" they would not need to repeat that work for litee.

We'd then comment a disclaimer saying "ensure you load nvim-web-devicons for this to work" somewhere in the plugin text.

I think that covers all our bases? 1) Overwriting "DevIcon" highlights will no longer occur as we'll move those to "LTDevIcon" highlights 2) If nvim-web-devicons is not installed you can adjust the defaults by defining "LTDevIcon" highlights 3) If you've spent time customizing "DevIcon" highlights to your liking, and want the same highlights in litee, just ensure you load nvim-web-devicons first and litee win inherit the icon highlights from nvim-web-devicons proper.

How does this sound?

levouh commented 2 years ago

Looking at the way it is used and taking a step back, is it really necessary to have nvim-web-devicons.lua in the repo. here? When using it for the file explorer is there a reason to not just require that the user have the plugin installed separately in order for it to work (or at least display icons correctly)?

A check could just be:

local ok, webicons = pcall(require, "nvim-web-devicons")
if not ok then
  vim.notify("nvim-web-devicons must be installed for file explorer to work", "warn")
  return
end

or whatever suffices, obviously icons could also be skipped if they correct plugin is not installed. Then this can be replaced with:

for _, icon_data in pairs(require("nvim-web-devicons").get_icons()) do

and this with:

icon = require("nvim-web-devicons").get_icon(node.filetree_item.is_dir and "dir" or node.name, nil, { default = true })

and the dir icon can be setup with:

require("nvim-web-devicons").set_icon({
  ["dir"] = {
    icon = "",
    color = "#6d8086",
    cterm_color = "108",
    name = "Directory",
  },
})

and finally this with:

require("nvim-web-devicons").set_up_highlights()

Seems a little bit "cleaner" (and as intended) but I'm not sure how well this fits your use case.

ldelossa commented 2 years ago

Looks like a solid way to go! Care to take the stab at the PR?

ldelossa commented 2 years ago

@levouh - it may actually make sense to hold on those changes. I have a major refactoring going on you can view here: https://github.com/ldelossa/litee.nvim/pull/80

We should probably address it in that pull vs in code that is about to change completely.

ldelossa commented 2 years ago

Okay @levouh I think I'm doing this correctly now:

https://github.com/ldelossa/litee.nvim/blob/15016d7f22506fafeb779724c44092a196aac37d/lua/litee/filetree/init.lua#L709-L724

and then setting up the syntax highlighting for the filetree buffer:

https://github.com/ldelossa/litee.nvim/blob/15016d7f22506fafeb779724c44092a196aac37d/lua/litee/filetree/init.lua#L688-L693

levouh commented 2 years ago

Sorry @ldelossa I completely missed these replies, I will try it out and let you know how it goes. Thanks for the fix, also I like the idea to split it out into separate repositories if e.g. I do not want to use the file tree.

ldelossa commented 2 years ago

@levouh no worries at all. Thanks, I agree the split is better. Lmk if any further issues with the devicons. Your suggestion was helpful 👍