Skip to content

fix: ensure icon overrides apply after refresh #573

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

Conversation

dsaenztagarro
Copy link
Contributor

@dsaenztagarro dsaenztagarro commented Apr 6, 2025

Resolves #572

What I did

Git diff maybe result a little bit confusing, but I only extracted apply_user_icons() from setup(), so it can be called also later during the refresh callback.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Unwound the diff, changes are simply:

  • apply_user_icons just moves the code out of setup unchanged
  • set_icon changes param to user_icons_opts to disambiguate

I wasn't able to replicate the issue, however changes are applied as expected on manual refresh and autocmd.

Many thanks for your contribution!

@@ -21,7 +21,7 @@ repos:
- id: colors
name: colors
description: Ensures Light Color Scheme version has been generated.
entry: make colors
entry: make colors-check
Copy link
Member

Choose a reason for hiding this comment

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

Thank you

@@ -132,6 +132,7 @@ return {
["odin"] = "odin",
["openscad"] = "scad",
["opus"] = "opus",
["org"] = "org",
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated nvim upstream addition. Not worth its own PR.

@alex-courtis alex-courtis merged commit 09d9caa into nvim-tree:master Apr 6, 2025
5 checks passed
alex-courtis added a commit that referenced this pull request Apr 6, 2025
@dsaenztagarro
Copy link
Contributor Author

@alex-courtis I've noticed you have reverted the change after being merged.. I feel confused.. can you please explain to me the reason?

The auto command that refresh the icons doesn't take care of keeping the icons overridden from the plugin setup configuration, and without that change, I won't be able to see the icons that I was expecting :(

I would appreciate a lot if you find the time for a few words, so I can see if I can bring to you the explanations or examples needed to keep up with the change.

Thanks in advance

@alex-courtis
Copy link
Member

That was an emergency revert following a user reporting a catastrophic error: #575 (comment)

I'd be most grateful if you investigated.

@dsaenztagarro
Copy link
Contributor Author

@alex-courtis I see! I totally understand the reasons and I completely agree with your criteria. Sure, I will review with care on the following days.

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.

Bug: user icons set from setup are lost after refresh
2 participants