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

Adding some more icons for filenames and extensions #929

Merged
merged 9 commits into from
Nov 2, 2023

Conversation

Babkock
Copy link
Contributor

@Babkock Babkock commented Oct 7, 2023

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:

  • Found a better (larger and clearer) Rust icon, so I changed that (\u{e68b})
  • Set emacs config files (config.el, packages.el etc.) to use the actual Emacs icon instead of GNU head for Emacs files (\u{e632})
  • Perl language extensions (.pl, .pm, .pod, .plx) now use the camel instead of the... circle (\u{e67e})
  • Added OCAML extensions (.ml, .mli, .mll)
  • Added many Go and Gradle filenames
  • Added separate key icons for public keys (extensions) and private keys (filenames)
  • Added a kitty cat for Yarn configuration files
  • Switched all the Bash, Zsh, and Csh filenames to use a shebang icon, instead of the command prompt

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

muniu-bot bot commented Oct 7, 2023

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@muniu-bot muniu-bot bot added the size/L label Oct 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.76%. Comparing base (64f9dab) to head (7b15f08).
Report is 62 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #929   +/-   ##
=======================================
  Coverage   85.76%   85.76%           
=======================================
  Files          51       51           
  Lines        5001     5001           
=======================================
  Hits         4289     4289           
  Misses        712      712           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Babkock
Copy link
Contributor Author

Babkock commented Oct 11, 2023

Hello, I have fixed the CI tests.

@zwpaper
Copy link
Member

zwpaper commented Oct 15, 2023

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
Copy link
Contributor Author

Babkock commented Oct 15, 2023

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.?

Copy link
Member

@zwpaper zwpaper left a comment

Choose a reason for hiding this comment

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

never mind, it seems reasonable to change at a second thought.

but found 2 nits

("shadow", "\u{e615}"), // ""
("share", "\u{f064}"), // ""
(".shellcheckrc", "\u{e615}"), // ""
("shells", "\u{e615}"), // ""
(".sqlite_history", "\u{e7c4}"), // ""
("src", "\u{f121}"), // ""
(".ssh", "\u{f023}"), // ""
("src", "\u{f19fc}"), // "󱧼"
Copy link
Member

Choose a reason for hiding this comment

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

it seems that the origin one is showing source code, and the new one could not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this back if you want.

src/theme/icon.rs Outdated Show resolved Hide resolved
@zwpaper
Copy link
Member

zwpaper commented Oct 31, 2023

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
Copy link
Contributor Author

Babkock commented Nov 1, 2023

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

zwpaper commented Nov 1, 2023

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

zwpaper commented Nov 1, 2023

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
Copy link
Contributor Author

Babkock commented Nov 1, 2023

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?

@zwpaper zwpaper merged commit 89659e4 into lsd-rs:master Nov 2, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants