fix: bread-theme per-output hot-reload watch reads rename source, not destination (ECO-14) #16

Closed

Breadway wants to merge 0 commits from fix/eco-14-output-theme-rename-watch into main

Breadway commented

2026-09-05 02:20:42 +08:00

Owner

Bug

arm_dir_reload_watch in bread-theme/src/gtk.rs (behind ensure_themes_watch() / ensure_palettes_watch(), which drive live per-monitor recolouring for every window bound via bind_window/bind_window_auto/*_with_app_css) never actually fired on a real write.

atomic_write (bread-theme/src/output.rs) writes every themes/<output>.css / palettes/<output>.json update as write-to-.{name}.tmp.{pid}-then-rename(). With GFileMonitorFlags::WATCH_MOVES set (which this watch requests), that rename lands as a single connect_changed event where file is the source (tmp) path and other is the destination (final) path.

The callback read:

let path = file.path().or_else(|| other.and_then(|f| f.path()));

file.path() is Some for a valid tmp path, so other — the only field carrying the correct .css/.json extension — was never consulted. reload_stem_for_event then ran against the tmp filename (e.g. .eDP-1.css.tmp.12345), failed its extension check, and the event was silently dropped. watch_theme_file (the shared-stylesheet watcher two functions above) already checks both file and other correctly, which is how this diverged.

Confirmed against real GIO FileMonitorEvent/WATCH_MOVES semantics and the file's own doc comments.

Fix

bread-theme/src/gtk.rs: extracted the file/other selection into a small pure function, reload_event_path, which prefers other (the rename destination) and falls back to file (for a direct CREATE/CHANGED with no rename, e.g. a non-atomic editor write). arm_dir_reload_watch now calls it instead of inlining the wrong precedence.

Tests

Added reload_event_path_prefers_the_rename_destination_over_the_tmp_source, a pure unit test mirroring the existing reload_stem_for_event_requires_the_right_extension style.

A full FileMonitor + real write-then-rename integration test was also tried (arming a real monitor, performing the literal atomic_write sequence, pumping the default glib::MainContext) — it passed in isolation but collided with shell::hotreload's own context-pumping tests under the default parallel test runner (the same runner CI uses: cargo test --workspace --locked --features gtk), aborting the whole test binary via glib's thread guard. Dropped in favor of the pure-function unit test, per the existing precedent in this file for keeping GTK/GIO-event-loop-dependent tests isolated (see the #[ignore]d widget-lifecycle tests' module note).

cargo build --release and cargo test --release --workspace --locked --features gtk pass, run 3x with no flakiness.

Fixes ECO-14.

## Bug `arm_dir_reload_watch` in `bread-theme/src/gtk.rs` (behind `ensure_themes_watch()` / `ensure_palettes_watch()`, which drive live per-monitor recolouring for every window bound via `bind_window`/`bind_window_auto`/`*_with_app_css`) never actually fired on a real write. `atomic_write` (`bread-theme/src/output.rs`) writes every `themes/<output>.css` / `palettes/<output>.json` update as write-to-`.{name}.tmp.{pid}`-then-`rename()`. With `GFileMonitorFlags::WATCH_MOVES` set (which this watch requests), that rename lands as a single `connect_changed` event where `file` is the **source** (tmp) path and `other` is the **destination** (final) path. The callback read: ```rust let path = file.path().or_else(|| other.and_then(|f| f.path())); ``` `file.path()` is `Some` for a valid tmp path, so `other` — the only field carrying the correct `.css`/`.json` extension — was never consulted. `reload_stem_for_event` then ran against the tmp filename (e.g. `.eDP-1.css.tmp.12345`), failed its extension check, and the event was silently dropped. `watch_theme_file` (the shared-stylesheet watcher two functions above) already checks both `file` and `other` correctly, which is how this diverged. Confirmed against real GIO `FileMonitorEvent`/`WATCH_MOVES` semantics and the file's own doc comments. ## Fix `bread-theme/src/gtk.rs`: extracted the file/other selection into a small pure function, `reload_event_path`, which prefers `other` (the rename destination) and falls back to `file` (for a direct CREATE/CHANGED with no rename, e.g. a non-atomic editor write). `arm_dir_reload_watch` now calls it instead of inlining the wrong precedence. ## Tests Added `reload_event_path_prefers_the_rename_destination_over_the_tmp_source`, a pure unit test mirroring the existing `reload_stem_for_event_requires_the_right_extension` style. A full `FileMonitor` + real write-then-rename integration test was also tried (arming a real monitor, performing the literal `atomic_write` sequence, pumping the default `glib::MainContext`) — it passed in isolation but collided with `shell::hotreload`'s own context-pumping tests under the default parallel test runner (the same runner CI uses: `cargo test --workspace --locked --features gtk`), aborting the whole test binary via glib's thread guard. Dropped in favor of the pure-function unit test, per the existing precedent in this file for keeping GTK/GIO-event-loop-dependent tests isolated (see the `#[ignore]`d widget-lifecycle tests' module note). `cargo build --release` and `cargo test --release --workspace --locked --features gtk` pass, run 3x with no flakiness. Fixes ECO-14.

Breadway added 1 commit

2026-09-05 02:20:42 +08:00

fix: arm_dir_reload_watch reads rename source path, not destination 2f364bed9f

bread-theme's per-output theme/palette hot-reload watch never fired on a
real write: with WATCH_MOVES set, a rename lands as one event where
`file` is the source (the .tmp.<pid> atomic_write writes to) and `other`
is the destination (the real .css/.json target). The callback took
file.path() unconditionally whenever it was Some, which it always is for
a valid tmp path, so `other` (the only field with the right extension)
was never consulted and reload_stem_for_event always failed its
extension check.

Extract the file/other selection into reload_event_path(), prefer
`other` (rename destination) over `file` (fallback for a direct
CREATE/CHANGED with no rename), and use it in arm_dir_reload_watch.
Mirrors watch_theme_file two functions above, which already checks both
fields correctly.

Adds a pure unit test on reload_event_path. A full FileMonitor +
real-rename integration test was tried and dropped: it pumps the shared
default glib MainContext, which collides with shell::hotreload's own
context-pumping tests under the default parallel test runner (aborts the
whole test binary via glib's thread guard) - exactly the
fights-the-event-loop case a pure-function seam exists to avoid.

Fixes ECO-14.

Breadway referenced this pull request

2026-09-05 02:20:52 +08:00

bread-theme::gtk per-output theme/palette hot-reload never fires (uses rename source path, not destination) #14

Breadway closed this pull request

2026-09-05 09:22:38 +08:00