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

feat: add all-the-icons support #5

Closed
wants to merge 5 commits into from

Conversation

librephoenix
Copy link

This PR adds all-the-icons support via a new variable magit-file-icons-icon-backend. This defaults to 'nerd-icons, but is automatically set to 'all-the-icons as a fallback if nerd-icons isn't installed. It also abstracts the icon getter functions into magit-file-icons-icon-for-file-func and magit-file-icons-icon-for-dir-func which can be set as custom function aliases if magit-file-icons-icon-backend is set to nil (in order to potentially support icon backends other than nerd-icons or all-the-icons).

I made just a few mistakes as I was debugging so there's some unneeded reversions in some of the commits. If you'd like me to redo them to be more clean (or fix any other part of it), I'd be happy to.

Also, this is my first PR ever! I hope its helpful! Thanks for making this package!

@gekoke
Copy link
Owner

gekoke commented Jun 27, 2024

Hi! Thanks a lot!

I made just a few mistakes as I was debugging so there's some unneeded reversions in some of the commits. If you'd like me to redo them to be more clean (or fix any other part of it), I'd be happy to.

No worries, I'll probably squash merge anyway.

Just a heads up, you might have to rebase onto main after #7 is merged, as it does modify some of the patching code that had to change due to recent updates in magit.

Also, this is my first PR ever!

Congrats!

@gekoke gekoke changed the title Add all-the-icons support feat: add all-the-icons support Jun 27, 2024
@gekoke
Copy link
Owner

gekoke commented Jun 27, 2024

On a different note, you may have noticed that I follow the conventional commits commit message convention for this project. In the future, it's best to conform to the commit message conventions of any project you contribute to (including in the PR title, as this is used to infer commit messages in various GitHub merge operations).

@gekoke
Copy link
Owner

gekoke commented Jun 27, 2024

Without even delving into the changes themselves, we might want to consider dropping the dependency on nerd-icons now that we will support more than one backend, and leave it to the users to provide whichever backend they want.

(fset 'magit-file-icons-icon-for-file-func 'all-the-icons-icon-for-file)
(fset 'magit-file-icons-icon-for-dir-func 'all-the-icons-icon-for-dir))))))

(magit-file-icons-refresh-backend)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can get rid of this line if we call the refresh function in the minor-mode anyway

Suggested change
(magit-file-icons-refresh-backend)

(require 'all-the-icons)
(setq magit-file-icons-icon-backend 'all-the-icons))))

(defcustom magit-file-icons-icon-for-file-func nil
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use :type 'function? It's a perfectly valid defcustom type, and means you don't have to explicitly explain how to customize the variable in the docstring.

:type 'symbol
:group 'magit-file-icons)

(if (not (require 'nerd-icons nil t)) (funcall
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this check is necessary if we have nerd-icons in the Package-Requires clause

Does not refresh if `magit-file-icons-icon-backend' is nil."
(if magit-file-icons-icon-backend
(if (eq magit-file-icons-icon-backend 'nerd-icons)
(funcall (lambda ()
Copy link
Owner

Choose a reason for hiding this comment

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

Don't mutate customizable variables - these are only meant to be touched by the user. Instead, define an internal variable and derive its initial state from any values for custom variables set by the user.

Comment on lines +132 to +134
(when magit-file-icons-enable-diff-file-section-icons (magit-file-icons-refresh-backend))
(when magit-file-icons-enable-untracked-icons (magit-file-icons-refresh-backend))
(when magit-file-icons-enable-diffstat-icons (magit-file-icons-refresh-backend)))
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to combine these into an or clause, or just run the refresh unconditionally

Suggested change
(when magit-file-icons-enable-diff-file-section-icons (magit-file-icons-refresh-backend))
(when magit-file-icons-enable-untracked-icons (magit-file-icons-refresh-backend))
(when magit-file-icons-enable-diffstat-icons (magit-file-icons-refresh-backend)))
(magit-file-icons-refresh-backend)

(when magit-file-icons-enable-diff-file-section-icons (magit-file-icons-refresh-backend))
(when magit-file-icons-enable-untracked-icons (magit-file-icons-refresh-backend))
(when magit-file-icons-enable-diffstat-icons (magit-file-icons-refresh-backend)))
(add-hook 'magit-mode-hook 'magit-file-icons-refresh-backend)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can get rid of these hooks - it's probably sufficient to refresh on minor mode activation.

@librephoenix
Copy link
Author

Thanks for all of the targeted feedback! It was very helpful to learn from!

I decided to just make a new branch called abstract-icon-getters based on the updated main branch and I think the new implementation should be much simpler. I'll close this PR and open a new one with the updated chages, and you'll have to let me know what you think!

Thanks!

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 this pull request may close these issues.

2 participants