search

lukoshkin / highlight-whitespace

Highlight unwanted whitespace and more in Vim/Neovim
MIT License
11 stars 2 forks source link

fix: remove invalid character names in `color`, to support `#RRGGBB` #2

Closed stevenxxiu closed 10 months ago

stevenxxiu commented 11 months ago

This gives support to #RRGGBB colors, such as Catppuccin's Latte flavor's background color of #eff1f5.

lukoshkin commented 11 months ago

Hey Steven! Thank you for your contribution. There are two remarks about the change you suggest:

  1. hyphen ('-') is also invalid char to use in a group name 
    -    local name = 'HWS_' .. color:gsub('[^a-zA-Z0-9_.@-]', '')
+    local name = 'HWS_' .. color:gsub('[^a-zA-Z0-9_.@]', '')
  2. Do we need to remove from the color name anything but '#'? I mean we usually use either named color or hex code. Are there other ways to specify color? It might be in gVim, but I am not sure 
    -    local name = 'HWS_' .. color:gsub('[^a-zA-Z0-9_.@]', '')
+    local name = 'HWS_' .. color:gsub('#', '')

    A user can set a color name that contains any char. However, it is better to warn that it will not work than removing invalid chars and ending up with not working highlighting (though there will be no errors in such case)

A better solution will be to strip '#' in core.lua file

# core.lua
+    local name = 'HWS_' .. valid_color(color)

And to check in init.lua user provided palette on valid colors with something like this function

# init.lua
+local function valid_color(color)
+  return api.nvim_get_color_by_name(color) ~= -1
+end

I think I can finish this pull request tomorrow. I was going to rework init.lua anyway

lukoshkin commented 11 months ago

@stevenxxiu, I can merge your pull request if you change it to

+    local name = 'HWS_' .. color:gsub('#', '')

Or I can merge the one below if you prove that it is better (e.g., by providing ways to set valid colors with '@' in the name)

+    local name = 'HWS_' .. color:gsub('[^a-zA-Z0-9_.@]', '')
stevenxxiu commented 10 months ago

Hi Vladislav!

The reason I changed this is due to your call to vim.fn.matchadd(). The first argument is a group name. Per Syntax - Neovim docs, the only valid group names are [a-zA-Z0-9_.@-]*.

Looking at https://neovim.io/doc/user/syntax.html#guifg, all the valid color names appear to work as group names, except the character #.

Which colors have a @ in them? I think my code works with @, as it only removes characters that aren't valid group names.

Or perhaps the group name can be generated separately from the color name?

lukoshkin commented 10 months ago

Yep, Syntax - Neovim docs say that the only valid group names are [a-zA-Z0-9_.@-]*. However, they might be out of date :( Since I tested group names containing dash char -, and Neovim complained about it. So experimentally [^a-zA-Z0-9_.@] is the right regex for substitution.

Anyway, I prepared an update that handles hex codes and wrong color group names. Sadly, but it means that it would have rewritten your PR completely, even if had merged it. But I'll think about how to say my thanks to you. Because I really appreciate your contribution!