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

utility function get_highlight breaks the plugin #22

Closed
atchim opened this issue Apr 25, 2022 · 7 comments
Closed

utility function get_highlight breaks the plugin #22

atchim opened this issue Apr 25, 2022 · 7 comments

Comments

@atchim
Copy link

atchim commented Apr 25, 2022

After commit 0b93d18, the plugin attempts to pass a table with the none key to nvim_set_hl function, which breaks the status line. I skimmed the code and I found the following piece in function get_highlight in module heirline.utils.

    -- ...
    for k, v in pairs(hl) do
        t[k] = v
    end
    t.fg = hl.foreground
    t.bg = hl.background
    t.sp = hl.special
    t.style = "none," -- The `none` field comes from here.
    if hl.underline then
        t.style = t.style .. "underline"
    end
    -- ...

Maybe the way that highlights were handled by this plugin is still not compliant to the nvim_set_hl function.

@rebelot
Copy link
Owner

rebelot commented Apr 25, 2022

Can you please report the error? style field should be cleared out by normalize_hl in heirline/highlights.lua

Edit: got it. I'll fix soon. For the moment, don't access the style when using this utility function.

@atchim
Copy link
Author

atchim commented Apr 26, 2022

The last commits now cause a stack overflow, which comes from recursive calls to __index. This happens in module heirline.utils, in the function get_highlight. The following code shows where the recursive calls occur.

    setmetatable(hl, {
        __index = function(t, k)
            if k == "fg" then
                return t.foreground -- Leads to stack overflow.
            elseif k == "bg" then
                return t.background -- This too.
            elseif k == "sp" then
                return t.special -- This too.
            else
                return t[k] -- And also this...
            end
        end,
    })

This is easily fixed by using the builtin rawget function, which allows direct access to data without metatable indirection.

    setmetatable(hl, {
        __index = function(t, k)
            if k == "fg" then
                return rawget(t, 'foreground')
            elseif k == "bg" then
                return rawget(t, 'background')
            elseif k == "sp" then
                return rawget(t, 'special')
            else
                return rawget(t, k)
            end
        end,
    })

By using the code above, I could fix the stack overflow bug, but by using the same configurations I used before 0b93d18 the colors in the status line are still broken. Seems that the usage of get_highlight utility function is not practical at this moment. May be a good idea to remove this function and its usage from the documents and leave this role to the user since they can use Neovim's builtin functions to achieve the same results.

@rebelot
Copy link
Owner

rebelot commented Apr 26, 2022

can you provide an example hl that gets you the error? after fix with rawget

rebelot added a commit that referenced this issue Apr 26, 2022
@atchim
Copy link
Author

atchim commented Apr 26, 2022

Expected:

1650961039

Got:

1650961833

I also get this warning about deprecated style field:

[Heirline]: style field is deprecated, use fields supported by nvim_set_hl. Example: hl = { fg = 'red', bold = true }

@rebelot
Copy link
Owner

rebelot commented Apr 26, 2022

I see you are using fennel. I cannot test with that and I cannot reproduce in lua.

hl table now needs to comply with nvim_set_hl. So style field is deprecated and style elements should be passed as Booleans, like { bold = true } instead of { style = "bold" }

please see #17 (comment)

Can you please test with a minimal config and report where it breaks?

@rebelot rebelot changed the title Bad use of nvim_set_hl breaks the plugin utility function get_highlight breaks the plugin Apr 26, 2022
@rebelot rebelot changed the title utility function get_highlight breaks the plugin utility function get_highlight breaks the plugin Apr 26, 2022
@atchim
Copy link
Author

atchim commented Apr 27, 2022

I realized that I was setting fields that doesn't work anymore as, for exemple, fg and style. This was the reason for the status line with broken colors. Thanks for fixing the get_highlight function! I use it in order to have a status line which inherits colors from the current color scheme, whatever it is.

@atchim atchim closed this as completed Apr 27, 2022
@rebelot
Copy link
Owner

rebelot commented Apr 27, 2022

Great! Btw, nvim_set_hl supports abbreviations for fg, bg and sp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants