Breadway/bread-ecosystem
1
0
Fork 0
Code Issues 1 Pull requests 4 Projects Releases Packages Wiki Activity Actions

bread-theme: fix per-monitor colour-theming leaks (define-color, bind lifecycle, watch arming) #7

Open
Breadway wants to merge 0 commits from fix/per-monitor-colour-leaks into main
pull from: fix/per-monitor-colour-leaks
merge into: Breadway:main
Conversation 0 Commits - Files changed - +466 -130
Breadway commented 2026-09-01 16:29:06 +08:00
Owner
Copy link

Fixes the six bread-ecosystem findings from the multi-monitor audit: per-monitor accent colours leaking across a multi-output Hyprland desktop.

Per-finding checklist

  • #1 stylesheet_resolved still emits @define-color — FIXED. The per-bind variant now strips every @define-color line; its rule bodies are already fully hex, so the block was pure liability. @define-color is stylesheet-global in GTK4, so keeping it meant every per-monitor bind/reload clobbered the display-global @accent/@bg/@on-* with that monitor's palette, and any CSS parsed afterwards in another window (apply_shared, apply_app_css, apply_user_css) resolved against whichever monitor bound last. The display-global stylesheet() / render() sheet keeps its block. Existing regression test tightened to assert the block is absent; new test guards the display-global sheet still carries it.
  • #2 ENTER_HOOKED never cleaned up on destroy — FIXED. It's keyed by GdkSurface pointer while the sibling sets are keyed by widget pointer, so ensure_destroy_cleanup never removed it. Now a widget -> surface mapping (ENTER_SURFACE) is recorded at hook time and the unified forget_bind (single connect_destroy, also folding in the previously-separate MAP_HOOKED cleanup) clears the enter hook by its real key.
  • #3 Descendants added deeper than one level after binding never get the sheet — FIXED. watch_root_children (root's direct children only) replaced with hook_subtree: recursively observes every current container and, from inside items-changed, re-walks the whole bound tree and hooks any newly-spliced-in containers. Child-models are owned by the WidgetBind so they drop together on destroy. The existing connect_map full-subtree re-attach is kept for the whole-window-remap case. Note: this registers one observe_children + signal per container at bind time — O(widgets in tree), which the audit explicitly sanctioned; acceptable for the window sizes involved.
  • #4 sanitize_output collisions — FIXED. Replaced the everything-to-_ mapping with percent-encoding of every byte outside [A-Za-z0-9._-]. Injective, so a/b and a:b can no longer share one palette/CSS file. Real Hyprland connector names are unaffected (already keep-set only); files are session-scoped and regenerated at login.
  • #5 One-shot watch arming — FIXED. ensure_themes_watch and the apply_shared / apply_app_css watchers stored None on a monitor_directory failure and were only re-armed as a side effect of another bind — so a bind-once consumer (bread-polkit) lost per-monitor reload for the session. New arm_or_retry helper: bounded lazy retry (5 attempts, 2s apart), guarded so a burst of binds doesn't each spawn a timer.
  • #6 Palette-only updates don't reach binds — FIXED. Reloads keyed strictly on .css events under themes/; a write of only palettes/<out>.json left bound windows stale (third-party tooling — the CLI writes both). Now also watches palettes/ for .json; the reload path re-reads the JSON via load_palette_for regardless, so reacting to either file is correct (a paired write reloads twice, harmlessly).
  • Coverage gap — the per-monitor GTK bind module had zero tests. Added: pure-logic tests for forget_bind's surface-keyed cleanup (#2), the css/json reload routing (#6), and watch arming (#5); plus one #[ignore]d display+GTK test covering the deep-subtree re-attach and bind lifecycle (#2/#3).

Test notes

  • cargo build --workspace / --features gtk: clean.
  • cargo clippy --workspace --all-targets and --features gtk with -D warnings: clean.
  • cargo test --workspace and cargo test --workspace --features gtk: all pass (note bread-polkit pulls bread-theme with the gtk feature, so the new gtk.rs tests run under a plain cargo test --workspace too).
  • The one #[ignore]d test needs a GDK display and calls gtk4::init(), which binds GTK to a single thread for the process and acquires the thread-default glib main context — running it in the shared test binary deadlocks the shell::hotreload tests (they pump that context for FileMonitor events). It still compiles on every build and passes when run alone:
    cargo test -p bread-theme --features gtk -- --ignored

CI wiring (not changed here)

.forgejo/workflows/dev-bread-theme.yml only runs cargo build ... --bin bread-theme — no cargo test, and nothing with --features gtk. So none of bread-theme's tests (new or old) run in CI today; only bakery has a cargo test step. Worth wiring a cargo test -p bread-theme --features gtk step (it needs system GTK4 on the runner, plus a headless display or the #[ignore] skip covers it), but that's left as a separate change to the workflow files.

