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

Feat: Improve changing of palette & extended palette #124

Closed
wants to merge 21 commits into from

Conversation

5-pebbles
Copy link
Collaborator

@5-pebbles 5-pebbles commented Mar 25, 2024

Changes:

  1. on_palette is called twice once at the start and once at the end of extend_palette now build_palette.

This means you can change the base palette same as before and those changes will be propagated to the extend palette.

However you can also change the extended palette, previously that would just be overwritten, but with this second call the changes will be set again.

You can even use values in the extended palette just be aware that they will be nil in one of two calls:

Example:

on_palette = function(palette)
    palette.black1 = "#BF616A" -- make it red
    palette.bg = "#1E222A" -- but keep bg black
    palette.fg = palette.yellow.base -- how to ruin a color scheme in 3 lines
    return palette
end,

Example of how to implement #100

on_palette = function(palette)
    -- the if is needed because on_palette is called twice and one time palette.bg is nil which breaks blend
    if palette.extended then
        palette.bg_visual = require("nordic.utils").blend(palette.orange.base, palette.bg, 0.15)
    end
    return palette
end,

 

  1. override configuration option replaced with on_highlight similar to tokyonight's on_highlights:
require 'nordic' .setup {
    -- This callback can be used to override highlights before they are applied.
    on_highlight = function(highlights, palette)
    
        highlights.TelescopePromptTitle = {
            fg = palette.red.bright,
            bg = palette.green.base,
            italic = true,
            underline = true,
            sp = palette.yellow.dim,
            undercurl = false
        }
        
        return highlights
    end
}

 

Closes #100
Closes #101
Closes #121

feat(config)!: replace override with func on_highlight;

feat(extend): on_palette can override extended palette;

fix(README): update highlight override example;

fix(tests): add on_highlight to tests;

feat(deprecated): re add override option with deprecated warning;

feat(colors): make on_palette stateless;

fix(on_palette): variables in on_palette should now work;

feat(on_palette): add extended flag to palette;

docs(examples): update comments & README to reflect changes;

fix(build_palette): fix broken reload in stateless palette;

merge(fix_palette): into changing_palette;

BREAKING CHANGE: override config option removed;
`on_palette` is now called at the start and end of `extend_palette`.

**previously:**

If you set a color like `palette.bg` it would be overwritten by extend palette.

**now:**

It will be overwritten at the start and then set again at the end.
while still allowing changes to the base palette to propagate.
@5-pebbles 5-pebbles requested a review from AlexvZyl March 25, 2024 21:20
@5-pebbles
Copy link
Collaborator Author

There is one very notable problem with this:

saving a value from the original palette for latter use won't work...

on_palette = function(palette)
    local old_black = palette.black1
    palette.black1 = "#BF616A" -- make it red
    palette.bg = old_black -- this will end up red... which is kind of a big issue
    palette.fg = palette.yellow.base
    return palette
end,

I can certainly come up with a way around this. (not sure it will be the most simple solution) but would this PR without the side effects be the right thing.

@5-pebbles 5-pebbles self-assigned this Mar 26, 2024
Currently on_palette can irreversibly change the palette requiring a
full restart to fix.

This also means that implementations of on_palette have to account for
changes made in previous calls.

This commit fixes that by rebuilding the palette with each setup.

As such extend_palette has been renamed to build_palette.
Example:
on_palette = function(palette)
    local old_black = palette.black1
    palette.black1 = "#BF616A" -- make it red
    palette.bg = old_black -- now this will still be black
    return palette
end,
@5-pebbles
Copy link
Collaborator Author

Fixed it

@5-pebbles 5-pebbles marked this pull request as draft March 27, 2024 01:10
@5-pebbles
Copy link
Collaborator Author

5-pebbles commented Apr 3, 2024

Alright, I think this on_palette is the best option
Its mildly complicated if you are maintaining this project or if you are trying to do advanced stuff.
However it provides far more flexibility then anything else I can come up with and its really nice to use:

on_palette = function(palette)
    local old_black = palette.black1
    palette.black1 = "#BF616A" -- red
    palette.bg = old_black -- black
    palette.fg = palette.yellow.base -- yellow
    palette.visual = palette.fg -- yellow
    palette.bg_float = palette.black1 -- red
    return palette
end,

I will get to adding code comments one of these days.

& I could also try adding a section to readme but it would have to be kind of long... Maybe I could make the on_palette & on_highlight parts collapsible?

When you get the chance could you look at this @AlexvZyl

