sayanarijit / xplr

A hackable, minimal, fast TUI file explorer
https://xplr.dev
MIT License
4.23k stars 80 forks source link

xplr.util.lscolor returns nil for normal files #705

Closed abhinavnatarajan closed 6 months ago

abhinavnatarajan commented 7 months ago

xplr.util.lscolor returns nil for a normal file (non-executable, not a symlink, and so on), which causes issues for xplr.util.style_mix.

Minimal example configuration:

version = '0.21.7'
local xplr = xplr
xplr.config.general.focus_ui.style = { add_modifiers = { "Bold", "Reversed" } }

The config works correctly for directories:

xplr_issue_1

It also works fine for any file with a non-default styling by LS_COLORS, for e.g. executable files:

xplr_issue_2

It does not work for files with empty styling:

xplr_issue_3

The issue seems to be that for files with empty styling, xplr.util.lscolor returns nil. A possible fix is to modify the rendering function:

xplr.fn.builtin.fmt_general_table_row_cols_1 = function(m)
    -- unmodified from init.lua
    -- ...
    -- change the below line of code
    local style = xplr.util.lscolor(m.absolute_path) or {} -- modification here
    -- continue unmodified
    -- ...
end

Then styling works correctly:

xplr_issue_4

I think the actual fix lies elsewhere though. I'm not yet familiar enough with Rust to open a PR, but there seems to be a bug in util::lscolor in the line below:

https://github.com/sayanarijit/xplr/blob/c1bb251fef46695a1eca59d974c2ad7dd04a1ac1/src/lua/util.rs#L667

LsColors::style_for_path returns an Option so there needs to be a check for None. Or alternatively Style::from needs to perform a check for None.

sayanarijit commented 6 months ago

Thanks for reporting this with detailed explanation. The function lscolor() documentation actually says it can return nil, so it's the right behavior, but I agree it's a bit not very intuitive, and so, even I am not doing any null checking in Lua code. While implementing this, I probably wanted to save some serialization time by returning nil, rather than an object to be serialized. But now I feel it's not worth it.

I'll change the function return type from Style|nil to Style.

abhinavnatarajan commented 6 months ago

The function lscolor() documentation actually says it can return nil, so it's the right behavior, but I agree it's a bit not very intuitive, and so, even I am not doing any null checking in Lua code.

Ah, I must have missed that when reading the documentation. Thanks anyway for the quick fix, this way the default implementation of xplr.fn.builtin.fmt_general_table_row_cols_1 works correctly!