Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Live change of theme from Preferences dropdown #1296

Merged
merged 8 commits into from
Aug 9, 2022

Conversation

francescospissu
Copy link
Contributor

@francescospissu francescospissu commented Aug 5, 2022

Motivation

The theme must be updated immediately if the user selects it from the Preferences drop-down menu. In this way it's possible to preview the appearance of the various themes available.

By pressing the Cancel button the initial theme must be restored.

Change description

Restore the previous theme when the dialog is closed by pressing the Cancel button.

Other information

This is an ad hoc solution for themes, in the long run this behavior should be generalized because other elements of the Preferences, such as the Editor font size or the Interface scale, may also require a live update to give immediate feedback to the user. In this case maybe a big rework of settings is needed.

Closes #1048.

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: theme Related to GUI theming labels Aug 5, 2022
francescospissu and others added 4 commits August 5, 2022 12:19
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I gave it a try on Windows and it works perfectly for me. Very cool enhancement to the IDE.

Thanks Francesco!

@francescospissu francescospissu marked this pull request as ready for review August 8, 2022 06:54
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It worked for me. I have verified it from the IDE2 started from the sources. 👍

@@ -591,6 +591,9 @@ export class SettingsComponent extends React.Component<
const theme = ThemeService.get().getThemes()[selectedIndex];
if (theme) {
this.setState({ themeId: theme.id });
if (ThemeService.get().getCurrentTheme().id !== theme.id) {
ThemeService.get().setCurrentTheme(theme.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

@kittaakos is the method .setCurrentTheme also called by Theia code after we click apply on the settings dialog?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@davegarthsimpson
Copy link
Contributor

davegarthsimpson commented Aug 8, 2022

considering the proposed changes maybe we should consequently mod this also:

  <select
    className="theia-select"
    value={
      ThemeService.get()
        .getThemes()
        .find(({ id }) => id === this.state.themeId)?.label ||
      nls.localize('arduino/common/unknown', 'Unknown')
    }
    onChange={this.themeDidChange}
  >

could be:

  <select
    className="theia-select"
    value={
      ThemeService.get().getCurrentTheme().label
    }
    onChange={this.themeDidChange}
  >

I don't see a reason to now reference the state prop themeId in the JSX (though maybe I'm missing something).

p.s. I don't know if || nls.localize('arduino/common/unknown', 'Unknown') is really required, here I've assumed not.

@francescospissu
Copy link
Contributor Author

I agree @davegarthsimpson. I applied the change here, I tested it and it works fine.

@francescospissu francescospissu merged commit aebec0f into main Aug 9, 2022
@francescospissu francescospissu deleted the fspissu/live-theme-change branch August 9, 2022 12:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
topic: code Related to content of the project itself topic: theme Related to GUI theming type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Live change of theme after selecting from preferences dropdown
4 participants