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

refactor!: use vim.api.nvim_set_hl() #70

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gegoune
Copy link
Contributor

@gegoune gegoune commented Feb 1, 2022

Completely broken.


Edited by @andersevenrud

  • Update examples
  • Add a git message with migration help
refactor!: use vim.api.nvim_set_hl()

Introduces changes in color definitions which is a BREAKING CHANGE
if you use the `custom_colors` callback function option.

[INSERT INSTRUCTIONS HERE WITH A SIMPLE EXAMPLE]

Closes #69

@gegoune
Copy link
Contributor Author

gegoune commented Feb 1, 2022

Not sure why it's never going into table conditional in https://github.com/andersevenrud/nordic.nvim/pull/70/files#diff-87af52ac4b5511b195aa96dd5bdea0128de6210195d900717f6e55f58c004c66R111.

I am trying to make output from nordic functions to return objects that are ready to be injected into vim.api.nvim_set_hl().

@andersevenrud andersevenrud self-requested a review February 1, 2022 21:56
Copy link
Owner

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to touch the abstraction here. You can simply replace:

                vim.highlight.create(group[1], {
                    guifg = group[2] or 'NONE',
                    guibg = group[3] or 'NONE',
                    gui = group[4] or 'NONE',
                    guisp = group[5] or 'NONE',
                })

with:

                vim.api.nvim_set_hl(0, group[1], {
                    guifg = group[2] or 'NONE',
                    guibg = group[3] or 'NONE',
                    gui = group[4] or 'NONE',
                    guisp = group[5] or 'NONE',
                })

@andersevenrud
Copy link
Owner

Ignore that comment. I clearly misread something in the docs. Stand by...

@andersevenrud
Copy link
Owner

Well, not that far off 😅 I was unaware of the naming and that the gui attribute is now split into properties.

                local definition = {
                    fg = group[2] or 'NONE',
                    bg = group[3] or 'NONE',
                    sp = group[5] or 'NONE',
                }

                if group[4] then
                    definition[group[4]] = true
                end

                vim.api.nvim_set_hl(0, group[1], definition)

One thing I don't know about here is underline though, because that's an option, and I don't know if underline supports a non-bool value 🤷‍♂️ I can't find the definition of this in the docs for some reason...

@gegoune
Copy link
Contributor Author

gegoune commented Feb 1, 2022

Thanks! Will look into it tomorrow. But basically you are against changing group configurations the way I did in fidget.lua or standard.lua? Should make it easier for that very PR, but could get rid of those table[n] maps.

Thanks for looking into it!

@andersevenrud
Copy link
Owner

andersevenrud commented Feb 1, 2022

But basically you are against changing group configurations

Nope.

I just didn't understand the impact of using this function.

But now that I get it this should be fairly easy to do. The fastest way is to change the tables from this:

{ name | name[], fg, bg, style, sp }

to:

{ name | name[], { fg = '', bg = '', ... } }

Then finally:

    local function load_group(list)
        for _, group in ipairs(list) do
            if type(group) == 'function' then
                load_group(group(unpack(arguments)))
            elseif type(group[1]) == 'table' then
                load_group(vim.tbl_map(function(highlight)
                    return { highlight, group[2] }
                end, group[1]))
            else
                vim.api.nvim_set_hl(0, group[1], group[2])
            end
        end
    end

@andersevenrud
Copy link
Owner

FYI: I updated the description with some tasks

@andersevenrud andersevenrud self-requested a review February 1, 2022 22:49
Copy link
Owner

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the changes in the following reference is the best way to solve this considering the internal API for color definitions supports tables as names.

#70 (comment)

Basically, keep things as is, but put the stylings into a separate nested table instead of a completely flat table.

@andersevenrud
Copy link
Owner

andersevenrud commented Feb 1, 2022

There's actually an even simpler way for this, but I'm not sure how I feel about it because it involves using some black Lua magic to have a mix between a list and a map:

-- This works :o
local t = { "foo", fg = "bar" }
print(t[0])
print(t.fg)

Which means that technically one can do this:

{ name | name[], fg = '', bg = '', ... }
        for _, group in ipairs(list) do
            if type(group) == 'function' then
                load_group(group(unpack(arguments)))
            elseif type(group[1]) == 'table' then
                load_group(vim.tbl_map(function(highlight)
                    return { highlight, group }
                end, group[1]))
            else
                local definition = {}
                for k, v in pairs(group) do
                  if type(k) == 'string' then
                    definition[k] = v
                  end
                end
                vim.highlight.create(group[1], definition)
            end
        end

@gegoune
Copy link
Contributor Author

gegoune commented Feb 3, 2022

I will come back to it, but most likely not in next couple of weeks unfortunately.

@andersevenrud
Copy link
Owner

Alright. Take your time :)

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

Successfully merging this pull request may close these issues.

2 participants