search

lsd-rs / lsd

The next gen ls command
Apache License 2.0
13.05k stars 425 forks source link

Adding some more icons for filenames and extensions #929

Closed Babkock closed 9 months ago

Babkock commented 10 months ago

Hello, I hope you all are doing well. I committed to this repository a little over a year ago, when I made a pull request like this one, suggesting ideas for icons. Many of these icon -> extension associations were taken from eza, because that project has many icons that this project does not have yet. I have a pull request pending on that repo, too. Please look over my changed file, but to save you some time here's the summary of what I've done:

You will notice I changed many of the existing icons in place for standard folder/directory name matches, such as in the home directory ("Pictures", "Music", "Downloads" now are actual folder icons) and in the root directory ("bin", "src"). These are in a separate commit, so if you aren't feeling those particular icons, we can roll it back to how those icons were before.

Thank you very much for your time. I welcome any and all feedback, and I will make more commits if necessary.

For reference

muniu-bot[bot] commented 10 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Babkock

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/lsd-rs/lsd/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
codecov-commenter commented 10 months ago

Codecov Report

Merging #929 (7b15f08) into master (64f9dab) will not change coverage. The diff coverage is 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #929   +/-   ##
=======================================
  Coverage   85.76%   85.76%           
=======================================
  Files          51       51           
  Lines        5001     5001           
=======================================
  Hits         4289     4289           
  Misses        712      712
Files Coverage Δ
src/theme/icon.rs 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Babkock commented 10 months ago

Hello, I have fixed the CI tests.

zwpaper commented 10 months ago

hi @Babkock, thanks for contributing, but I would prefer to keep the origin icons if there are no strong reasons to change them, it may break the existed users

Babkock commented 10 months ago

hi @Babkock, thanks for contributing, but I would prefer to keep the origin icons if there are no strong reasons to change them, it may break the existed users

Which original icons, specifically? I think the Rust, subtitle, and Emacs icon changes should stay; are you talking about those, or the ones for Music, Downloads, Pictures, etc.?

zwpaper commented 10 months ago

Hi @Babkock, thanks so much for contributing, all the icons look great but the src, the new icon seems to be less expressive than the origin one.

and we could also change the sbin folder to the icon bin used here https://github.com/lsd-rs/lsd/pull/929/files#diff-5e5b98f7f3576b07a63f44e581ef0ddfa6829569af86bac0231b330cca0a357eL290

Babkock commented 10 months ago

Hi @Babkock, thanks so much for contributing, all the icons look great but the src, the new icon seems to be less expressive than the origin one.

and we could also change the sbin folder to the icon bin used here https://github.com/lsd-rs/lsd/pull/929/files#diff-5e5b98f7f3576b07a63f44e581ef0ddfa6829569af86bac0231b330cca0a357eL290

I think I wanted the src directory to have a folder icon that matches the others, i.e. one that looks like a folder. The little wrench matches the wrench for the other files meson.build and configure. I can change it back to the generic XML icon </> aka \u{f121} if that is what you want. I am not sure what you are referencing with the sbin? sbin and bin both have the same icon, the gear folder \u{e5fc}, at least in my fork they do.

I have pushed some fixes, and added a couple icons that were missing.

zwpaper commented 10 months ago

to have a folder icon that matches the others

I agree with you on this one,

The little wrench matches the wrench for the other files

but not this one.

I also seek for a better one but failed.

zwpaper commented 10 months ago

for the sbin icon, I refer to this line you added https://github.com/lsd-rs/lsd/pull/929/files#diff-5e5b98f7f3576b07a63f44e581ef0ddfa6829569af86bac0231b330cca0a357eR428 ("bin", "\u{eae8}"), // "" the eae8 with 0101 seem great for bin and sbin, but like the previous comment, it also does not have a folder sign

Babkock commented 10 months ago

for the sbin icon, I refer to this line you added https://github.com/lsd-rs/lsd/pull/929/files#diff-5e5b98f7f3576b07a63f44e581ef0ddfa6829569af86bac0231b330cca0a357eR428 ("bin", "\u{eae8}"), // "" the eae8 with 0101 seem great for bin and sbin, but like the previous comment, it also does not have a folder sign

Ohh, okay. So that bin icon (the eae8 0101) is with the extensions, so that icon is only supposed to go with filenames like bootloader.bin or kernel.bin, stuff like that. But for exact filename matches, bin and sbin both have the gear folder icon e5fc. I do not believe sbin should be with the file extensions.

I apologize for any confusion. Are we on the same page? Did I address your concern? Is there anything you would like me to change?