diff --git a/docs/architecture.md b/docs/architecture.md index ba685d4..94dfe8e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -315,15 +315,23 @@ A fifth theming axis, and the only one that is a **class** (`.win7` on ``) rather than a `data-*` attribute — `7.css/dist/7.scoped.css` is precompiled against a literal `.win7`, so the name is not ours to pick. Default `modern` = class absent, matching the delete-when-default rule. -`SettingsService.applySkin` owns the class once the page is live; -`buildPreload` writes it for the first paint. +`buildPreload` is the **only** writer of that class, ever — there is +deliberately no `SettingsService.applySkin`. -Unlike the other four axes, this one is not only CSS: it selects a whole -component tree (see "UI layout"). So a skin change is **two-phase** — -`applySkin` flips the CSS immediately, but the tree and the native frame are -both fixed at window construction (`main.ts` reads the preload-baked -`__APP_SKIN`; `titleBarStyle`/`transparent` are `BrowserWindow` constructor -options). The options row carries the reopen hint as its tooltip. +**Unlike the other four axes, this one is launch-time only.** It selects a +whole component tree (see "UI layout") as well as CSS, and neither half can +change on a live window: the tree is bootstrapped once from the preload-baked +`__APP_SKIN`, and `titleBarStyle`/`transparent` are `BrowserWindow` constructor +options with no setters. Clicking a skin therefore only persists it; the +options row shows the saved value as pending (`skinPending()` compares +`settings.skin()` against `APP_SKIN`) and the running UI is untouched until the +next app start. + +The bun side pins this: `initSessionSkin` freezes the skin at startup, before +any window exists, and both `buildPreload` and `chromeOptions` read that pinned +value rather than the live settings snapshot. Otherwise an editor or manager +opened after a pending change would boot the *new* tree and frame while the +main window still ran the old one. **Stylesheet split.** `ui/src/styles.css` holds only what BOTH skins consume — the `@layer` statement, the Tailwind import, OKLCH tokens, `@theme inline`, @@ -427,9 +435,9 @@ without decorations (custom titlebar)" (verified on GNOME/Wayland), so the frameless Aero chrome is not a mac/win-only feature. The webview then draws the whole frame via `ui/src/app/skins/7like/title-bar/title-bar.ts` (7.css's `.title-bar` > `.title-bar-text` + `.title-bar-controls`, buttons keyed by `aria-label`), -mounted by all three window roots. Because `titleBarStyle` is fixed at -construction, **a skin change only reaches the frame on the next window open**; -the CSS half flips live, and the options row says so. `IS_WIN7_CHROME` in +mounted by all three of that skin's window roots. Because `titleBarStyle` is +fixed at construction, **a skin change only reaches the frame on the next app +start** — and nothing about the skin flips live, deliberately. `IS_WIN7_CHROME` in `platform.ts` reads the preload-baked `__APP_SKIN` (settings resolve too late for layout) and forces `IS_MAC_CHROME` false, since there are no traffic lights to reserve `pl-24` for. diff --git a/docs/gotchas.md b/docs/gotchas.md index a7289f1..74a2b20 100644 --- a/docs/gotchas.md +++ b/docs/gotchas.md @@ -201,10 +201,30 @@ touching the adjacent code. The win7 skin adds a **fourth value to keep in sync**: `buildPreload` also bakes the `.win7` class, and index.html carries a matching `html.win7` arm for all four theme states. Miss it and every win7 window flashes white. -- **`titleBarStyle` is construction-time only.** Switching skin cannot - re-frame a live window; `chromeOptions` is read once per `new - BrowserWindow`. The options row tells the user to reopen. Don't "fix" this - by recreating the main window — that would drop every session and tab. +- **The skin is a LAUNCH-TIME choice. Never apply one to a running window.** + This shipped broken once: `SettingsService.applySkin` toggled the `.win7` + class the moment the setting changed, which was harmless when there was one + component tree and became a broken window as soon as there were two — 7.css + painted onto shadlike markup that has no `.window.glass`, no `.title-bar`, no + `menu[role=tablist]` and no `` ListView. Each skin's CSS is written + against its OWN tree's markup; the class must only ever be written by + `buildPreload`, for the tree that window actually booted. + - **A reload does not rescue it.** The preload is a persistent + `WebKitUserScript` injected at DOCUMENT_START, so its string is frozen at + webview construction — `webview.loadURL()` re-runs it with the OLD + `__APP_SKIN`. (`updatePreloadScriptToWebView` exists in the FFI but is + reachable only from electrobun's internal `` handler.) + - **`titleBarStyle`/`transparent` are construction-time only** — no setters + exist, so a live window can never be re-framed. win7 → modern is the + dangerous direction: the modern tree draws no title bar, so it would land + in a frameless transparent window with no way to move, minimize or close. + - Don't "fix" this by recreating the main window either: `exitOnLastWindowClosed` + is true, the main `rpc` is captured by six push closures, the tray captures + `win`, listeners cannot be removed (no `off()`) — and it would drop every + session and tab. + - Bun pins it: `initSessionSkin` freezes the skin before any window exists, + and `buildPreload`/`chromeOptions` read that, never the live settings. Skip + the pin and a window opened after a pending change boots the other skin. - Dev vs prod URL: `getMainViewUrl()` resolves the ng-serve dev server only when `DEV_SERVER_URL` is set AND the updater channel is `dev` AND the server answers a HEAD request; secondary windows must reuse the resolved diff --git a/src/bun/index.ts b/src/bun/index.ts index e192f41..8d32755 100644 --- a/src/bun/index.ts +++ b/src/bun/index.ts @@ -15,7 +15,13 @@ import { migrateLegacyDataDir } from "./store/paths"; import { SettingsStore } from "./store/settings"; import { AppTray } from "./tray"; import { TransferEngine } from "./transfers/engine"; -import { buildPreload, chromeOptions, showWhenReady, trackMaximizeState } from "./windows"; +import { + buildPreload, + chromeOptions, + initSessionSkin, + showWhenReady, + trackMaximizeState, +} from "./windows"; const PROD_URL = "views://app/index.html"; @@ -76,6 +82,11 @@ const url = await getMainViewUrl(); // fresh via the saveSettings flow below. let currentSettings = await settings.load(); setBackendLocale(currentSettings.locale); // native menus/errors follow the UI language +// Freeze the skin for this process BEFORE any window is constructed. It picks +// a component tree and a native frame, neither of which can change on a live +// window, so the persisted value may only take effect at the next start — see +// initSessionSkin. +initSessionSkin(currentSettings.skin); const win = new BrowserWindow({ title: BRANDING.appName, @@ -87,8 +98,8 @@ const win = new BrowserWindow({ y: 120, }, // Frame decision lives in chromeOptions — see windows.ts. Constructor-only, - // so a skin change reaches the frame on the NEXT open, not live. - ...chromeOptions(currentSettings.skin), + // and read from the pinned session skin, not from currentSettings. + ...chromeOptions(), // lets the UI pick the fused layout + theme before first paint preload: buildPreload("main", currentSettings), rpc, diff --git a/src/bun/windows.ts b/src/bun/windows.ts index 200716b..65e6b3a 100644 --- a/src/bun/windows.ts +++ b/src/bun/windows.ts @@ -1,6 +1,27 @@ import { BrowserWindow } from "electrobun/bun"; -import type { AppSettings } from "../shared/types"; +import { DEFAULT_SETTINGS, type AppSettings } from "../shared/types"; +/** + * The skin THIS PROCESS booted with, frozen at startup by `initSessionSkin`. + * + * Every window must be built from this, never from the live settings snapshot. + * The skin picks a whole component tree (bootstrapped from the preload-baked + * __APP_SKIN) and the native frame (constructor-only), so it cannot change + * without a restart — but the persisted setting CAN change mid-session. Reading + * the live value would boot a newly opened editor or manager into the pending + * skin while the main window still runs the old one: two windows, two trees, + * two frames. + * + * Pinning it in one place makes that impossible rather than making each call + * site remember. Everything else in the preload keeps using the live snapshot, + * because theme/tone/accent/text-size genuinely do apply live. + */ +let sessionSkin: AppSettings["skin"] = DEFAULT_SETTINGS.skin; + +/** Call once, at startup, before any window is constructed. */ +export function initSessionSkin(skin: AppSettings["skin"]): void { + sessionSkin = skin; +} /** * The preload globals every window gets. Single composition point: the UI @@ -18,16 +39,19 @@ export function buildPreload( settings: AppSettings, ctx?: unknown, ): string { + // sessionSkin, NOT settings.skin — a pending skin change must not leak into + // a window opened before the restart. See initSessionSkin. let script = `window.__APP_PLATFORM=${JSON.stringify(process.platform)};` + `window.__APP_MODE=${JSON.stringify(mode)};` + - `window.__APP_SKIN=${JSON.stringify(settings.skin)};`; + `window.__APP_SKIN=${JSON.stringify(sessionSkin)};`; if (ctx !== undefined) script += `window.__APP_CTX=${JSON.stringify(ctx)};`; // A class, not a data-* attribute: 7.scoped.css is precompiled to require a - // literal `.win7` ancestor, so the name is not ours to choose. Mirrors - // SettingsService.applySkin, which owns it once the page is live. - if (settings.skin === "win7") { + // literal `.win7` ancestor, so the name is not ours to choose. This is the + // ONLY place it is ever written — the UI must never toggle it live, because + // each skin's CSS is written against its own tree's markup. + if (sessionSkin === "win7") { script += `document.documentElement.classList.add("win7");`; } @@ -46,12 +70,13 @@ export function buildPreload( * Native chrome options for a window. The ONE place the frame decision is made — * both the main window (src/bun/index.ts) and openSecondaryWindow spread this. * - * Both values are BrowserWindow CONSTRUCTOR options, which is exactly why a - * skin change only reaches the frame on the next window open: there is no API - * to re-decorate a live window. SettingsService.applySkin flips the CSS half - * immediately, and the options row tells the user about the gap. + * Both values are BrowserWindow CONSTRUCTOR options — there is no API to + * re-decorate a live window — which is half of why the skin is a launch-time + * choice. Takes no argument on purpose: it reads the pinned sessionSkin, so no + * call site can accidentally frame a window from a pending setting. */ -export function chromeOptions(skin: AppSettings["skin"]) { +export function chromeOptions() { + const skin = sessionSkin; // win7 draws its own Aero title bar (drag region + our minimize/maximize/ // close), so the window is frameless on EVERY platform. `titleBarStyle: // "hidden"` clears `Titled` from the Cocoa style mask, which is also what @@ -141,7 +166,7 @@ export function openSecondaryWindow(opts: SecondaryWindowOptions): BrowserWindow title: opts.title, url: opts.url, frame: opts.frame, - ...chromeOptions(opts.settings.skin), + ...chromeOptions(), preload: buildPreload(opts.mode, opts.settings, opts.ctx), rpc: opts.rpc, hidden: true, diff --git a/src/shared/i18n/de.ts b/src/shared/i18n/de.ts index ccf6490..ac530b8 100644 --- a/src/shared/i18n/de.ts +++ b/src/shared/i18n/de.ts @@ -37,7 +37,7 @@ export const de = { label: "Optik", modern: "Modern", win7: "Windows 7", - restartHint: "Fenster neu öffnen, um den Rahmen zu ändern", + restartHint: "Wird beim nächsten Start von miniSCP übernommen", }, theme: { label: "Design", system: "System", light: "Hell", dark: "Dunkel" }, tone: { label: "Ton", dark: "Dunkel", darker: "Dunkler" }, diff --git a/src/shared/i18n/en.ts b/src/shared/i18n/en.ts index cb71fa3..0cb4170 100644 --- a/src/shared/i18n/en.ts +++ b/src/shared/i18n/en.ts @@ -41,7 +41,7 @@ export const en = { label: "Skin", modern: "Modern", win7: "Windows 7", - restartHint: "Reopen the window to change its frame", + restartHint: "Applies next time you start miniSCP", }, theme: { label: "Theme", system: "System", light: "Light", dark: "Dark" }, tone: { label: "Tone", dark: "Dark", darker: "Darker" }, diff --git a/src/shared/i18n/es.ts b/src/shared/i18n/es.ts index c52464e..6dae928 100644 --- a/src/shared/i18n/es.ts +++ b/src/shared/i18n/es.ts @@ -37,7 +37,7 @@ export const es = { label: "Aspecto", modern: "Moderno", win7: "Windows 7", - restartHint: "Vuelve a abrir la ventana para cambiar su marco", + restartHint: "Se aplica la próxima vez que abras miniSCP", }, theme: { label: "Tema", system: "Sistema", light: "Claro", dark: "Oscuro" }, tone: { label: "Tono", dark: "Oscuro", darker: "Más oscuro" }, diff --git a/src/shared/i18n/eu.ts b/src/shared/i18n/eu.ts index e045bea..495d4af 100644 --- a/src/shared/i18n/eu.ts +++ b/src/shared/i18n/eu.ts @@ -39,7 +39,7 @@ export const eu = { label: "Itxura", modern: "Modernoa", win7: "Windows 7", - restartHint: "Ireki leihoa berriro bere markoa aldatzeko", + restartHint: "Hurrengoan miniSCP abiaraztean aplikatuko da", }, theme: { label: "Gaia", system: "Sistema", light: "Argia", dark: "Iluna" }, tone: { label: "Tonua", dark: "Iluna", darker: "Ilunagoa" }, diff --git a/src/shared/i18n/fr.ts b/src/shared/i18n/fr.ts index 4d6263a..5a227fc 100644 --- a/src/shared/i18n/fr.ts +++ b/src/shared/i18n/fr.ts @@ -37,7 +37,7 @@ export const fr = { label: "Apparence", modern: "Moderne", win7: "Windows 7", - restartHint: "Rouvrez la fenêtre pour changer son cadre", + restartHint: "Prend effet au prochain démarrage de miniSCP", }, theme: { label: "Thème", system: "Système", light: "Clair", dark: "Sombre" }, tone: { label: "Ton", dark: "Sombre", darker: "Plus sombre" }, diff --git a/src/shared/i18n/ja.ts b/src/shared/i18n/ja.ts index 9c599ad..eb02d58 100644 --- a/src/shared/i18n/ja.ts +++ b/src/shared/i18n/ja.ts @@ -38,7 +38,7 @@ export const ja = { label: "外観", modern: "モダン", win7: "Windows 7", - restartHint: "ウィンドウの枠を変更するには開き直してください", + restartHint: "次回 miniSCP を起動したときに適用されます", }, theme: { label: "テーマ", system: "システム", light: "ライト", dark: "ダーク" }, tone: { label: "トーン", dark: "ダーク", darker: "より暗く" }, diff --git a/src/shared/i18n/zh.ts b/src/shared/i18n/zh.ts index 4d838ad..355f641 100644 --- a/src/shared/i18n/zh.ts +++ b/src/shared/i18n/zh.ts @@ -38,7 +38,7 @@ export const zh = { label: "外观", modern: "现代", win7: "Windows 7", - restartHint: "重新打开窗口以更改窗口边框", + restartHint: "下次启动 miniSCP 时生效", }, theme: { label: "主题", system: "系统", light: "浅色", dark: "深色" }, tone: { label: "色调", dark: "深色", darker: "更深" }, diff --git a/ui/src/app/core/settings.service.ts b/ui/src/app/core/settings.service.ts index 33697f6..fba3708 100644 --- a/ui/src/app/core/settings.service.ts +++ b/ui/src/app/core/settings.service.ts @@ -64,7 +64,9 @@ export class SettingsService { this.autoClearCompleted.set(settings.autoClearCompleted); this.editor.set(settings.editor); this.editorApp.set(settings.editorApp); - this.applySkin(); + // NO applySkin here, deliberately — see setSkin. applyAll also runs on the + // settingsChanged push, so flipping the class here would break every OPEN + // editor and manager window too, not just the one that changed the setting. this.applyTheme(); this.applyDarkTone(); this.applyAccent(); @@ -79,15 +81,26 @@ export class SettingsService { } /** - * Only the CSS half of a skin change lands here. The component TREE and the - * native frame are both fixed at window construction (main.ts reads the - * preload-baked __APP_SKIN; titleBarStyle/transparent are BrowserWindow - * constructor options), so the switch is not complete until the window is - * reopened. The options row says so. + * Saves the preference and NOTHING else — the skin is a launch-time choice. + * + * It deliberately does not touch the `.win7` class, which is the whole bug + * this replaced: each skin's CSS is written against its OWN component tree's + * markup, and the tree is fixed at bootstrap from the preload-baked + * __APP_SKIN. Flipping the class live therefore painted 7.css onto shadlike + * markup that has no .window.glass, no .title-bar, no menu[role=tablist] and + * no
ListView — a broken window, not a reskinned one. + * + * A reload would not have helped either: the preload string is frozen at + * webview construction (a persistent WebKitUserScript), and titleBarStyle / + * transparent have no setters, so the native frame cannot change on a live + * window. win7 -> modern without a reframe is the dangerous direction — the + * modern tree draws no title bar, so it would land in a frameless window + * with no way to move, minimize or close it. + * + * The options row shows the saved value as pending until the next start. */ setSkin(skin: AppSettings['skin']): void { this.skin.set(skin); - this.applySkin(); void this.persist(); } @@ -158,12 +171,11 @@ export class SettingsService { void this.persist(); } - /** A CLASS, not a data-* attribute: 7.scoped.css is precompiled to require a - * literal `.win7` ancestor, so the name is not ours to choose. Mirrors what - * buildPreload writes for the first paint. */ - private applySkin(): void { - document.documentElement.classList.toggle('win7', this.skin() === 'win7'); - } + // There is deliberately no applySkin(). The `.win7` class is written once by + // buildPreload at DOCUMENT_START for the skin this window actually booted, + // and must never change afterwards — see setSkin for why. Every other apply* + // below is safe to run live because it only re-points tokens, and both skins + // consume the same token system. private applyTheme(): void { const theme = this.theme(); diff --git a/ui/src/app/skins/7like/7like.css b/ui/src/app/skins/7like/7like.css index 47abcd8..19cba8a 100644 --- a/ui/src/app/skins/7like/7like.css +++ b/ui/src/app/skins/7like/7like.css @@ -1,6 +1,7 @@ /* The SCOPED 7.css build: postcss-prefix-selector rewrote every selector to `.win7` (root rules) or `.win7 ` (the rest), so the whole framework is - inert until SettingsService.applySkin puts that class on . All 61 + inert until the `.win7` class is on — written once by buildPreload + for the skin this window booted, and never changed after. All 61 icons are data URIs, so nothing needs copying into assets — and NOT the `7.inline.css` build, which flattens away the --w7-* vars the dark palette below depends on. The deep path is required: 7.css ships no `exports` map @@ -9,7 +10,7 @@ @import "7.css/dist/7.scoped.css" layer(skin); /* ========================================================================== - Windows 7 skin — SettingsService.applySkin puts `.win7` on + Windows 7 skin — buildPreload puts `.win7` on at first paint ========================================================================== Three tiers, and the split matters: diff --git a/ui/src/app/skins/7like/tab-bar/tab-bar.ts b/ui/src/app/skins/7like/tab-bar/tab-bar.ts index b7fca3b..db2d579 100644 --- a/ui/src/app/skins/7like/tab-bar/tab-bar.ts +++ b/ui/src/app/skins/7like/tab-bar/tab-bar.ts @@ -1,11 +1,11 @@ -import { Component, ElementRef, effect, inject, signal, viewChild } from '@angular/core'; +import { Component, ElementRef, computed, effect, inject, signal, viewChild } from '@angular/core'; import { NgIcon } from '@ng-icons/core'; import { BRANDING } from '@shared/branding'; import { LOCALE_LABELS, SUPPORTED } from '@shared/i18n'; import type { AccentTone } from '@shared/types'; import { I18nService } from '@core/i18n.service'; import { PanelsService } from '@core/panels.service'; -import { MOD_LABEL } from '@core/platform'; +import { APP_SKIN, MOD_LABEL } from '@core/platform'; import { RpcService } from '@core/rpc.service'; import { SettingsService } from '@core/settings.service'; import { UpdaterService } from '@core/updater.service'; @@ -175,10 +175,13 @@ const CONCURRENCY_CHOICES = [2, 3, 4, 6, 8] as const; - +
{{ i18n.t('options.skin.label') }}
@@ -196,6 +199,11 @@ const CONCURRENCY_CHOICES = [2, 3, 4, 6, 8] as const; }
+ @if (skinPending()) { +

+ {{ i18n.t('options.skin.restartHint') }} +

+ }
{{ i18n.t('options.theme.label') }} @@ -455,6 +463,12 @@ export class TabBar { /** What the window was BUILT with, not the current setting — the frameless * chrome only changes on reopen, so the hint compares against this. */ readonly skins = SKINS; + /** + * The saved skin differs from the one this window actually booted, so the + * choice is waiting on a restart. APP_SKIN is the preload-baked value that + * picked the running component tree; settings.skin() is merely persisted. + */ + readonly skinPending = computed(() => this.settings.skin() !== APP_SKIN); readonly themes = THEMES; readonly darkTones = DARK_TONES; readonly accents = ACCENTS; diff --git a/ui/src/app/skins/shadlike/tab-bar/tab-bar.ts b/ui/src/app/skins/shadlike/tab-bar/tab-bar.ts index 62ca01f..5c890bf 100644 --- a/ui/src/app/skins/shadlike/tab-bar/tab-bar.ts +++ b/ui/src/app/skins/shadlike/tab-bar/tab-bar.ts @@ -1,11 +1,11 @@ -import { Component, ElementRef, effect, inject, signal, viewChild } from '@angular/core'; +import { Component, ElementRef, computed, effect, inject, signal, viewChild } from '@angular/core'; import { NgIcon } from '@ng-icons/core'; import { BRANDING } from '@shared/branding'; import { LOCALE_LABELS, SUPPORTED } from '@shared/i18n'; import type { AccentTone } from '@shared/types'; import { I18nService } from '@core/i18n.service'; import { PanelsService } from '@core/panels.service'; -import { IS_MAC_CHROME, MOD_LABEL } from '@core/platform'; +import { APP_SKIN, IS_MAC_CHROME, MOD_LABEL } from '@core/platform'; import { RpcService } from '@core/rpc.service'; import { SettingsService } from '@core/settings.service'; import { UpdaterService } from '@core/updater.service'; @@ -181,10 +181,13 @@ const CONCURRENCY_CHOICES = [2, 3, 4, 6, 8] as const;
- +
{{ i18n.t('options.skin.label') }}
@@ -202,6 +205,11 @@ const CONCURRENCY_CHOICES = [2, 3, 4, 6, 8] as const; }
+ @if (skinPending()) { +

+ {{ i18n.t('options.skin.restartHint') }} +

+ }
{{ i18n.t('options.theme.label') }} @@ -460,6 +468,12 @@ export class TabBar { readonly appName = BRANDING.appName; readonly buildLabel = BRANDING.buildLabel; readonly skins = SKINS; + /** + * The saved skin differs from the one this window actually booted, so the + * choice is waiting on a restart. APP_SKIN is the preload-baked value that + * picked the running component tree; settings.skin() is merely persisted. + */ + readonly skinPending = computed(() => this.settings.skin() !== APP_SKIN); readonly themes = THEMES; readonly darkTones = DARK_TONES; readonly accents = ACCENTS;