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

theming: Properly Apply Default Icon Theme #12419

Conversation

FernandoAscencio
Copy link
Contributor

What it does

Closes #11837
The issue was caused by the editor reading the theme before the active theme was changed.
In other words, the editor read the previous theme and placed the classes for those icons. Once the theme changed those class icons had no icons associated with them and as such were not able to load.

This commit solves the issue by changing the order of instructions. Assigning the new theme to the active parameter before moving on to the disposing and activating the theme.

How to test

  1. Run Theia
  2. Open the Icon Theme menu quick pick
  3. Use the arrow keys to highlight/select between themes
  4. Check that all icon themes display properly

Alternatively, you can use the mouse or arrow keys to select and change between themes.

Issue11837Test01

Notes

Currently, disposing of the current theme triggers the editor to set labels and thus read the icon themes.
The plugins add another event trigger. The none theme has this event trigger twice. One in the middle of setting the new theme (the method setCurrent which is default behavior) then another one after the method finishes. Probably due to being tied with the IconThemeService. The Default/Theia theme only triggered this "reload" event once and thus was the one where the issue was noticeable. This double trigger in the none theme does not happen when plugins are not installed.

Review checklist

Reminder for reviewers

This commit closes issue 11837.
The Theme was not applying properly due to the editor reading the theme
before it was changed.

Signed-Off-By: FernandoAscencio <[email protected]>
@vince-fugnitto vince-fugnitto added the theming issues related to theming label Apr 17, 2023
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍
I confirmed that:

  • the file-icons for opened tabs are correctly applied when cycling through available themes
  • the file-icons are properly applied for new tabs
  • the file-icons are properly applied on restart

@JonasHelming
Copy link
Contributor

@FernandoAscencio @vince-fugnitto Can we merge this?

@vince-fugnitto vince-fugnitto merged commit b206d89 into eclipse-theia:master Apr 20, 2023
@vince-fugnitto vince-fugnitto deleted the fa/FileIconProperlyApplied branch April 20, 2023 12:57
@vince-fugnitto vince-fugnitto added this to the 1.37.0 milestone Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theming issues related to theming
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

theming: theia file icons theme not applied correctly
3 participants