By providing a standard way to check if the palette is extended, we avoid people checking if an arbitrary color is nil. Which would be messy if we change the palette in the future.
I made the configuration example collapsible
& added a new section for on_palette.
@5-pebbles 5-pebbles marked this pull request as ready for review April 10, 2024 17:16
@AlexvZyl AlexvZyl changed the base branch from main to dev April 14, 2024 11:41
@5-pebbles 5-pebbles added the Enhancement New feature, request or suggestion label Apr 14, 2024
@5-pebbles
Copy link
Collaborator Author

@AlexvZyl when you get a chance can you review this and #125 ?

@AlexvZyl
Copy link
Owner

Sorry again @5-pebbles... Looking now.

Copy link
Owner

@AlexvZyl AlexvZyl left a comment

Choose a reason for hiding this comment

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

This is nice :)


-- This flag allows users to check if the extended palette can be used.
-- On the first run only the base palette is available.
C.extended = false
Copy link
Owner

Choose a reason for hiding this comment

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

I am not exactly sure what this is for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on_palette is called twice, but one of those times all values in the extended palette are nil.

Say I want to make bg_visual a blend of bg & orange.base this will panic:

on_palette = function(palette)
    -- the if is needed because on_palette is called twice and one time palette.bg is nil which breaks blend
    palette.bg_visual = require("nordic.utils").blend(palette.orange.base, palette.bg, 0.15)
    return palette
end,

because blend is passed a nil value (bg)...

So you would have to check if a value in the extended palette is not nil before preforming you operation extended just provides a standard way of doing that:

on_palette = function(palette)
    -- the if is needed because on_palette is called twice and one time palette.bg is nil which breaks blend
    if palette.extended then
        palette.bg_visual = require("nordic.utils").blend(palette.orange.base, palette.bg, 0.15)
    end
    return palette
end,

I am not sure if its the best solution because it adds a fair bit of complexity,

For reference, here's the section I added to the readme for on_palette:

An example of overriding colors in the base palette & extended palette:

require 'nordic' .setup {
    on_palette = function(palette)
        palette.black1 = "#BF616A"
        palette.bg = palette.yellow.base
        return palette
    end,
}

Internally the provided function is called twice, so you can safely use colors form the extended palette if you check that palette.extended is true.

Example of how to change the cursorline color to orange:

require 'nordic' .setup {
    on_palette = function(palette)
        -- If the palette is extended, we can safely use palette.bg
        if palette.extended then
            palette.bg_visual = require("nordic.utils").blend(palette.orange.base, palette.bg, 0.2)
        end
        return palette
    end,
}

Copy link
Owner

Choose a reason for hiding this comment

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

What if we added a third callback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if we added a third callback?

That's the other good option. It's definitely simpler:

on_palette = function(palette)
    -- the if is needed because on_palette is called twice and one time palette.bg is nil which breaks blend
    if palette.extended then
        palette.bg_visual = require("nordic.utils").blend(palette.orange.base, palette.bg, 0.15)
    end
    return palette
end,

vs

after_palette = function(palette)
    palette.bg_visual = require("nordic.utils").blend(palette.orange.base, palette.bg, 0.15)
    return palette
end,

Another example:

on_palette = function(palette)
    local old_black = palette.black1
    palette.black1 = "#BF616A" -- red
    palette.bg = old_black -- black
    palette.fg = palette.yellow.base -- yellow
    palette.visual = palette.fg -- yellow
    palette.bg_float = palette.black1 -- red
    return palette
end,

vs

on_palette = function(palette)
    palette.old_black = palette.black1
    palette.black1 = "#BF616A" -- red
    return palette
end,
after_palette = function(palette)
    palette.bg = palette.old_black -- black
    palette.fg = palette.yellow.base -- yellow
    palette.visual = palette.fg -- yellow
    palette.bg_float = palette.black1 -- red
    return palette
end,

It's up to you; a third callback is simpler, and twice-called on_palette is generally shorter.

Copy link
Owner

@AlexvZyl AlexvZyl May 27, 2024

Choose a reason for hiding this comment

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

Yeah, third callback is definitely the way to go.

@5-pebbles 5-pebbles marked this pull request as draft May 28, 2024 17:05
@5-pebbles
Copy link
Collaborator Author

I am splitting this PR into two parts; the first is #141, but I am going to leave this a draft for easy access until both are done and merged.

@5-pebbles 5-pebbles added the Won't Fix This will not be worked on, but is valid label Jun 1, 2024
@AlexvZyl
Copy link
Owner

AlexvZyl commented Jun 3, 2024

I think we covered everything?

@5-pebbles
Copy link
Collaborator Author

Yup, I just forgot to close it.

@5-pebbles 5-pebbles closed this Jun 3, 2024
@5-pebbles 5-pebbles deleted the changing_palette branch June 12, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature, request or suggestion Won't Fix This will not be worked on, but is valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: Improve changing palette and extended palette Cursorline option to set visual selection color
2 participants