diff --git a/bread-theme/src/gtk.rs b/bread-theme/src/gtk.rs index e4ffeda..007f140 100644 --- a/bread-theme/src/gtk.rs +++ b/bread-theme/src/gtk.rs @@ -1,12 +1,14 @@ use gtk4::gdk::prelude::*; use gtk4::gio; +use gtk4::glib; use gtk4::glib::object::ObjectType; use gtk4::prelude::*; use gtk4::CssProvider; -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::collections::{HashMap, HashSet}; use std::path::Path; use std::rc::Rc; +use std::thread::LocalKey; use crate::Palette; @@ -21,12 +23,58 @@ const BIND_PRIORITY: u32 = gtk4::STYLE_PROVIDER_PRIORITY_USER - 10; thread_local! { static SHARED_PROVIDER: RefCell> = const { RefCell::new(None) }; static SHARED_MONITOR: RefCell> = const { RefCell::new(None) }; + static SHARED_RETRY: Cell = const { Cell::new(false) }; static APP_PROVIDER: RefCell> = const { RefCell::new(None) }; static APP_MONITOR: RefCell> = const { RefCell::new(None) }; + static APP_RETRY: Cell = const { Cell::new(false) }; #[allow(clippy::type_complexity)] static APP_BUILDER: RefCell String>>> = const { RefCell::new(None) }; } +/// Arm a directory `FileMonitor` into `slot`, and if `monitor_directory` fails +/// (a dir race at login, a transient permissions hiccup) schedule a **bounded** +/// lazy retry instead of giving up for the whole session. +/// +/// The pre-fix code stored `None` on failure; the watch was only ever re-armed +/// as a side effect of another `bind_window*` call, so a consumer that binds +/// exactly once (e.g. `bread-polkit`) lost per-monitor reload permanently after +/// a single failure. `guard` stops a burst of `bind_window*` calls from each +/// spawning their own retry timer. +fn arm_or_retry( + slot: &'static LocalKey>>, + guard: &'static LocalKey>, + arm: fn() -> Option, +) { + if slot.with(|c| c.borrow().is_some()) { + return; + } + if let Some(m) = arm() { + slot.with(|c| *c.borrow_mut() = Some(m)); + return; + } + if guard.with(|g| g.replace(true)) { + return; // a retry loop is already ticking + } + let mut attempts: u32 = 0; + gtk4::glib::timeout_add_seconds_local(2, move || { + attempts += 1; + let armed = slot.with(|c| c.borrow().is_some()) + || match arm() { + Some(m) => { + slot.with(|c| *c.borrow_mut() = Some(m)); + true + } + None => false, + }; + if armed || attempts >= 5 { + guard.with(|g| g.set(false)); + gtk4::glib::ControlFlow::Break + } else { + gtk4::glib::ControlFlow::Continue + } + }); +} + fn reload_shared() { let css = std::fs::read_to_string(crate::shared_css_path()).unwrap_or_else(|_| crate::render()); SHARED_PROVIDER.with(|cell| apply_css(&css, cell)); @@ -83,12 +131,7 @@ fn watch_theme_file(reload: fn()) -> Option { pub fn apply_app_css String + 'static>(build: F) { APP_BUILDER.with(|b| *b.borrow_mut() = Some(Box::new(build))); reload_app(); - APP_MONITOR.with(|cell| { - if cell.borrow().is_some() { - return; - } - *cell.borrow_mut() = watch_theme_file(reload_app); - }); + arm_or_retry(&APP_MONITOR, &APP_RETRY, || watch_theme_file(reload_app)); } /// Load the ecosystem's shared stylesheet (the file written by @@ -100,11 +143,8 @@ pub fn apply_app_css String + 'static>(build: F) { /// app-specific rules win on equal specificity. pub fn apply_shared() { reload_shared(); - SHARED_MONITOR.with(|cell| { - if cell.borrow().is_some() { - return; - } - *cell.borrow_mut() = watch_theme_file(reload_shared); + arm_or_retry(&SHARED_MONITOR, &SHARED_RETRY, || { + watch_theme_file(reload_shared) }); } @@ -162,21 +202,66 @@ pub fn output_for_widget(widget: &impl IsA) -> Option { monitor.connector().map(|c| c.to_string()) } +/// Every `observe_children` model in a bound widget's current subtree, kept +/// alive together (drop them all when the bind is forgotten). Shared so the +/// recursive [`hook_subtree`] can push newly-discovered descendant containers +/// in from inside an `items-changed` closure. +type SubtreeWatches = Rc>>; + struct WidgetBind { output: String, theme: CssProvider, app: Option, app_build: Option, - /// Keep the directory monitor + child model alive for this widget. - _watch: Option, + /// Keep every subtree child-model alive for this widget. + _watch: SubtreeWatches, } thread_local! { static BINDS: RefCell> = RefCell::new(HashMap::new()); static THEMES_MONITOR: RefCell> = const { RefCell::new(None) }; + static THEMES_RETRY: Cell = const { Cell::new(false) }; + static PALETTES_MONITOR: RefCell> = const { RefCell::new(None) }; + static PALETTES_RETRY: Cell = const { Cell::new(false) }; static DESTROY_HOOKED: RefCell> = RefCell::new(HashSet::new()); static AUTO_HOOKED: RefCell> = RefCell::new(HashSet::new()); + static MAP_HOOKED: RefCell> = RefCell::new(HashSet::new()); + /// Surfaces whose `enter-monitor` we've already hooked, keyed by + /// `GdkSurface` pointer (not widget pointer — a widget can be re-realized + /// onto a fresh surface). static ENTER_HOOKED: RefCell> = RefCell::new(HashSet::new()); + /// widget pointer -> the surface pointer currently in [`ENTER_HOOKED`] for + /// it, so [`forget_bind`] can clear the enter hook by the right key when + /// the widget is destroyed. + static ENTER_SURFACE: RefCell> = RefCell::new(HashMap::new()); +} + +/// Drop every thread-local record for a destroyed bound widget. +/// +/// `DESTROY_HOOKED`/`AUTO_HOOKED`/`MAP_HOOKED`/`BINDS` are keyed by the widget +/// pointer; `ENTER_HOOKED` is keyed by the *surface* pointer, so we look the +/// surface up via `ENTER_SURFACE`. Leaving the `ENTER_HOOKED` entry behind used +/// to mean a later window whose surface was allocated at the same address would +/// hit the `already` early-return in [`attach_enter_monitor`] and silently +/// never re-theme when moved between monitors. +fn forget_bind(key: usize) { + BINDS.with(|b| { + b.borrow_mut().remove(&key); + }); + DESTROY_HOOKED.with(|s| { + s.borrow_mut().remove(&key); + }); + AUTO_HOOKED.with(|s| { + s.borrow_mut().remove(&key); + }); + MAP_HOOKED.with(|s| { + s.borrow_mut().remove(&key); + }); + if let Some(surf_key) = ENTER_SURFACE.with(|m| m.borrow_mut().remove(&key)) { + ENTER_HOOKED.with(|s| { + s.borrow_mut().remove(&surf_key); + }); + } } fn widget_key(widget: >k4::Widget) -> usize { @@ -209,47 +294,59 @@ fn ensure_destroy_cleanup(widget: >k4::Widget) { if !inserted { return; } - widget.connect_destroy(move |w| { - let key = widget_key(w); - BINDS.with(|b| { - b.borrow_mut().remove(&key); - }); - DESTROY_HOOKED.with(|s| { - s.borrow_mut().remove(&key); - }); - AUTO_HOOKED.with(|s| { - s.borrow_mut().remove(&key); - }); - }); + widget.connect_destroy(move |w| forget_bind(widget_key(w))); } -fn ensure_themes_watch() { - THEMES_MONITOR.with(|cell| { - if cell.borrow().is_some() { +/// `themes/.` (or `palettes/.`) → the file stem, iff the +/// event path actually carries `want_ext`. Factored out so the reload-routing +/// logic is unit-testable without a `FileMonitor`. +fn reload_stem_for_event(path: &Path, want_ext: &str) -> Option { + if path.extension().and_then(|e| e.to_str()) != Some(want_ext) { + return None; + } + path.file_stem() + .and_then(|s| s.to_str()) + .map(|s| s.to_string()) +} + +fn arm_dir_reload_watch(dir: std::path::PathBuf, want_ext: &'static str) -> Option { + let _ = std::fs::create_dir_all(&dir); + let monitor = gio::File::for_path(&dir) + .monitor_directory(gio::FileMonitorFlags::WATCH_MOVES, gio::Cancellable::NONE) + .ok()?; + monitor.connect_changed(move |_, file, other, _event| { + let path = file.path().or_else(|| other.and_then(|f| f.path())); + let Some(stem) = path.as_deref().and_then(|p| reload_stem_for_event(p, want_ext)) else { return; - } - let dir = crate::themes_dir(); - let _ = std::fs::create_dir_all(&dir); - let monitor = gio::File::for_path(&dir) - .monitor_directory(gio::FileMonitorFlags::WATCH_MOVES, gio::Cancellable::NONE) - .ok(); - if let Some(ref m) = monitor { - m.connect_changed(move |_, file, other, _event| { - let path = file.path().or_else(|| other.and_then(|f| f.path())); - let Some(path) = path else { - return; - }; - if path.extension().and_then(|e| e.to_str()) != Some("css") { - return; - } - let Some(stem) = path.file_stem().and_then(|s| s.to_str()) else { - return; - }; - reload_binds_for_sanitized(stem); - }); - } - *cell.borrow_mut() = monitor; + }; + reload_binds_for_sanitized(&stem); }); + Some(monitor) +} + +fn try_arm_themes_watch() -> Option { + arm_dir_reload_watch(crate::themes_dir(), "css") +} + +/// Watch `themes/.css` so a per-output pywal regenerate recolours every +/// window bound to that output in place. Bounded lazy retry: a transient +/// `monitor_directory` failure no longer kills per-monitor reload for the +/// session (findings #5). +fn ensure_themes_watch() { + arm_or_retry(&THEMES_MONITOR, &THEMES_RETRY, try_arm_themes_watch); +} + +fn try_arm_palettes_watch() -> Option { + arm_dir_reload_watch(crate::palettes_dir(), "json") +} + +/// Also watch `palettes/.json` (finding #6): the CLI writes the `.json` +/// and the `.css` together, but third-party tooling that only rewrites the +/// palette JSON would otherwise leave every bound window stale — the reload +/// path re-reads the JSON via `load_palette_for` regardless, so reacting to +/// either file is correct (a paired write just reloads twice, harmlessly). +fn ensure_palettes_watch() { + arm_or_retry(&PALETTES_MONITOR, &PALETTES_RETRY, try_arm_palettes_watch); } fn reload_binds_for_sanitized(sanitized: &str) { @@ -268,21 +365,56 @@ fn reload_binds_for_sanitized(sanitized: &str) { }); } -fn watch_root_children(widget: >k4::Widget) -> gio::ListModel { +/// Re-run [`attach_tree`] over the whole bound subtree of `root`. +fn reattach_bound_tree(root: >k4::Widget) { + let key = widget_key(root); + BINDS.with(|binds| { + if let Some(bind) = binds.borrow().get(&key) { + attach_tree(root, &bind.theme, bind.app.as_ref()); + } + }); +} + +/// Recursively hook `observe_children` on `widget` *and every current +/// descendant container*, so a subtree added lazily at **any** depth after the +/// bind — a popover's contents, menu items, rows appended into an existing box +/// — gets the per-output provider too (finding #3). +/// +/// The pre-fix code only watched the root's *direct* children, so anything +/// deeper than one level that appeared after bind/map silently rendered with +/// the display-wide shared sheet (the wrong monitor's colours). Every model is +/// parked in `watches` (which the `WidgetBind` owns) so the whole set is +/// dropped together when the bind is forgotten; newly-appeared containers are +/// hooked in from inside the `items-changed` closure. +fn hook_subtree(widget: >k4::Widget, root: &glib::WeakRef, watches: &SubtreeWatches) { let model = widget.observe_children(); - let root = widget.downgrade(); - model.connect_items_changed(move |_, _, _, _| { - let Some(root) = root.upgrade() else { - return; - }; - let key = widget_key(&root); - BINDS.with(|binds| { - if let Some(bind) = binds.borrow().get(&key) { - attach_tree(&root, &bind.theme, bind.app.as_ref()); + { + let root = root.clone(); + let watches = watches.clone(); + model.connect_items_changed(move |model, pos, _removed, added| { + let Some(root_w) = root.upgrade() else { + return; + }; + reattach_bound_tree(&root_w); + for i in pos..pos + added { + if let Some(child) = model.item(i).and_downcast::() { + hook_subtree(&child, &root, &watches); + } } }); - }); - model + } + watches.borrow_mut().push(model); + let mut child = widget.first_child(); + while let Some(c) = child { + hook_subtree(&c, root, watches); + child = c.next_sibling(); + } +} + +fn watch_subtree(widget: >k4::Widget) -> SubtreeWatches { + let watches: SubtreeWatches = Rc::new(RefCell::new(Vec::new())); + hook_subtree(widget, &widget.downgrade(), &watches); + watches } fn bind_window_inner( @@ -331,7 +463,7 @@ fn bind_window_inner( attach_tree(widget, &theme, app.as_ref()); - let child_model = watch_root_children(widget); + let watches = watch_subtree(widget); map.insert( key, WidgetBind { @@ -339,38 +471,27 @@ fn bind_window_inner( theme, app, app_build, - _watch: Some(child_model), + _watch: watches, }, ); }); ensure_destroy_cleanup(widget); ensure_themes_watch(); + ensure_palettes_watch(); ensure_map_reattach(widget); } fn ensure_map_reattach(widget: >k4::Widget) { - // `connect_map` once per widget — re-bind already lives in BINDS. - thread_local! { - static MAP_HOOKED: RefCell> = RefCell::new(HashSet::new()); - } + // `connect_map` once per widget — re-bind already lives in BINDS. Covers the + // whole-window-remapped case; `watch_subtree` covers lazily-grown subtrees. + // `MAP_HOOKED` is cleared by `forget_bind` on destroy. let key = widget_key(widget); let inserted = MAP_HOOKED.with(|s| s.borrow_mut().insert(key)); if !inserted { return; } - widget.connect_map(|w| { - BINDS.with(|binds| { - if let Some(bind) = binds.borrow().get(&widget_key(w)) { - attach_tree(w, &bind.theme, bind.app.as_ref()); - } - }); - }); - widget.connect_destroy(move |_| { - MAP_HOOKED.with(|s| { - s.borrow_mut().remove(&key); - }); - }); + widget.connect_map(reattach_bound_tree); } /// Attach a widget-level `CssProvider` with @@ -399,6 +520,10 @@ fn attach_enter_monitor(widget: >k4::Widget, build: Option) { return; }; let surf_key = surface.as_ptr() as usize; + // Record widget -> surface *before* the dedupe check so `forget_bind` can + // always clear the right `ENTER_HOOKED` key on destroy, even when this call + // early-returns because the surface was already hooked. + ENTER_SURFACE.with(|m| m.borrow_mut().insert(widget_key(widget), surf_key)); let already = ENTER_HOOKED.with(|s| !s.borrow_mut().insert(surf_key)); if already { return; @@ -493,3 +618,166 @@ pub fn apply_user_css(path: &Path, provider: &RefCell>) { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn nanos() -> u128 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + } + + // ---- pure logic (no display needed) -------------------------------------- + + #[test] + fn reload_stem_for_event_requires_the_right_extension() { + assert_eq!( + reload_stem_for_event(Path::new("/run/user/1/bread/themes/eDP-1.css"), "css") + .as_deref(), + Some("eDP-1") + ); + assert_eq!( + reload_stem_for_event(Path::new("/x/palettes/HDMI-A-1.json"), "json").as_deref(), + Some("HDMI-A-1") + ); + // wrong extension → no reload (this is the finding #6 seam: the palettes + // watch must accept `.json`, the themes watch only `.css`) + assert_eq!( + reload_stem_for_event(Path::new("/x/themes/eDP-1.json"), "css"), + None + ); + assert_eq!( + reload_stem_for_event(Path::new("/x/themes/.eDP-1.css.tmp.42"), "css"), + None + ); + } + + #[test] + fn forget_bind_clears_every_record_including_the_surface_keyed_enter_hook() { + // Regression for finding #2: `ENTER_HOOKED` is keyed by surface pointer, + // the rest by widget pointer. A destroyed window used to leave its + // `ENTER_HOOKED` entry behind forever, so a later window landing on the + // same surface address never re-themed on a monitor move. + let wkey = 0x1234_5678_usize; + let skey = 0x8765_4321_usize; + ENTER_SURFACE.with(|m| { + m.borrow_mut().insert(wkey, skey); + }); + ENTER_HOOKED.with(|s| { + s.borrow_mut().insert(skey); + }); + DESTROY_HOOKED.with(|s| { + s.borrow_mut().insert(wkey); + }); + AUTO_HOOKED.with(|s| { + s.borrow_mut().insert(wkey); + }); + MAP_HOOKED.with(|s| { + s.borrow_mut().insert(wkey); + }); + + forget_bind(wkey); + + assert!( + ENTER_HOOKED.with(|s| !s.borrow().contains(&skey)), + "ENTER_HOOKED entry leaked past destroy" + ); + assert!(ENTER_SURFACE.with(|m| !m.borrow().contains_key(&wkey))); + assert!(DESTROY_HOOKED.with(|s| !s.borrow().contains(&wkey))); + assert!(AUTO_HOOKED.with(|s| !s.borrow().contains(&wkey))); + assert!(MAP_HOOKED.with(|s| !s.borrow().contains(&wkey))); + } + + #[test] + fn dir_reload_watches_arm_and_create_their_directories() { + // Finding #5: arming must actually succeed in a writable runtime dir, + // and `arm_or_retry` only falls back to its bounded timer when it + // genuinely can't. + let _lock = crate::output::XDG_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let dir = std::env::temp_dir().join(format!("bt-gtk-watch-{}-{}", std::process::id(), nanos())); + std::fs::create_dir_all(&dir).unwrap(); + let old = std::env::var("XDG_RUNTIME_DIR").ok(); + std::env::set_var("XDG_RUNTIME_DIR", &dir); + + let themes = try_arm_themes_watch(); + let palettes = try_arm_palettes_watch(); + + let themes_dir_made = crate::themes_dir().is_dir(); + let palettes_dir_made = crate::palettes_dir().is_dir(); + + match old { + Some(v) => std::env::set_var("XDG_RUNTIME_DIR", v), + None => std::env::remove_var("XDG_RUNTIME_DIR"), + } + let _ = std::fs::remove_dir_all(&dir); + + assert!(themes_dir_made && palettes_dir_made, "watch arming must mkdir -p its dir"); + assert!(themes.is_some(), "themes watch failed to arm in a writable dir"); + assert!(palettes.is_some(), "palettes watch failed to arm in a writable dir"); + } + + // ---- widget lifecycle -------------------------------------------------- + // + // These need a real GDK display *and* call `gtk4::init()`, which acquires + // the thread-default glib main context — running them alongside the rest of + // the suite (where `shell::hotreload` tests pump that context to receive + // `FileMonitor` events) deadlocks those. So they're `#[ignore]`d by default + // and the `--features gtk` CI/pre-push command skips them; run them on + // their own: + // + // cargo test -p bread-theme --features gtk -- --ignored --test-threads=1 + // + // They still compile with every build, so they can't silently rot. + + // One test only: `gtk4::init()` binds GTK to the calling thread for the + // life of the process and panics ("two different threads") if any *other* + // test thread calls it afterwards — so all the widget-level assertions + // share a single init here. + #[test] + #[ignore = "needs a GDK display + exclusive glib main context; run alone (see module note)"] + fn gtk_widget_lifecycle_and_deep_subtree_reattach() { + if gtk4::init().is_err() { + return; // headless + } + + // Finding #3a: watch_subtree hooks every container, not just the root. + let root = gtk4::Box::new(gtk4::Orientation::Vertical, 0); + let mid = gtk4::Box::new(gtk4::Orientation::Vertical, 0); + let leaf = gtk4::Box::new(gtk4::Orientation::Vertical, 0); + root.append(&mid); + mid.append(&leaf); + let watches = watch_subtree(root.upcast_ref()); + assert!( + watches.borrow().len() >= 3, + "expected a child-model per container (root/mid/leaf), got {}", + watches.borrow().len() + ); + drop(watches); + + // Finding #3b: a grandchild appended *after* the bind must trigger the + // recursive items-changed hook on `mid` and get re-walked. + let root = gtk4::Box::new(gtk4::Orientation::Vertical, 0); + let mid = gtk4::Box::new(gtk4::Orientation::Vertical, 0); + root.append(&mid); + bind_window(&root, "eDP-1"); + let key = widget_key(root.upcast_ref()); + assert!(BINDS.with(|b| b.borrow().contains_key(&key))); + + let before = BINDS.with(|b| b.borrow().get(&key).unwrap()._watch.borrow().len()); + mid.append(>k4::Label::new(Some("added late"))); + let after = BINDS.with(|b| b.borrow().get(&key).unwrap()._watch.borrow().len()); + assert!( + after > before, + "a widget added two levels below the bound root never triggered re-attach ({before} -> {after})" + ); + + // Finding #2 sibling: forget_bind drops the record. + forget_bind(key); + assert!(BINDS.with(|b| !b.borrow().contains_key(&key))); + } +} diff --git a/bread-theme/src/lib.rs b/bread-theme/src/lib.rs index a9ab77c..00fc163 100644 --- a/bread-theme/src/lib.rs +++ b/bread-theme/src/lib.rs @@ -318,11 +318,38 @@ pub fn write_shared_css() -> std::io::Result { write_shared_css_from(&load_palette()) } -/// `stylesheet()` with `@name` references in rule bodies replaced by hex. -/// Longer names first (`on-surface` before `surface`, `on-bg` before `bg`) -/// so a prefix match cannot half-replace `@on-bg`. +/// `stylesheet()` with `@name` references in rule bodies replaced by hex **and +/// the leading `@define-color` block stripped entirely**. +/// +/// This variant is only ever loaded by the per-monitor bind path +/// (`gtk::bind_window*` / `reload_binds_for_sanitized`), which attaches it as a +/// widget-tree `CssProvider` for one output's palette. Two things matter there: +/// +/// 1. Every rule body is already hex (longer names first — `on-surface` before +/// `surface`, `on-bg` before `bg` — so a prefix match cannot half-replace +/// `@on-bg`), so the sheet needs no `@define-color` block to resolve. +/// 2. `@define-color` in GTK4 is **stylesheet-global**, not provider- or +/// subtree-scoped (confirmed by GTK's CSS maintainers). A per-monitor +/// provider that still emitted the block would redefine the display-global +/// `@accent`/`@bg`/`@on-*` to *this* monitor's palette on every bind and +/// every per-output reload — so `apply_shared`, `apply_app_css`, +/// `apply_user_css` and any CSS parsed afterwards in another window would +/// resolve their named colours against whichever monitor bound last. That is +/// exactly the "wrong monitor's accent leaks" failure the hex-inlining was +/// added to prevent, just displaced out of the bound tree. +/// +/// The display-global sheet ([`stylesheet`] / [`render`], loaded by +/// [`gtk::apply_shared`] at APPLICATION priority) keeps its `@define-color` +/// block — that is the one provider that is *supposed* to own those names. pub fn stylesheet_resolved(p: &Palette) -> String { - resolve_color_names(&stylesheet(p), p) + let resolved = resolve_color_names(&stylesheet(p), p); + let mut out: String = resolved + .lines() + .filter(|l| !l.trim_start().starts_with("@define-color")) + .collect::>() + .join("\n"); + out.push('\n'); + out } /// Replace `@define-color` names (`@accent`, `@on-bg`, …) with hex values. @@ -551,34 +578,32 @@ mod tests { }; let css = stylesheet_resolved(&p); assert!(css.contains("#7aa2f7"), "color4 must appear as hex: {css}"); - // Rule bodies must not keep named colors — GTK display-global - // @define-color would otherwise leak the wrong monitor's accent. - let rules = css - .lines() - .filter(|l| !l.trim_start().starts_with("@define-color")) - .collect::>() - .join("\n"); + // The whole `@define-color` block must be gone: it is stylesheet-global + // in GTK4, so a per-monitor provider that kept it would clobber the + // display-global @accent/@bg/@on-* with this output's palette. assert!( - !rules.contains("@accent"), - "leftover @accent in rules:\n{rules}" - ); - assert!( - !rules.contains("@on-bg"), - "leftover @on-bg in rules:\n{rules}" - ); - assert!( - !rules.contains("@on-surface"), - "leftover @on-surface in rules:\n{rules}" - ); - assert!( - !rules.contains("@on-accent"), - "leftover @on-accent in rules:\n{rules}" + !css.contains("@define-color"), + "stylesheet_resolved must not emit any @define-color line:\n{css}" ); + // ...and no rule body may keep a named colour either. + assert!(!css.contains("@accent"), "leftover @accent:\n{css}"); + assert!(!css.contains("@on-bg"), "leftover @on-bg:\n{css}"); + assert!(!css.contains("@on-surface"), "leftover @on-surface:\n{css}"); + assert!(!css.contains("@on-accent"), "leftover @on-accent:\n{css}"); + assert!(!css.contains("@bg"), "leftover @bg:\n{css}"); // Longer names first: @on-bg must not become @on-#... - assert!( - !rules.contains("@on-#"), - "half-replaced on-* name:\n{rules}" - ); + assert!(!css.contains("@on-#"), "half-replaced on-* name:\n{css}"); + // The component rules themselves must survive the filter. + assert!(css.contains("button.suggested-action"), "rules dropped:\n{css}"); + assert!(css.contains("#7aa2f7"), "accent hex must reach rule bodies"); + } + + #[test] + fn render_and_stylesheet_keep_define_color_block() { + // The display-global sheet is the one provider that *should* own the + // @define-color names — only the per-bind `_resolved` variant drops it. + assert!(render().contains("@define-color accent ")); + assert!(stylesheet(&Palette::default()).contains("@define-color accent ")); } #[test] diff --git a/bread-theme/src/output.rs b/bread-theme/src/output.rs index ebf3a1b..7733001 100644 --- a/bread-theme/src/output.rs +++ b/bread-theme/src/output.rs @@ -22,23 +22,31 @@ pub(crate) fn runtime_bread_dir() -> PathBuf { .join("bread") } -/// Keep `[A-Za-z0-9._-]`; replace everything else with `_`. +/// Turn an output/connector name into a single path segment for +/// `palettes/.json` / `themes/.css`. +/// +/// Keep `[A-Za-z0-9._-]` verbatim (every real Hyprland connector — `eDP-1`, +/// `HDMI-A-1`, `DP-2` — is already only those); **percent-encode** every other +/// byte as `%XX` (upper-hex). Percent-encoding is injective, so two different +/// connectors can never land on the same file: the old scheme mapped `/`, `:` +/// and space all to `_`, so `a/b` and `a:b` both became `a_b` and silently +/// shared one palette + stylesheet, last writer wins. `%` itself is not in the +/// keep-set, so it encodes to `%25` and the mapping stays reversible in +/// principle (nothing needs to decode today — [`reload_binds_for_sanitized`] +/// only compares `sanitize_output(name)` against the on-disk file stem). pub fn sanitize_output(output: &str) -> String { - let s: String = output - .chars() - .map(|c| { - if c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-') { - c - } else { - '_' - } - }) - .collect(); - if s.is_empty() { - "_".into() - } else { - s + if output.is_empty() { + return "_".into(); } + let mut s = String::with_capacity(output.len()); + for b in output.bytes() { + if b.is_ascii_alphanumeric() || matches!(b, b'.' | b'_' | b'-') { + s.push(b as char); + } else { + s.push_str(&format!("%{b:02X}")); + } + } + s } pub fn themes_dir() -> PathBuf { @@ -268,11 +276,26 @@ mod tests { } #[test] - fn sanitize_output_replaces_unsafe_chars() { - assert_eq!(sanitize_output("HDMI A:1"), "HDMI_A_1"); - assert_eq!(sanitize_output("foo/bar"), "foo_bar"); + fn sanitize_output_percent_encodes_unsafe_chars() { + assert_eq!(sanitize_output("HDMI A:1"), "HDMI%20A%3A1"); + assert_eq!(sanitize_output("foo/bar"), "foo%2Fbar"); assert_eq!(sanitize_output(""), "_"); assert_eq!(sanitize_output("..ok_name-1"), "..ok_name-1"); + assert_eq!(sanitize_output("a%b"), "a%25b"); + } + + #[test] + fn sanitize_output_is_injective_across_old_collisions() { + // The old `_`-for-everything scheme collapsed all of these together. + let names = ["a/b", "a:b", "a b", "a_b", "a%2Fb"]; + let mut seen = std::collections::HashSet::new(); + for n in names { + assert!( + seen.insert(sanitize_output(n)), + "collision on {n} -> {}", + sanitize_output(n) + ); + } } #[test] @@ -281,8 +304,8 @@ mod tests { std::env::set_var("XDG_RUNTIME_DIR", "/run/user/1234"); let css = output_css_path("HDMI A:1"); let pal = output_palette_path("HDMI A:1"); - assert_eq!(css, themes_dir().join("HDMI_A_1.css")); - assert_eq!(pal, palettes_dir().join("HDMI_A_1.json")); + assert_eq!(css, themes_dir().join("HDMI%20A%3A1.css")); + assert_eq!(pal, palettes_dir().join("HDMI%20A%3A1.json")); assert!(css.starts_with(themes_dir())); assert!(pal.starts_with(palettes_dir())); assert_eq!(