Fixes the six bread-ecosystem findings from the multi-monitor audit: per-monitor accent colours leaking across a multi-output Hyprland desktop. ## Per-finding checklist - [x] **#1 `stylesheet_resolved` still emits `@define-color`** — FIXED. The per-bind variant now strips every `@define-color` line; its rule bodies are already fully hex, so the block was pure liability. `@define-color` is stylesheet-global in GTK4, so keeping it meant every per-monitor bind/reload clobbered the display-global `@accent`/`@bg`/`@on-*` with that monitor's palette, and any CSS parsed afterwards in another window (`apply_shared`, `apply_app_css`, `apply_user_css`) resolved against whichever monitor bound last. The display-global `stylesheet()` / `render()` sheet keeps its block. Existing regression test tightened to assert the block is absent; new test guards the display-global sheet still carries it. - [x] **#2 `ENTER_HOOKED` never cleaned up on destroy** — FIXED. It's keyed by `GdkSurface` pointer while the sibling sets are keyed by widget pointer, so `ensure_destroy_cleanup` never removed it. Now a `widget -> surface` mapping (`ENTER_SURFACE`) is recorded at hook time and the unified `forget_bind` (single `connect_destroy`, also folding in the previously-separate `MAP_HOOKED` cleanup) clears the enter hook by its real key. - [x] **#3 Descendants added deeper than one level after binding never get the sheet** — FIXED. `watch_root_children` (root's direct children only) replaced with `hook_subtree`: recursively observes every current container and, from inside `items-changed`, re-walks the whole bound tree and hooks any newly-spliced-in containers. Child-models are owned by the `WidgetBind` so they drop together on destroy. The existing `connect_map` full-subtree re-attach is kept for the whole-window-remap case. Note: this registers one `observe_children` + signal per container at bind time — O(widgets in tree), which the audit explicitly sanctioned; acceptable for the window sizes involved. - [x] **#4 `sanitize_output` collisions** — FIXED. Replaced the everything-to-`_` mapping with percent-encoding of every byte outside `[A-Za-z0-9._-]`. Injective, so `a/b` and `a:b` can no longer share one palette/CSS file. Real Hyprland connector names are unaffected (already keep-set only); files are session-scoped and regenerated at login. - [x] **#5 One-shot watch arming** — FIXED. `ensure_themes_watch` and the `apply_shared` / `apply_app_css` watchers stored `None` on a `monitor_directory` failure and were only re-armed as a side effect of another bind — so a bind-once consumer (`bread-polkit`) lost per-monitor reload for the session. New `arm_or_retry` helper: bounded lazy retry (5 attempts, 2s apart), guarded so a burst of binds doesn't each spawn a timer. - [x] **#6 Palette-only updates don't reach binds** — FIXED. Reloads keyed strictly on `.css` events under `themes/`; a write of only `palettes/<out>.json` left bound windows stale (third-party tooling — the CLI writes both). Now also watches `palettes/` for `.json`; the reload path re-reads the JSON via `load_palette_for` regardless, so reacting to either file is correct (a paired write reloads twice, harmlessly). - [x] **Coverage gap** — the per-monitor GTK bind module had zero tests. Added: pure-logic tests for `forget_bind`'s surface-keyed cleanup (#2), the css/json reload routing (#6), and watch arming (#5); plus one `#[ignore]`d display+GTK test covering the deep-subtree re-attach and bind lifecycle (#2/#3). ## Test notes - `cargo build --workspace` / `--features gtk`: clean. - `cargo clippy --workspace --all-targets` and `--features gtk` with `-D warnings`: clean. - `cargo test --workspace` and `cargo test --workspace --features gtk`: all pass (note `bread-polkit` pulls `bread-theme` with the `gtk` feature, so the new `gtk.rs` tests run under a plain `cargo test --workspace` too). - The one `#[ignore]`d test needs a GDK display and calls `gtk4::init()`, which binds GTK to a single thread for the process and acquires the thread-default glib main context — running it in the shared test binary deadlocks the `shell::hotreload` tests (they pump that context for `FileMonitor` events). It still compiles on every build and passes when run alone: `cargo test -p bread-theme --features gtk -- --ignored` ## CI wiring (not changed here) `.forgejo/workflows/dev-bread-theme.yml` only runs `cargo build ... --bin bread-theme` — no `cargo test`, and nothing with `--features gtk`. So none of bread-theme's tests (new or old) run in CI today; only `bakery` has a `cargo test` step. Worth wiring a `cargo test -p bread-theme --features gtk` step (it needs system GTK4 on the runner, plus a headless display or the `#[ignore]` skip covers it), but that's left as a separate change to the workflow files.
Breadway added 3 commits 2026-09-01 16:29:07 +08:00 
stylesheet_resolved() is loaded *only* by the per-monitor bind path
(gtk::bind_window* / reload_binds_for_sanitized), which attaches it as a
widget-tree CssProvider carrying one output's palette. It already hex-
inlines every rule body, but it still emitted the leading @define-color
accent/bg/on-* block verbatim.

@define-color in GTK4 is stylesheet-global -- not provider- or subtree-
scoped (confirmed by GTK's CSS maintainers). So every bind and every
per-output reload redefined the display-global @accent/@bg/@on-* to that
monitor's palette, and any CSS parsed afterwards in another window --
apply_shared in every other bread app, apply_app_css, apply_user_css --
resolved its 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.

Drop every @define-color line from the _resolved output: the rules are
fully self-contained hex, so the block was pure liability there. The
display-global sheet (stylesheet() / render(), loaded at APPLICATION
priority) keeps its block -- that is the one provider meant to own those
names. Tightens the existing regression test to assert the block is now
actually absent, and adds one guarding the display-global sheet still
carries it.
The old sanitiser mapped '/', ':' and space -- and every other unsafe
byte -- all to '_', so "a/b" and "a:b" both became "a_b" and silently
shared one palettes/<name>.json + themes/<name>.css, last writer wins.
No collision detection at all.

Percent-encode every byte outside [A-Za-z0-9._-] as %XX instead. That is
injective, so two connectors can never land on the same file. '%' itself
isn't in the keep-set so it encodes to %25 and the mapping stays
reversible in principle (nothing decodes today -- reload_binds_for_
sanitized only compares sanitize_output(name) against the on-disk stem).

Real Hyprland connectors (eDP-1, HDMI-A-1, DP-2) are already only
keep-set bytes, so their filenames are unchanged; the files are
session-scoped under XDG_RUNTIME_DIR and regenerated at login anyway.
Four separate per-monitor theming bugs in the window-bind module, plus
the first tests the module has ever had.

ENTER_HOOKED never cleaned up on destroy (#2). It's keyed by GdkSurface
pointer and inserted once per surface, but ensure_destroy_cleanup only
removed the widget-pointer-keyed sets (DESTROY_HOOKED / AUTO_HOOKED). A
closed window left its ENTER_HOOKED entry forever, so a later window
whose surface was allocated at the same address hit the `already` early-
return in attach_enter_monitor and silently never re-themed when moved
between monitors. Record widget->surface in ENTER_SURFACE at hook time so
the single connect_destroy path (now forget_bind, also folding in the
previously separate MAP_HOOKED cleanup) can clear the enter hook by its
real key.

Subtrees grown deeper than one level after bind never got the sheet (#3).
watch_root_children only observed the root's *direct* children, and a
style-context provider isn't inherited by children in GTK4 -- so a
popover's contents, menu items, or rows appended into an existing box
after bind/map rendered with the display-wide shared sheet (the wrong
monitor's colours). Replace it with hook_subtree: recursively observe
every current container and, from inside items-changed, re-walk the tree
and hook any newly-spliced-in containers too. All the child-models are
parked in the WidgetBind so they drop together on destroy.

One-shot watch arming (#5). ensure_themes_watch / the apply_shared /
apply_app_css watchers stored None on a monitor_directory failure; the
watch was only ever re-armed as a side effect of another bind_window*
call, so a consumer that binds exactly once (bread-polkit) lost per-
monitor reload permanently after a single transient failure. Add
arm_or_retry: a bounded lazy retry (5 attempts, 2s apart) guarded so a
burst of binds doesn't each spawn a timer.

Palette-only writes didn't reach binds (#6). Reloads keyed strictly on
.css events under themes/; writing only palettes/<out>.json left bound
windows stale (bites third-party tooling -- the CLI writes both). Also
watch palettes/ for .json; 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).

Tests: the module was feature-gated behind `gtk` and had zero coverage
(the audit noted #2/#3/#5 are "exactly what tests would have caught").
Adds pure-logic tests for forget_bind's surface-keyed cleanup, the
css/json reload routing, and watch arming; plus one #[ignore]d
display+GTK test for the deep-subtree re-attach and bind lifecycle
(gtk4::init() binds GTK to one thread for the process and pumps the glib
main context, which deadlocks the shell::hotreload tests -- so it can't
run in the shared binary; it still compiles every build).
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/per-monitor-colour-leaks:fix/per-monitor-colour-leaks
git checkout fix/per-monitor-colour-leaks

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main
git merge --no-ff fix/per-monitor-colour-leaks
git checkout fix/per-monitor-colour-leaks
git rebase main
git checkout main
git merge --ff-only fix/per-monitor-colour-leaks
git checkout fix/per-monitor-colour-leaks
git rebase main
git checkout main
git merge --no-ff fix/per-monitor-colour-leaks
git checkout main
git merge --squash fix/per-monitor-colour-leaks
git checkout main
git merge --ff-only fix/per-monitor-colour-leaks
git checkout main
git merge fix/per-monitor-colour-leaks
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Breadway/bread-ecosystem#7
No description provided.
Delete branch "fix/per-monitor-colour-leaks"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?