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

swap_backgrounds not working #108

Closed
arkrost opened this issue Jan 25, 2024 · 8 comments · Fixed by #113 or #115
Closed

swap_backgrounds not working #108

arkrost opened this issue Jan 25, 2024 · 8 comments · Fixed by #113 or #115
Assignees
Labels
Bug / Poor Colors Something isn't working, unusable or badly designed

Comments

@arkrost
Copy link

arkrost commented Jan 25, 2024

I'm using lazy.nvim. I found out that swap_background option is not working. I tried to swap them with on_palette option and figured out that it is called twice and therefore not working. I believe it is a cause why swap_backgrounds is broken.

    {
      'AlexvZyl/nordic.nvim',
      event = 'VeryLazy',
      config = function()
        local counter = 0
        require('nordic').setup({
          on_palette = function(palette)
            counter = counter + 1
            print(counter, palette.gray0, palette.black1)
            local gray0 = palette.gray0
            palette.gray0 = palette.black1
            palette.black1 = gray0
            return palette
          end,
          -- swap_backgrounds = true,
        })
        vim.cmd('colorscheme nordic')
      end,
    },

@5-pebbles
Copy link
Collaborator

5-pebbles commented Feb 22, 2024

Disclaimer: I have not checked if this is the case with your issue, but...

A lot of the options don't work if you run setup more than once (but some do).

extend_palette is now being called twice after #107, thereby calling on_palette twice.

Either you're right, and it's getting swapped twice, or it's being called with the default value of false and then not updating.

Again, I have not checked if I am right; this is just a guess...

The whole thing is a stateful mess courtesy of Lua, and I think it needs a major refactor.

  • I would be willing to refactor it but not if I need to split it up into a few dozen PRs, each of which will take weeks to merge.

@AlexvZyl
Copy link
Owner

Yes, pretty sure I messed up or misunderstand how state works in Lua.

Sorry for being silent, and thanks for sticking around @5-pebbles

I am going to sort it out this weekend.

@5-pebbles
Copy link
Collaborator

5-pebbles commented Feb 22, 2024

I would be happy to sort it out, however I don't like merge conflicts, so I have been waiting on #101.

@arkrost is right the values are being switched [color/init.lua - 23]:

    -- Swap background
    if O.swap_backgrounds then
        local gray0 = C.gray0
        C.gray0 = C.black1
        C.black1 = gray0
    end

This will be accidentally fixed in #101.

@5-pebbles
Copy link
Collaborator

5-pebbles commented Feb 22, 2024

So I just looked it up...

This returns a ref:

require("module")

This returns a clone:

require("module").value

In colors/init.lua you have:

local O = require('nordic.config').options

So after the first import that value is not being updated, which would also explain why the tests are running with the same config twice.

#101 is using this inside extend_palette:

function C.extend_palette()
    local options = require('nordic.config').options

So its getting a updated clone with each call.

Example: https://replit.com/@5-pebbles/LuaImportsAreHard#main.lua

AlexvZyl added a commit that referenced this issue Feb 24, 2024
@AlexvZyl
Copy link
Owner

AlexvZyl commented Feb 24, 2024

So I just looked it up...

This returns a ref:

require("module")

This returns a clone:

require("module").value``

Are you sure? I thought all tables are references?

@AlexvZyl AlexvZyl self-assigned this Feb 24, 2024
@AlexvZyl
Copy link
Owner

AlexvZyl commented Feb 24, 2024

I'm using lazy.nvim. I found out that swap_background option is not working. I tried to swap them with on_palette option and figured out that it is called twice and therefore not working. I believe it is a cause why swap_backgrounds is broken.

    {
      'AlexvZyl/nordic.nvim',
      event = 'VeryLazy',
      config = function()
        local counter = 0
        require('nordic').setup({
          on_palette = function(palette)
            counter = counter + 1
            print(counter, palette.gray0, palette.black1)
            local gray0 = palette.gray0
            palette.gray0 = palette.black1
            palette.black1 = gray0
            return palette
          end,
          -- swap_backgrounds = true,
        })
        vim.cmd('colorscheme nordic')
      end,
    },

Oh, yeah... That's not good. Sorry just properly read this. Thought the problem was something else this entire time.

@AlexvZyl
Copy link
Owner

Sorry for the delay. Should be fixed in the next merge.

@5-pebbles
Copy link
Collaborator

Are you sure? I thought all tables are references?

Yup your right, but what is it?

This import is definitely the changing factor, I tested that.

As far as I know only other thing that it could be, is changing the order of imports.

I don't even remember why I moved the import location but it must have caused me some issue.

I will open a issue for a refactor just to make the code more clear, so we can avoid issues like this in the future.

@AlexvZyl AlexvZyl mentioned this issue Feb 28, 2024
@AlexvZyl AlexvZyl added the Bug / Poor Colors Something isn't working, unusable or badly designed label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug / Poor Colors Something isn't working, unusable or badly designed
Projects
None yet
3 participants