mistercrunch commented on code in PR #34144:
URL: https://github.com/apache/superset/pull/34144#discussion_r2205938335
##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -45,202 +81,618 @@ export class ThemeController {
private storage: ThemeStorage;
- private storageKey: string;
-
private modeStorageKey: string;
private defaultTheme: AnyThemeConfig;
- private systemMode: ThemeMode.DARK | ThemeMode.LIGHT;
+ private darkTheme: AnyThemeConfig | null;
- private currentMode: ThemeMode;
+ private themeSettings: SerializableThemeSettings;
- private customizations: AnyThemeConfig = {};
+ private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT;
- private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
+ private currentMode: ThemeMode;
- private canUpdateThemeFn: () => boolean;
+ private readonly hasBootstrapThemes: boolean;
- private canUpdateModeFn: () => boolean;
+ private onChangeCallbacks: Set<(theme: Theme) => void> = new Set();
private mediaQuery: MediaQueryList;
- constructor(options: ThemeControllerOptions = { themeObject }) {
- this.storage = options.storage || new LocalStorageAdapter();
- this.storageKey = options.storageKey || 'superset-theme';
- this.modeStorageKey = options.modeStorageKey || `${this.storageKey}-mode`;
- this.defaultTheme = options.defaultTheme || {};
- this.themeObject = options.themeObject;
-
- // Load customizations from storage
- const savedThemeJson = this.storage.getItem(this.storageKey);
- if (savedThemeJson) {
- try {
- this.customizations = JSON.parse(savedThemeJson);
- } catch (e) {
- console.error('Failed to parse saved theme:', e);
- this.storage.removeItem(this.storageKey);
- }
+ constructor(options: ThemeControllerOptions = {}) {
+ const {
+ storage = new LocalStorageAdapter(),
+ modeStorageKey = STORAGE_KEYS.THEME_MODE,
+ themeObject: fallbackThemeObject = supersetThemeObject,
+ defaultTheme = (supersetThemeObject.theme as AnyThemeConfig) ?? {},
+ onChange = null,
+ } = options;
+
+ this.storage = storage;
+ this.modeStorageKey = modeStorageKey;
+
+ // Initialize bootstrap data and themes
+ const {
+ bootstrapDefaultTheme,
+ bootstrapDarkTheme,
+ bootstrapThemeSettings,
+ hasBootstrapThemes,
+ }: BootstrapThemeData = this.loadBootstrapData();
+
+ this.hasBootstrapThemes = hasBootstrapThemes;
+ this.themeSettings = bootstrapThemeSettings || {};
+
+ // Set themes based on bootstrap data availability
+ if (this.hasBootstrapThemes) {
+ this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null;
+ this.defaultTheme =
+ bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme;
+ } else {
+ this.darkTheme = null;
+ this.defaultTheme = defaultTheme;
}
- // Determine initial mode
- this.systemMode = ThemeController.getSystemMode();
- const savedMode = this.storage.getItem(this.modeStorageKey) as ThemeMode;
- this.currentMode = savedMode || ThemeMode.SYSTEM;
+ // Initialize system theme detection
+ this.systemMode = ThemeController.getSystemPreferredMode();
- // Apply the initial theme and mode
- this.applyTheme();
+ // Only initialize media query listener if OS preference is allowed
+ if (this.shouldInitializeMediaQueryListener())
+ this.initializeMediaQueryListener();
- if (options.onChange) {
- this.onChangeCallbacks.add(options.onChange);
- }
- this.canUpdateThemeFn = options.canUpdateTheme || (() => true);
- this.canUpdateModeFn = options.canUpdateMode || (() => true);
+ // Initialize theme and mode
+ this.currentMode = this.determineInitialMode();
+ const { theme, themeObject } =
+ this.createInitialThemeObject(fallbackThemeObject);
- // Listen for system theme changes to enable dynamic `SYSTEM` mode
- this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
- this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+ this.themeObject = themeObject;
+
+ // Setup change callback
+ if (onChange) this.onChangeCallbacks.add(onChange);
+
+ // Apply initial theme and persist mode
+ this.applyTheme(theme);
+ this.persistMode();
}
+ // Public Methods
+
/**
- * Cleans up listeners. Should be called when the controller is no longer
needed.
+ * Cleans up listeners and references. Should be called when the controller
is no longer needed.
*/
public destroy(): void {
this.mediaQuery.removeEventListener('change',
this.handleSystemThemeChange);
+ this.onChangeCallbacks.clear();
}
- public canUpdateTheme(): boolean {
- return this.canUpdateThemeFn();
+ /**
+ * Check if the user can update the theme.
+ */
+ public canSetTheme(): boolean {
+ return !this.themeSettings?.enforced;
}
- public canUpdateMode(): boolean {
- return this.canUpdateModeFn();
+ /**
+ * Check if the user can update the theme mode.
+ */
+ public canSetMode(): boolean {
+ return this.isModeUpdatable();
}
+ /**
+ * Returns the current theme object.
+ */
public getTheme(): Theme {
return this.themeObject;
}
+ /**
+ * Returns the current theme mode.
+ */
public getCurrentMode(): ThemeMode {
return this.currentMode;
}
/**
- * Sets new theme customizations (e.g., from a JSON editor).
- * This method updates the theme's appearance but preserves the current mode.
+ * Sets new theme.
+ * @param theme - The new theme to apply
+ * @throws {Error} If the user does not have permission to update the theme
*/
- public setTheme(newCustomizations: AnyThemeConfig): void {
- if (!this.canUpdateTheme()) {
- throw new Error('User does not have permission to update the theme');
- }
- this.customizations = newCustomizations;
+ public setTheme(theme: AnyThemeConfig): void {
+ this.validateThemeUpdatePermission();
- if (!newCustomizations.algorithm) {
- this.currentMode = ThemeMode.LIGHT;
- }
+ const { mode, normalizedTheme } = this.normalizeTheme(theme);
+ this.currentMode = mode;
+
+ this.updateTheme(normalizedTheme);
+ }
- if (newCustomizations?.algorithm) {
- this.currentMode = newCustomizations.algorithm as ThemeMode;
+ /**
+ * Sets the theme mode (light, dark, or system).
+ * @param mode - The new theme mode to apply
+ * @throws {Error} If the user does not have permission to update the theme
mode
+ */
+ public setThemeMode(mode: ThemeMode): void {
+ this.validateModeUpdatePermission(mode);
+
+ if (this.currentMode === mode) return;
+
+ const theme: AnyThemeConfig | null = this.getThemeForMode(mode);
+ if (!theme) {
+ console.warn(`Theme for mode ${mode} not found, falling back to
default`);
+ this.fallbackToDefaultMode();
+ return;
}
- this.applyTheme();
- this.persist();
- this.notifyListeners();
+ this.currentMode = mode;
+ this.updateTheme(theme);
}
/**
- * Changes the theme mode (light, dark, or system).
- * This is for the mode switch.
+ * Resets the theme to the default theme.
*/
- public changeThemeMode(newMode: ThemeMode): void {
- if (!this.canUpdateMode()) {
- throw new Error('User does not have permission to update the theme
mode');
+ public resetTheme(): void {
+ this.currentMode = ThemeMode.DEFAULT;
+ const defaultTheme: AnyThemeConfig =
+ this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+ this.updateTheme(defaultTheme);
+ }
+
+ /**
+ * Handles system theme changes with error recovery.
+ */
+ private handleSystemThemeChange = (): void => {
+ try {
+ const newSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
+ ThemeController.getSystemPreferredMode();
+
+ // Update systemMode regardless of current mode
+ const oldSystemMode: ThemeMode.DEFAULT | ThemeMode.DARK =
this.systemMode;
+ this.systemMode = newSystemMode;
+
+ // Only update theme if currently in SYSTEM mode and the preference
changed
+ if (
+ this.currentMode === ThemeMode.SYSTEM &&
+ oldSystemMode !== newSystemMode
+ ) {
+ const newTheme: AnyThemeConfig | null = this.getThemeForMode(
+ ThemeMode.SYSTEM,
+ );
+
+ if (newTheme) this.updateTheme(newTheme);
+ }
+ } catch (error) {
+ console.error('Failed to handle system theme change:', error);
}
- if (this.currentMode === newMode) return;
+ };
- this.currentMode = newMode;
+ /**
+ * Updates the theme.
+ * @param theme - The new theme to apply
+ */
+ private updateTheme(theme?: AnyThemeConfig): void {
+ try {
+ // If no config provided, use current mode to get theme
+ const config: AnyThemeConfig =
+ theme || this.getThemeForMode(this.currentMode) || this.defaultTheme;
- this.applyTheme();
- this.persist();
- this.notifyListeners();
+ // Normalize the theme
+ const { normalizedTheme } = this.normalizeTheme(config);
+
+ this.applyTheme(normalizedTheme);
+ this.persistMode();
+ this.notifyListeners();
+ } catch (error) {
+ console.error('Failed to update theme:', error);
+ this.fallbackToDefaultMode();
+ }
}
- public resetTheme(): void {
- this.customizations = this.defaultTheme;
- this.applyTheme();
- this.persist();
+ /**
+ * Fallback to default mode with error recovery.
+ */
+ private fallbackToDefaultMode(): void {
+ this.currentMode = ThemeMode.DEFAULT;
+
+ // Get the default theme which will have the correct algorithm
+ const defaultTheme: AnyThemeConfig =
+ this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme;
+
+ this.applyTheme(defaultTheme);
+ this.persistMode();
this.notifyListeners();
}
+ /**
+ * Registers a callback to be called when the theme changes.
+ * @param callback - The callback to be called on theme change
+ * @returns A function to unsubscribe from the theme change events
+ */
public onChange(callback: (theme: Theme) => void): () => void {
this.onChangeCallbacks.add(callback);
- return () => {
- this.onChangeCallbacks.delete(callback);
+ return () => this.onChangeCallbacks.delete(callback);
+ }
+
+ // Private Helper Methods
+
+ /**
+ * Determines whether the MediaQueryList listener for system theme changes
should be initialized.
+ * This checks if OS preference detection is enabled in the theme settings.
+ * @returns {boolean} True if the media query listener should be
initialized, false otherwise
+ */
+ private shouldInitializeMediaQueryListener(): boolean {
+ const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
+ this.themeSettings || {};
+
+ return allowOSPreference === true;
+ }
+
+ /**
+ * Initializes media query listeners if OS preference is allowed
+ */
+ private initializeMediaQueryListener(): void {
+ this.mediaQuery = window.matchMedia(MEDIA_QUERY_DARK_SCHEME);
+ this.mediaQuery.addEventListener('change', this.handleSystemThemeChange);
+ }
+
+ /**
+ * Loads and validates bootstrap theme data.
+ */
+ private loadBootstrapData(): BootstrapThemeData {
+ const {
+ common: { theme = {} as BootstrapThemeDataConfig },
+ } = getBootstrapData();
+
+ const {
+ default: defaultTheme,
+ dark: darkTheme,
+ settings: themeSettings,
+ } = theme;
+
+ const hasValidDefault: boolean = this.hasKeys(defaultTheme);
+ const hasValidDark: boolean = this.hasKeys(darkTheme);
+ const hasValidSettings: boolean = this.hasKeys(themeSettings);
+
+ return {
+ bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null,
+ bootstrapDarkTheme: hasValidDark ? darkTheme : null,
+ bootstrapThemeSettings: hasValidSettings ? themeSettings : null,
+ hasBootstrapThemes: hasValidDefault || hasValidDark,
};
}
- private handleSystemThemeChange = (): void => {
- const newSystemMode = ThemeController.getSystemMode();
- if (this.systemMode === newSystemMode) return;
+ /**
+ * Checks if an object has keys (not empty).
+ */
+ private hasKeys(obj: Record<string, any> | undefined | null): boolean {
+ return Boolean(
+ obj && typeof obj === 'object' && Object.keys(obj).length > 0,
+ );
+ }
- this.systemMode = newSystemMode;
- // If the current mode is SYSTEM, we need to re-apply the theme
- if (this.currentMode === ThemeMode.SYSTEM) {
- this.applyTheme();
- this.notifyListeners();
+ /**
+ * Determines if mode updates are allowed.
+ */
+ private isModeUpdatable(): boolean {
+ const {
+ enforced = DEFAULT_THEME_SETTINGS.enforced,
+ allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching,
+ } = this.themeSettings || {};
+
+ return !enforced && allowSwitching;
+ }
+
+ /**
+ * Normalizes the theme configuration to ensure it has a valid algorithm.
+ * @param theme - The theme configuration to normalize
+ * @returns An object with normalized mode and algorithm.
+ */
+ private normalizeTheme(theme: AnyThemeConfig): {
Review Comment:
Isn't this duplicated logic from what's in `utils.ts`?
https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/theme/utils.ts#L111-L116
Seems ThemeController should probably only deal with `AnyThemeConfig` and
assume that the Theme class will always handle things properly or error out if
there's a bad theme object (?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
