fix: add back normal group fallback

[PriceHiller]

Fixes #23.

Adding back the fallback for the normal group resolves the normal bg group lacking a value and allows modes to work as expected. See #23 for further details.

[PriceHiller]

Arguably this issue could actually crop up in any of those groups where the bg is set to NONE, so we may want to add back all the fallbacks and revert 4e69c2aa27a9a6166870a4383ea13eb4d864a1d7

[PriceHiller]

Coming back to this, might make sense to close this out and just create a new pull request only reverting 4e69c2aa27a9a6166870a4383ea13eb4d864a1d7

[fitrh]

What about this approach ?

diff --git a/lua/modes/utils.lua b/lua/modes/utils.lua
index 5bdaaa9..4ff6751 100644
--- a/lua/modes/utils.lua
+++ b/lua/modes/utils.lua
@@ -63,12 +63,12 @@ end
 M.get_fg = function(name, fallback)
    local id = vim.api.nvim_get_hl_id_by_name(name)
    if not id then
-       return fallback
+       return fallback or name
    end

    local foreground = vim.fn.synIDattr(id, 'fg')
    if not foreground or foreground == '' then
-       return fallback
+       return fallback or name
    end

    return foreground
@@ -77,12 +77,12 @@ end
 M.get_bg = function(name, fallback)
    local id = vim.api.nvim_get_hl_id_by_name(name)
    if not id then
-       return fallback
+       return fallback or name
    end

    local background = vim.fn.synIDattr(id, 'bg')
    if not background or background == '' then
-       return fallback
+       return fallback or name
    end

    return background

[PriceHiller]

That'd 100% work as well, not sure if there are any gotchas with that route that might want to be avoided, but it should be fine.

[fitrh]

Yeah, I think reverting https://github.com/mvllow/modes.nvim/commit/4e69c2aa27a9a6166870a4383ea13eb4d864a1d7 is simpler

[PriceHiller]

I'm in a debate if I like your changes more currently, the fallbacks are good, but modifying the function in the manner you did makes it less prone to blowing off feet due to misunderstanding. I doubt the name will ever be passed in as nil, so it might actually be solid. It's entirely possible to later have fg or bg be set to NONE.

For instance, currently this is the fg defaults with the fallback removed in 4e69c2a:

    default_colors = {
        mode_msg = utils.get_fg('ModeMsg'),
    }

and if someone overrides the ModeMsg to none then it blows their foot off without the fallbacks in place, which, as we've seen, can quite easily occur and can potentially lie dormant until someone like me uses a NONE bg or fg value. I think it's just somewhat prone to someone not understanding why the fallbacks are in place and leads us right back to here if they remove them. Your changes somewhat enforce the behavior.

Though the fallbacks are internal, so using them should be fine anyways so reverting 4e69c2a is probably the way to go, but if that is done there should probably be a comment explaining why the fallbacks are in place for folks looking at it later down the line :smile:.

[mvllow]

Did a revert for quickness 😌 Huge thanks for all the information here and also in #23.

I'm going to have to learn how to write tests in lua to prevent silly errors getting through 😔

[PriceHiller]

Yeah, I appreciate the merge and rapid response. luassert might be worth a look at for testing the code, there should be other solutions as well if that doesn't fit your workflow.