freddiehaddad / feline.nvim

A minimal, stylish and customizable statusline, statuscolumn, and winbar for Neovim
GNU General Public License v3.0
289 stars 8 forks source link

bug: Icon is overwritten when the highlight of the icon is changed #49

Closed tummetott closed 10 months ago

tummetott commented 10 months ago

Did you check docs and existing issues?

Neovim version (nvim -v)

NVIM v0.9.2

Operating system/version

MacOs 13.4.1

Describe the bug

Most default providers return an icon. It is possible to overwrite the icon by adding

icon = {
    str = 'new_icon', 
    hl = {
        fg = '#ff0000',
        bg = '#00ff00',
    }
}

to the component. However, it is not possible to ONLY change the highlight and keep the default icon of the provider

Steps To Reproduce

  1. nvim -u repro.lua --> icon is not shown

  2. Delete the following part

    icon = {
    hl = {
        fg = '#ff0000',
        bg = '#00ff00',
    }
    }

    --> icon is shown

Expected Behavior

I would like the ability to customize the icon highlighting of providers without losing the associated icon string.

Repro

local comp = {
    provider = {
        name = 'file_type',
        opts = {
            filetype_icon = true
        }
    },
    -- As soon as you add this, the icon is not shown anymore
    icon = {
        hl = {
            fg = '#ff0000',
            bg = '#00ff00',
        }
    }
}

require('lazy').setup({
    {
        'freddiehaddad/feline.nvim',
        config = function()
            require('feline').setup {
                components = {
                    active = { { comp } }
                }
            }
        end,
    }
})
freddiehaddad commented 10 months ago

You're wanting the default icon but with your custom highlight, right? You're correct that once you introduce the icon component setting, it's an all or none situation. This should be relatively easy to implement.

If you're interested in creating a PR, I believe the only area that needs some reworking is here:

https://github.com/freddiehaddad/feline.nvim/blob/8bfa1a88d19d86756ef468ad2e499e0337084317/lua/feline/providers/file.lua#L82C1-L96C8

Happy to provide some support as well.

Similar changes for the other providers that also render an icon are going to be needed.

I'm definitely not trying to push this off on you. Happy to work on it, just putting out the option to take it on if it seemed interesting.

freddiehaddad commented 10 months ago

@tummetott I was looking at this closer. The issue isn't a bug, it has to do w/ caching for performance. I'm still looking into how I can do this w/o impacting cache performance. But my suggestion for now is something like this:

icon = function()
    local icon = {}
    icon.str = require('nvim-web-devicons').get_icon(vim.fn.expand('%:t'))
    icon.hl = { fg = '#ff0000' } -- here we are using custom color
    return icon
end,
freddiehaddad commented 10 months ago

@tummetott Done and merged! :) From my local testing, I believe the issue is resolved. Please let me know and I'll close the ticket.

seblj commented 10 months ago

The PR that you merged breaks the icons for me... This line: https://github.com/freddiehaddad/feline.nvim/blob/9581872c05dffc32b2405242bfb5772b05714301/lua/feline/generator.lua#L466 will prefer the default icon over the icon that I specified.

For example, the git branch provider will return return b.gitsigns_head or '', ' '. And line 466 will then prefer the default icon over the one that I specified...

freddiehaddad commented 10 months ago

I was concerned that might happen. Maybe I can add some logic to make a more intelligent decision.

Otherwise, the best solution here is going to be my initial suggestion which is to define a custom icon behavior.

I will try to fix this later toss or just revert it.

Thanks!

freddiehaddad commented 10 months ago

Okay, I created a new PR and reverted that line.

@tummetott The simplest solution here is a custom icon function as I suggested above.

icon = function()
    local icon = {}
    icon.str = require('nvim-web-devicons').get_icon(vim.fn.expand('%:t'))
    icon.hl = { fg = '#ff0000' } -- here we are using custom color
    return icon
end,
tummetott commented 10 months ago

Okay, I created a new PR and reverted that line.

@tummetott The simplest solution here is a custom icon function as I suggested above.

icon = function()
  local icon = {}
  icon.str = require('nvim-web-devicons').get_icon(vim.fn.expand('%:t'))
  icon.hl = { fg = '#ff0000' } -- here we are using custom color
  return icon
end,

I frequently develop my own providers, each returning a tuple consisting of provider text and an associated icon. It would greatly enhance efficiency and conciseness if it were possible to selectively override the highlights of these icons. This way, I could elegantly assign distinct highlights to both the provider text and the icon without the need for redundant calls to the provider. Without this capability, the current process necessitates invoking the provider twice—once for the provider text and a second time for the icon. This not only introduces some level of inefficiency but also adds verbosity to the code. Hence, the motivation behind my request is to streamline this process and achieve a more elegant and succinct solution.

freddiehaddad commented 10 months ago

@tummetott @seblj I believe the issue is fixed to satisfy both use cases with (#55)

freddiehaddad commented 10 months ago

@tummetott @seblj Essentially, if the component.icon key is set and its a function or a string, then it will be used as is. If it's a table, then if str or hl keys are missing, the defaults will be used. Please let me know if this works correct for each of you.

freddiehaddad commented 10 months ago

Okay, I created a new PR and reverted that line. @tummetott The simplest solution here is a custom icon function as I suggested above.

icon = function()
    local icon = {}
    icon.str = require('nvim-web-devicons').get_icon(vim.fn.expand('%:t'))
    icon.hl = { fg = '#ff0000' } -- here we are using custom color
    return icon
end,

I frequently develop my own providers, each returning a tuple consisting of provider text and an associated icon. It would greatly enhance efficiency and conciseness if it were possible to selectively override the highlights of these icons. This way, I could elegantly assign distinct highlights to both the provider text and the icon without the need for redundant calls to the provider. Without this capability, the current process necessitates invoking the provider twice—once for the provider text and a second time for the icon. This not only introduces some level of inefficiency but also adds verbosity to the code. Hence, the motivation behind my request is to streamline this process and achieve a more elegant and succinct solution.

The tricky part to your request is that the user may configure the component in such a way where sometimes they don't want an icon and other times they do. So if the code says, "oh, you didn't specify an icon, I'll pick one for you," it may not be what they want.

However, I think i came up with a solution that gets around that.

freddiehaddad commented 10 months ago

In hindsight, it probably would have been better to add a config option use_default_icon :)

tummetott commented 10 months ago

amazing thanks. works perfect now

mawkler commented 9 months ago

Hi @freddiehaddad! The commit 1dcf6bf0b7fcaf2a2cddec377266eb2e6a41492e broke my custom git component that looks like this:

{
  provider = 'git_branch',
  icon = {
    str = '  ', -- Custom git icon
    hl = { fg = '#f34f29' },
  },
}

The default icon is now used instead of the custom one, and the custom highlight is no longer applied.

Is this expected behaviour?

freddiehaddad commented 9 months ago

Hi @freddiehaddad! The commit 1dcf6bf broke my custom git component that looks like this:

{
  provider = 'git_branch',
  icon = {
    str = '  ', -- Custom git icon
    hl = { fg = '#f34f29' },
  },
}

The default icon is now used instead of the custom one, and the custom highlight is no longer applied.

Is this expected behaviour?

Hm.. That's strange. I tested your case explicitly w/o issue. Mind sharing your config so I can test it? At first glance, I don't see why it's failing.

EDIT:

The default component.icon table is merged with yours and should keep your settings instead of the default. I might need to look at the remaining providers as there could be a case I missed.

To answer your question, it should have been a non-breaking change. I will address it.