feat: per-window themes ("This window" theme scope)#279
feat: per-window themes ("This window" theme scope)#279TranscriptionFactory wants to merge 1 commit into
Conversation
Let a user set a theme for one window without affecting others, persisted across restart. The active theme stays global by default; a per-window override is resolved during that window's render pass. Mechanism: an ambient "current render window" thread-local in warpui_core, set by the 3 render chokepoints (build_scene/render_view/render_views) via an RAII guard. Appearance::theme()/ui_builder() consult it to resolve a per-window override from new HashMaps, with a zero-cost is_empty() fast path so the feature costs nothing when unused. (The plan called for setting the ambient from Appearance directly in warpui_core, but warpui_core cannot depend on warp_core/Appearance, so the ambient lives in warpui_core and Appearance reads it.) - Appearance: theme_overrides/ui_builder_overrides maps; set/clear_window_theme invalidate only the target window. - AppearanceManager: set/clear_window_theme; Workspace::on_window_closed clears the override so maps don't leak. - Theme chooser: "All windows"/"This window" scope toggle (default All); "This window" routes to the per-window path and records the override on the Workspace for persistence. - Persistence: theme_override TEXT column (migration + schema + model), saved via serde_json so ThemeKind::Custom round-trips, re-applied on window restore. Verification: warp_core, warpui_core and persistence compile cleanly and a new Appearance unit test covers override resolution + the fast path. The warp app crate could not be built/tested in this environment (xcrun "metal" toolchain unavailable; wasm fallback blocked by clang) -- run ./script/presubmit to verify the app crate, clippy/fmt, and the new test. (cherry picked from commit 80f5c59)
zerx-lab
left a comment
zerx-lab
left a comment
There was a problem hiding this comment.
（详见 inline comments 和下方总结 comment）
— 由 Claude Routine 自动生成；如需复评请 @ 维护者人工触发。
Generated by Claude Code
| let window_id = self.window_id; | ||
| AppearanceManager::handle(ctx).update(ctx, |appearance_manager, ctx| match scope { | ||
| ThemeScope::AllWindows => appearance_manager.set_transient_theme(kind, ctx), | ||
| ThemeScope::ThisWindow => appearance_manager.set_window_theme(window_id, kind, ctx), |
There was a problem hiding this comment.
Bug（阻塞）：悬停预览在 "This window" 作用域下不可撤销
AllWindows 作用域下，set_transient_theme 是一个临时预览状态，用户按 Escape 或关闭主题选择器时，Workspace 会通过既有逻辑还原 previous_theme。
但在 ThisWindow 作用域下，set_window_theme 直接写入 theme_overrides HashMap（永久覆盖），没有对应的"撤销悬停预览"路径。结果是：
- 用户切换到 "This window" 作用域
- 鼠标悬停在几个主题上（每次 hover 都调用
select_theme_for_preview→set_window_theme） - 用户按 Escape 取消——
previous_theme的还原逻辑只处理全局 transient 主题，当前窗口的 per-window override 不会被撤销 - 最后一个悬停过的主题被永久应用到此窗口，用户没有选择任何主题
建议修复：在 set_remote_markdown_rendered 之前（即进入 hover 循环前）记录当前窗口的 override（或 None），在 ThemeChooserEvent::Close 处理时若用户取消，调用 clear_window_theme（或恢复到 cancel 前的 override）。具体可参考全局 transient 主题的 previous_theme 模式，在 per-window 维度做相同的处理。
Generated by Claude Code
| @@ -0,0 +1 @@ | |||
| ALTER TABLE windows DROP COLUMN theme_override; | |||
There was a problem hiding this comment.
Question: SQLite DROP COLUMN 兼容性
ALTER TABLE ... DROP COLUMN 是 SQLite 3.35.0（2021-03）才支持的特性。如果应用分发版本中附带了更老的 SQLite（macOS 系统库），执行 down.sql migration 回滚时会失败。
如果回滚路径在实际部署中不被使用（或总是用打包的新版 SQLite），可以不管；否则建议改为创建新表 + 复制数据 + 重命名的传统写法，或在注释里说明最低支持的 SQLite 版本。
Generated by Claude Code
Review 总结整体判断：Request Changes。架构设计合理——thread-local ambient render window + RAII guard + per-window 阻塞问题悬停预览在 "This window" 作用域下不可撤销（见 theme_chooser.rs inline comment）：
结果：用户进入 "This window" 作用域、鼠标悬停几个主题后按 Escape，最后一个悬停过的主题被静默地永久应用到该窗口——这不是用户选择的主题。 建议
看起来不错的地方
— 由 Claude Routine 自动生成；如需复评请 @ 维护者人工触发。 Generated by Claude Code |
2 participants
What
Adds a per-window theme override: a window can use a different theme from the global one, and the choice persists across restart. The active theme stays global by default; selecting "This window" in the theme chooser scopes the theme to the current window only.
Why
Users running multiple windows (e.g. prod vs. dev SSH sessions) often want a visual distinction per window. Today the theme is global only.
How
warpui_core, set by the three render chokepoints (build_scene/render_view/render_views) via an RAII guard.Appearance::theme()/ui_builder()consult it to resolve a per-window override from newHashMaps, with a zero-costis_empty()fast path so there is no cost when the feature is unused.theme_overrides/ui_builder_overridesmaps;set_window_theme/clear_window_themeinvalidate only the target window.Workspace.Workspace::on_window_closedclears the override so the maps don't leak.theme_overrideTEXT column (migration + schema + model), serialized viaserde_jsonsoThemeKind::Customround-trips, re-applied on window restore.Testing
cargo check -p warppasses. A newAppearanceunit test covers override resolution and the empty-map fast path. Please run./script/presubmitto verify the app crate, clippy/fmt, and the new test in CI (the Metal toolchain was unavailable in my local sandbox for a full app build).