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

Fix: Handle table values in highlight options properly #428 #390

Closed
wants to merge 1 commit into from

Conversation

Debajyati
Copy link

This PR fixes the #428 issue

The plugins actually doesn't work properly in anywhere neither windows, nor WSL. It works only for the first time after the plugin installs(not just with volt theme switcher but also in case of Telescope themes).

The problem was in the init.lua inside lua/base46 directory of the base46 plugin.

The old tb_2str function -

M.tb_2str = function(tb)
  local result = ""

  for hlgroupName, v in pairs(tb) do
    local hlname = "'" .. hlgroupName .. "',"
    local hlopts = ""

    for optName, optVal in pairs(v) do
      local valueInStr = ((type(optVal)) == "boolean" or type(optVal) == "number") and tostring(optVal)
        or '"' .. optVal .. '"'
      hlopts = hlopts .. optName .. "=" .. valueInStr .. ","
    end

    result = result .. "vim.api.nvim_set_hl(0," .. hlname .. "{" .. hlopts .. "})"
  end

  return result
end

As far as I understand, this function converts a highlight group table into a string of nvim_set_hl commands.
The problem is that some highlight options (like link) can have table values, and the current code tries to concatenate them as strings.

This is the original code, which tried to concatenate a table directly, which is not allowed in Lua.

So, I corrected the function definition to properly convert highlight table to string even if the tables has some table values as properties.
This is the new function definition -

M.tb_2str = function(tb)
  local result = ""

  for hlgroupName, v in pairs(tb) do
    local hlname = "'" .. hlgroupName .. "',"
    local hlopts = ""

    for optName, optVal in pairs(v) do
      local valtype = type(optVal)

      if valtype == "boolean" or valtype == "number" or valtype == "string" then
        local valueInStr = tostring(optVal)
        if valtype == "string" then
          valueInStr = '"' .. valueInStr .. '"'
        end
        hlopts = hlopts .. optName .. "=" .. valueInStr .. ","
      elseif valtype == "table" then
        -- For handling table values (e.g., "link") properly
        local valueInStr = "{"
        for k,v in pairs(optVal) do
          local vstr = type(v) == "string" and '"' .. v .. '"' or tostring(v)
          valueInStr = valueInStr .. tostring(k) .. "=" .. vstr .. ","
        end
        valueInStr = valueInStr .. "}"
        hlopts = hlopts .. optName .. "=" .. valueInStr .. ","
      end
    end

    result = result .. "vim.api.nvim_set_hl(0," .. hlname .. "{" .. hlopts .. "})"
  end

  return result
end

The new tb_2str checks for the table type and iterates through the table's key-value pairs, formatting them into a string representation suitable for nvim_set_hl.
Thus the PR has resolved the "attempt to concatenate local 'optVal' (a table value)" error and restores the functionality of theme switching and other features relying on highlight linking.

I have tested this code in both Windows 11 and WSL. Works like a charm.
Now base46 theme changing works with both Telescope and volt theme picker plugins.

Kindly review the changes whether this solution is viable for merging.
Thanks in advance,

2025-02-16.04-02-25.mp4

@siduck
Copy link
Member

siduck commented Feb 16, 2025

that code looks scary, If the table was a issue, why dont other users face is it? Only a bunch of windows users face it, I dont see windows users in my discord server facing the issue

@Debajyati
Copy link
Author

Debajyati commented Feb 16, 2025

that code looks scary,

Maybe because I don't write lua code in my day to day life (except for Neovim), my approach is NOT efficient. I don't know what you mean.
 

Only a bunch of windows users face it, I dont see windows users in my discord server facing the issue.

Maybe they aren't using latest version of plugin or IDK what lol maybe they didn't try different cases properly.

Hey, what I see is the volt theme switcher and :Telescope themes with the old tb_2str function indeed works for the first time changing theme after plugin installation. Afterwards, whenever you try you get the Error.
 

This is the scenario -

First time using After plugins installation -  
   | ------------------------------- |       ✔️      |  -----------------------------------------------------   |

   |  previously set colorscheme  |   ------->  |  Selected NVChad theme from :Telescope themes  |

   | with whatever theme             |                 |                       or Volt theme switcher UI                  | 
   | ------------------------------   |                 |  -----------------------------------------------------    |

Afterwards - 

  |  ---------------------------------------------  |       ❌         | ---------------------------------------------------------  |

  |    Any theme already set in chadrc.lua file   |   --------->  |  Theme trying to be selected from volt or telescope   |
  |   -------------------------------------------    |                     |  --------------------------------------------------------   |

The plugin actually fails building at the time of installation -
build failed

So, what have you decided of this issue and PR?

@Debajyati
Copy link
Author

Debajyati commented Feb 16, 2025

Until you find the best solution, I am switching to my fork of base46 because it is working Errorless at least for now.

With my fork, the build isn't failing -
Debajyati/base46

@siduck
Copy link
Member

siduck commented Feb 16, 2025

this is very weird, none of the OS users are facing this issue whilst using latest version. And do know that the tbl2str function is ages old! it existed before 1 year ago

Edit: closed the PR cuz the issue is resolved, was user's config issue, not the plugins

@siduck siduck closed this Feb 27, 2025
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