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

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

Closed
abhinavnatarajan opened this issue Apr 30, 2024 · 2 comments · Fixed by #706
Closed

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

abhinavnatarajan opened this issue Apr 30, 2024 · 2 comments · Fixed by #706

Comments

@abhinavnatarajan
Copy link
Contributor

abhinavnatarajan commented Apr 30, 2024

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:

let style = LS_COLORS.style_for_path(path).map(Style::from);

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
Copy link
Owner

sayanarijit commented May 1, 2024

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.

sayanarijit added a commit that referenced this issue May 1, 2024
Closes: #705

Also update xplr version.
sayanarijit added a commit that referenced this issue May 1, 2024
Closes: #705

Also update xplr version.
sayanarijit added a commit that referenced this issue May 1, 2024
Closes: #705

Also update xplr version.
sayanarijit added a commit that referenced this issue May 1, 2024
Closes: #705

Also update xplr version.
@abhinavnatarajan
Copy link
Contributor Author

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!

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 a pull request may close this issue.

2 participants