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

Modify theme services to use preference service #11473

Conversation

colin-grant-work
Copy link
Contributor

What it does

This PR's primary objective is to modify the ThemeService and IconThemeService to interact directly with the PreferenceService as their primary source of truth. Previously, neither service was injectable, and so neither had access to the PreferenceService directly. As a consequence, their interactions with the PreferenceService were mediated by the CommonFrontendContribution and both services relied heavily on local storage for persistence.

How to test

  1. Open your user settings.json and execute one of the commands Preferences: Color Theme or Preferences: Icon Theme.
  2. Move the selector to preview themes.
  3. Note that the settings.json file is not modified.
  4. Select a theme from the quick pick.
  5. Note that your settings.json is modified.
  6. In the settings.json file, set a nonsensical value ("Definitely not any kind of theme!") for the preference.
  7. Note that the theme you selected remains in place.
  8. Reload the page.
  9. Note that the theme you selected (if it was a color theme - we don't store assets of icon themes) still remains in place.

This is different from VSCode's behavior where the theme is reset to default on restart. It would be possible to implement this behavior, if preferable.


Check for #10938

  1. Remove the Material Icon Theme from your plugins, if you have it.
  2. Open the application, and in your user settings.json, set workbench.iconTheme to "material-icon-theme".
  3. No change should be immediately observable.
  4. Install the Material Icon Theme from Open VSX.
  5. The theme should immediately take effect.

  1. Set your theme to Red or something else with a distinctive editor background color.
  2. Reload the application.
  3. There should be no theme flashes.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added the theming issues related to theming label Jul 27, 2022
@msujew msujew self-requested a review July 29, 2022 12:32
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

  • Selected themes (color/icon) are persisted across reloads
    • When changing the theme and reloading the app, the background color is correctly set to the last theme applied.
    • Even if the user enters an incorrect/nonsensical entry in the user settings
  • Setting the icon theme and only loading the corresponding vscode extension afterwards correctly applies the icon theme to the application.

@colin-grant-work colin-grant-work force-pushed the bugfix/theme-source-of-truth branch from 32f14bd to ac74ea2 Compare August 1, 2022 22:03
@colin-grant-work colin-grant-work force-pushed the bugfix/theme-source-of-truth branch from ac74ea2 to 2c29f57 Compare August 1, 2022 22:14
@colin-grant-work colin-grant-work merged commit 220af8d into eclipse-theia:master Aug 2, 2022
@colin-grant-work colin-grant-work deleted the bugfix/theme-source-of-truth branch August 2, 2022 14:24
@colin-grant-work colin-grant-work added this to the 1.29.0 milestone Aug 2, 2022
@307god
Copy link

307god commented Oct 24, 2023

let theiaDidInitialize = false;
const originalInitialize = StandaloneServices.initialize;
StandaloneServices.initialize = overrides => {
    if (!theiaDidInitialize) {
        console.warn('Monaco was initialized before overrides were installed by Theia\'s initialization.'
            + ' Please check the lifecycle of services that use Monaco and ensure that Monaco entities are not instantiated before Theia is initialized.', new Error());
    }
    return originalInitialize(overrides);
};

this contaminate @Injectable() MonacoFrontendApplicationContribution
if i want rebind MonacoFrontendApplicationContribution, i can`t change theiaDidInitialize and StandaloneServices.initialize,it always tell me 'Monaco was initialized before overrides were installed by Theia's initialization.'
what can i do?
@colin-grant-work @msujew

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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants