tbonelee commented on code in PR #5078: URL: https://github.com/apache/zeppelin/pull/5078#discussion_r2394676425
########## zeppelin-web-angular/src/app/services/theme.service.ts: ########## @@ -0,0 +1,197 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Injectable, OnDestroy } from '@angular/core'; +import { BehaviorSubject, Observable } from 'rxjs'; + +export type ThemeMode = 'light' | 'dark' | 'system'; + +const THEME_STORAGE_KEY = 'zeppelin-theme'; +const MONACO_THEMES = { + light: 'vs', + dark: 'vs-dark' +} as const; + +@Injectable({ + providedIn: 'root' +}) +export class ThemeService implements OnDestroy { + private currentTheme: BehaviorSubject<ThemeMode>; + public theme$: Observable<ThemeMode>; + private currentEffectiveTheme: BehaviorSubject<'light' | 'dark'>; + public effectiveTheme$: Observable<'light' | 'dark'>; + private mediaQuery?: MediaQueryList; + private systemThemeListener?: (e: MediaQueryListEvent) => void; + private systemStartedWith: 'light' | 'dark' | null = null; + + ngOnDestroy() { + this.removeSystemThemeListener(); + this.currentTheme.complete(); + this.currentEffectiveTheme.complete(); + } + + constructor() { + const initialTheme = this.detectInitialTheme(); + this.currentTheme = new BehaviorSubject<ThemeMode>(initialTheme); + this.theme$ = this.currentTheme.asObservable(); + + const initialEffectiveTheme = this.resolveEffectiveTheme(initialTheme); + this.currentEffectiveTheme = new BehaviorSubject<'light' | 'dark'>(initialEffectiveTheme); + this.effectiveTheme$ = this.currentEffectiveTheme.asObservable(); + + this.initSystemThemeDetection(); + + this.applyTheme(initialEffectiveTheme); + } + + detectInitialTheme(): ThemeMode { + try { + const savedTheme = localStorage.getItem(THEME_STORAGE_KEY); + if (savedTheme && this.isValidTheme(savedTheme)) { + return savedTheme; + } + return 'system'; + } catch { + return 'system'; + } + } + + isValidTheme(theme: string): theme is ThemeMode { + return theme === 'light' || theme === 'dark' || theme === 'system'; + } + + resolveEffectiveTheme(theme: ThemeMode): 'light' | 'dark' { + if (theme === 'system') { + return window.matchMedia?.('(prefers-color-scheme: dark)')?.matches ? 'dark' : 'light'; + } + return theme; + } + + getCurrentTheme(): ThemeMode { + return this.currentTheme.value; + } + + getEffectiveTheme(): 'light' | 'dark' { + return this.currentEffectiveTheme.value; + } + + isDarkMode(): boolean { + return this.currentEffectiveTheme.value === 'dark'; + } + + setTheme(theme: ThemeMode, save: boolean = true) { + if (this.currentTheme.value === theme) { + return; + } + + this.currentTheme.next(theme); + const effectiveTheme = this.resolveEffectiveTheme(theme); + this.currentEffectiveTheme.next(effectiveTheme); + this.applyTheme(effectiveTheme); + + if (save) { + localStorage.setItem(THEME_STORAGE_KEY, theme); + } + + // Update system theme listener based on new theme + if (theme === 'system') { + this.initSystemThemeDetection(); + } else { + this.removeSystemThemeListener(); + } + } + + toggleTheme() { + const currentTheme = this.getCurrentTheme(); + + if (currentTheme === 'system') { + const currentEffectiveTheme = this.getEffectiveTheme(); + this.systemStartedWith = currentEffectiveTheme; + this.setTheme(currentEffectiveTheme === 'dark' ? 'light' : 'dark'); + } else if (currentTheme === 'dark') { + if (this.systemStartedWith === 'dark') { + this.setTheme('system'); + this.systemStartedWith = null; + } else { + this.setTheme('light'); + } + } else { + if (this.systemStartedWith === 'light') { + this.setTheme('system'); + this.systemStartedWith = null; + } else if (this.systemStartedWith === 'dark') { + this.setTheme('dark'); + } else { + this.setTheme('system'); + } + } + } Review Comment: I think this makes thes state changes feel unexpected when the user's system is set to dark mode. (Parenthesis indicate the repeating cycle.) If starting in light: ``` light -> system -> (light -> dark -> system) -> (light -> dark -> system) -> ... ``` If starting in dark: ``` dark -> light -> system -> (light -> dark -> system) -> (light -> dark -> system) -> ... ``` ########## zeppelin-web-angular/src/app/services/theme.service.ts: ########## Review Comment: I noticed the theme service manually manages MediaQuery listeners with `initSystemThemeDetection()` and `removeSystemThemeListener()`. This works, but it's quite verbose and the cleanup logic is easy to miss. The core issue is that `effectiveTheme` depends on two inputs (user preference `theme$` and OS preference (MediaQuery) ), but this relationship is currently maintained imperatively with manual `.next()` calls scattered across `setTheme()` and the listener callback. What if we made this relationship explicit using RxJS? Something like: I noticed the theme service manually manages MediaQuery listeners with imperative `.next()` calls in multiple places. Since `effectiveTheme` depends on both user preference (`theme$`) and OS preference (MediaQuery), what if we made this relationship more explicit using RxJS? ```typescript const systemTheme$ = fromEvent<MediaQueryListEvent>(mediaQuery, 'change').pipe( map(e => e.matches ? 'dark' : 'light'), startWith(mediaQuery.matches ? 'dark' : 'light') ); combineLatest([theme$, systemTheme$]).pipe( map(([theme, sys]) => theme === 'system' ? sys : theme) ).subscribe(effectiveTheme => { effectiveThemeSubject.next(effectiveTheme); applyTheme(effectiveTheme); }); ``` This way, the data flow becomes: ``` [User Preference] [OS Preference] theme$ systemTheme$ ↓ ↓ └────→ combineLatest ←───────┘ ↓ effectiveTheme ↓ applyTheme() ↓ [DOM + Monaco Updates] ``` Whenever either source changes, `effectiveTheme` automatically recalculates. This would simplify the manual listener management, make cleanup automatic through RxJS subscriptions, and make the state dependencies more explicit. What do you think? Example implementation : https://github.com/tbonelee/zeppelin/commit/2fbe5aa62bc826417b673c355e95f568b8f7f4a5 ########## zeppelin-web-angular/src/app/share/theme-toggle/theme-toggle.component.ts: ########## @@ -0,0 +1,64 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, OnDestroy, OnInit } from '@angular/core'; +import { Subject } from 'rxjs'; +import { takeUntil } from 'rxjs/operators'; + +import { ThemeMode, ThemeService } from '../../services/theme.service'; + +@Component({ + selector: 'zeppelin-theme-toggle', + templateUrl: './theme-toggle.component.html', + styleUrls: ['./theme-toggle.component.less'], + changeDetection: ChangeDetectionStrategy.OnPush +}) +export class ThemeToggleComponent implements OnInit, OnDestroy { + private destroy$ = new Subject(); + currentTheme: ThemeMode = 'light'; + isDarkMode = false; + + constructor(private themeService: ThemeService, private cdr: ChangeDetectorRef) {} + + ngOnInit() { + this.currentTheme = this.themeService.getCurrentTheme(); + this.isDarkMode = this.currentTheme === 'dark'; + + this.themeService.theme$.pipe(takeUntil(this.destroy$)).subscribe(theme => { + if (this.currentTheme !== theme) { + this.currentTheme = theme; + this.isDarkMode = theme === 'dark'; + this.cdr.markForCheck(); + } + }); + } + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } + + toggleTheme() { + this.themeService.toggleTheme(); + } + + setTheme(theme: ThemeMode) { + this.themeService.setTheme(theme); + } + + getThemeIcon(): string { + if (this.currentTheme === 'system') { + return '🤖'; + } + return this.currentTheme === 'dark' ? '🌙' : '☀️'; + } +} Review Comment: Could we switch from Unicode emoji to SVG icons? Emoji rendering depends on OS/browser font support and can vary or fail, while SVGs provide consistent visuals across environments. -- 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]
