Skip to content

Support icon color#1674

Merged
gokcehan merged 5 commits into
gokcehan:masterfrom
wodesuck:master
Apr 20, 2024
Merged

Support icon color#1674
gokcehan merged 5 commits into
gokcehan:masterfrom
wodesuck:master

Conversation

@wodesuck

@wodesuck wodesuck commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

Implement #928 #1520. Now you can add the third column in ~/.config/lf/icons to set icon colors, fallback to filename color in ~/.config/lf/colors if icon color is not specified.

Example:

*.styl              00;38;2;141;193;73
*.sass              00;38;2;245;83;133
*.scss              00;38;2;245;83;133
*.htm               00;38;2;227;76;38
*.html              00;38;2;227;76;38
image

Also implement #992. You can remove icons by writting only one column in ~/.config/lf/icons, for example:

ln             # LINK
or             # ORPHAN
ow             # OTHER_WRITABLE
di             # DIR
ex              # EXEC, disable excutable file icon, select icon by filename

@joelim-work joelim-work left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch, overall looks good. The only downside to this approach is that you can't specify icon colors using environment variables, but it's probably not worth doing anyway.

In addition, I suggest you should:

  1. Change the unit tests for readPairs to test readArrays instead.
  2. Update the documentation for icons.
  3. Add some examples (commented out) to the etc/icons.example config file.

Comment thread misc.go Outdated
}

pairs = append(pairs, pair)
pairs = append(pairs, arr)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might want to rename this variable:

Suggested change
pairs = append(pairs, arr)
arrs = append(arrs, arr)

@wodesuck

wodesuck commented Apr 7, 2024

Copy link
Copy Markdown
Contributor Author

@joelim-work Updated, check again pls. 🙏

@joelim-work joelim-work left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks once again for the changes.

Comment thread etc/icons.example Outdated
# other formats
*.pdf 

# icons with color

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OPTIONAL: This is just an idea, maybe it might be worth moving this into a separate file (e.g. icons_colored.example). But either way is OK for me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good idea too. Already moved.

@gokcehan

gokcehan commented Apr 7, 2024

Copy link
Copy Markdown
Owner

Thank you very much @wodesuck for the patch and @joelim-work for the review. There is some conflict in this patch after #1644 is merged, and I wasn't sure how to resolve them. Can you please update the patch one more time? Sorry for the inconvenience as we have been holding many feature patches for a while due to the new release.

@wodesuck

wodesuck commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

@gokcehan Conflict resolved.

@gokcehan gokcehan merged commit 6cabb0e into gokcehan:master Apr 20, 2024
@joelim-work

Copy link
Copy Markdown
Collaborator

I have closed #928 and #1520 since they are now addressed by this PR. I will leave #992 open though, although it's possible to delete the entry for ex, I'm not sure whether there's still a way to keep it but prioritize it lower than file extensions.

@joelim-work joelim-work added this to the r33 milestone Jun 24, 2024
@joelim-work joelim-work added the new Pull requests that add new behavior label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new Pull requests that add new behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants