feat(wallpaper): source selector, per-source grid scoping, and rotation controls - #25
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
mirkobrombin
left a comment
There was a problem hiding this comment.
Please replace the NCZ-specific state files, paths, and daemon contract with Singularity-owned settings and interfaces before this can be merged.
|
Thanks for flagging this, Mirko — you were right, this PR had leaked three NCZ-specific things into otherwise generic code. Fixed in 53cff13:
I also renamed the "ncz" placeholder id/paths used in test fixtures to generic sample names (
Verified with a full Re-requesting your review. |
mirkobrombin
left a comment
There was a problem hiding this comment.
The Singularity-owned rotation files now have no runtime consumer, so please wire them to a real rotator or IPC service before exposing these controls.
|
Fair point, and you were right — the files had a writer and no reader, so those two controls were cosmetic. Fixed in c4180da: Where it runs, and why not a daemon or IPC. It runs in the shell process. What it does:
The directory scan runs on a worker thread, the same way I also pulled the registry roots and the state directory into single accessors ( Tests.
|
|
Follow-up in 8ddf523, before you spend time on this — adversarial review of my own commit above caught something I should have seen first time.
Absent now means off. Rotation is opt-in; the switch and the interval row read the same accessor, so an untouched install shows rotation off and does nothing. Two tests lock that in and both fail if the old default is restored. Same commit also corrects two comments about registry-root ordering:
|
mirkobrombin
left a comment
There was a problem hiding this comment.
The runtime consumer is now wired and the 12 tests pass, but this revision still adds long explanatory comment blocks and a Unicode dash in the source label, so please keep only the non-obvious comments and use ASCII punctuation before merge.
|
Addressed the current review in 153cc08:
@mirkobrombin please take another look. |
…h the rotator daemon (cherry picked from commit 50db4b3)
…rols Implements Tasks 3-4 of docs/superpowers/plans/2026-09-04-wallpaper-source-selector-rotation.md (Tasks 1-2 already landed as 36bcc12/50db4b3). - Wire WallpaperCollections.parse() and WallpaperRotationState into desktop_page.vala: a Wallpaper Source selector row lists every installed .collection, and populate_grid() now scans only the selected collections directory instead of every backgrounds path plus every collection at once. - Remove the now-superseded private collection_dirs() helper, fully replaced by WallpaperCollections.parse(). - Add Rotate Wallpapers (on/off) and Rotation Interval rows sharing the same ~/.config/ncz-wallpaper/{collection,rotate-enabled,rotate-interval} state files the shipped ncz-wallpaper-rotate/-daemon scripts already poll -- no daemon change, no new IPC. - meson.build: register wallpaper_collections.vala and wallpaper_rotation_state.vala in the main app source list. Tasks 1-2 only wired them into their own standalone test executables; desktop_page.vala could not resolve the types without this. Verified via a real container build (SINGULARITY_SOURCE_DIR override, build-singularity.sh): singularity-desktop links clean, and the shipped binary carries both the new symbols (singularity_wallpaper_collections_parse, singularity_wallpaper_rotation_state_get_selected_collection, etc.) and the literal UI strings ("Wallpaper Source", "Rotate Wallpapers", "Rotation Interval", "Every 10 minutes") -- checked directly in the binary, not inferred from build success. Not yet done: manual on-hardware verification of the actual UI (open Desktop settings, switch sources, toggle rotation) -- this commit verifies the code builds and ships, not that the UX behaves correctly end to end. (cherry picked from commit 8bd1f5e)
Admit recent images only when the selected collection scan contains them. Exclude separately registered nested packs while retaining provider subdirectories. Exercise the production scanner with mixed history and a live-state probe. (cherry picked from commit bd9dd7a)
Not something an upstream maintainer should receive as part of the feature -- it was a local live-state debugging aid, not a test or a build artifact anything else depends on.
Five real correctness issues found by an adversarial review of the prior four commits before opening this as an upstream PR: - WallpaperRotationState.write(): write-then-rename instead of a direct set_contents, so the rotator daemon (which polls these files on its own timer, independent of this UI) can never observe a partial write. - get_rotate_enabled(): accept "false"/"off" as disabled, not only the literal "0" the UI itself writes -- a daemon written to a slightly different convention would otherwise read as always-on. - WallpaperCollections.parse(): trim KeyFile string values (Dir/Id/Name/ Artist/Type). GLib.KeyFile permits "Key = value" with surrounding whitespace; an untrimmed Dir would silently fail every path comparison downstream, and an untrimmed Id would defeat duplicate-id detection. - WallpaperGallery.scan(): canonicalize paths (Posix.realpath) before comparing collection roots for the source-boundary exclusion, so a pack installed through a symlinked directory is still recognized as the same root instead of leaking into another source's scan. - desktop_page.vala populate_grid(): when the persisted selection doesn't match any installed collection, persist the fallback (rather than re-deriving and re-logging the same mismatch on every refresh). Verified via a real container build (SINGULARITY_SOURCE_DIR override, build-singularity.sh) on ULTRA.
Replaces the NCZ-specific paths and default id that leaked into the wallpaper source selector / rotation feature with Singularity-owned equivalents, addressing Mirko's review on this PR: - Rotation state directory moves from ~/.config/ncz-wallpaper to ~/.config/singularity/wallpaper-rotation, matching the singularity/<feature> convention already used by search_manager.vala (singularity/search-providers) and display_manager.vala (singularity/displays.json). - The pack registry search roots move from <datadir>/ncz-wallpapers/ collections to <datadir>/singularity/wallpaper-collections, so any downstream OS or pack installer -- not just NCZ -- can drop a .collection file there and have it appear in the picker. - The hardcoded "ncz" default collection id is replaced with "", which the existing "no matching collection" fallback (already present in both call sites) turns into "use whatever collection is actually installed" instead of assuming one named ncz exists. - WallpaperRotationState's doc comment no longer names cix-installer's ncz-wallpaper-rotate/ncz-wallpaper-daemon scripts; it now documents the three state files (collection, rotate-enabled, rotate-interval) as a plain, project-owned contract that any rotation daemon can implement, since that's what this class actually is regardless of which daemon reads it. - Test fixtures that used "ncz" purely as a sample id/dir name are renamed to generic placeholders (vendor, system, default) for consistency; WallpaperCollections and WallpaperGallery were already fully generic and needed no logic changes. No behavior changes for existing installs beyond the config paths moving -- the fallback-to-first-available-collection logic already present in populate_grid() and the initial-selection code is what now does the full job the "ncz" default used to do. Verified: full singularity-desktop build (meson setup + ninja) against libsingularity built from the same tree, and `meson test` -- all 8 suites pass, including the 19 wallpaper-collections/-gallery/ -rotation-state tests.
The rotation state files under $XDG_CONFIG_HOME/singularity/
wallpaper-rotation had no reader, so "Rotate Wallpapers" and "Rotation
Interval" persisted a preference nothing acted on.
WallpaperRotator is that reader, in the shell process rather than a
separate daemon or a new IPC surface: the shell already owns the
wallpaper through WallpaperManager and already repaints on a
background-picture-uri change, so a rotation is a timer plus the same
settings write the gallery makes, and the crossfade, rescaling,
preview and accent extraction come along unchanged. A separate binary
would need its own copy of the collection parsing, its own schema
lookup against the shell's prefix, and a session unit that reliably
starts.
- rotate-enabled decides whether a timer exists at all
- rotate-interval is read when the timer is armed, and a FileMonitor
on the state directory re-arms on any write, so a change applies
immediately rather than after the current period
- collection scopes the pick to that registry entry only
- the pick is random but never the wallpaper already showing
- an empty pack, a provider that fetched nothing or a stale
collection id leaves the current wallpaper alone
- suppressed in safe mode with the other optional startup work
The scan runs on a worker thread, as populate_grid() already does for
the identical walk -- it is filesystem I/O and this main loop is the
compositor's.
The registry roots and the state directory each had their path spelled
out inline in the settings page; both now come from one accessor, so
the writer and the reader cannot drift apart.
Tests (13 cases) cover the pick (deterministic under an injected roll),
collection scoping, the stale-id and empty-pack fallbacks, the threaded
rotation hand-off, and that the switch and interval govern the armed
timer. Checked by mutation: making pick() ignore the current wallpaper,
reschedule() ignore rotate-enabled, and rotate_async() never announce,
each fails its own test. meson test: 12/12 suites pass on aarch64.
Assisted-by: Claude Code:claude-opus-5
AI-Scope: generated wallpaper_rotator.vala, its test, and the
start_rotation() wiring, from a brief to build a real consumer for the
existing rotation-state files using the shell's current display path.
get_rotate_enabled() treated a missing rotate-enabled file as enabled. That was harmless while nothing read these files, but the previous commit gave them a runtime consumer -- so as written, every existing install would begin replacing the wallpaper its user had chosen, every ten minutes, from whichever collection sorts first in the registry, without anyone having touched the switch. The settings page only writes that file from the toggle handler, which is connected after the SwitchRow is built, so a user who never opened the page has no file at all. Absent now means off. The switch and the interval row read the same accessor, so an untouched install shows rotation off, which is also what it does. Also corrects the registry-root ordering comments. parse() is first-root-wins and default_search_roots() passes system dirs first, so a shipped collection beats a user file reusing the same Id -- the opposite of what both comments claimed. Ordering and behaviour are unchanged; only the description was wrong. Tests: rotate-enabled now defaults false, and a rotator started against an untouched config arms no timer. Both fail if the old default is restored. Assisted-by: Claude Code:claude-opus-5 AI-Scope: adversarial review of the preceding commit found the default-on regression; Claude Code wrote this fix and its tests.
8e0830f to
81269e8
Compare
mirkobrombin
left a comment
There was a problem hiding this comment.
A custom rotate-interval is displayed as 10 minutes while WallpaperRotator keeps using the stored custom value; please preserve that value in the selector or persist the normalized preset.
…earest preset
Mirko's review: a custom rotate-interval displayed as "Every 10 minutes"
while WallpaperRotator kept using the actual stored value.
The Rotation Interval selector only offers five fixed presets (10min/30min/
1h/4h/day). When the stored interval didn't match any of them, the row
silently fell back to the 10-minute id purely for display, while
WallpaperRotationState.get_rotate_interval_seconds() -- read directly by
WallpaperRotator -- kept returning the real stored value. Rotation itself
was never affected, but the settings page actively misrepresented what was
about to happen.
Fixed by preserving the value instead of collapsing it: when the current
interval has no matching preset, a synthesized entry showing the real
duration ("Every 2 hours (custom)") is inserted into the option list in
chronological order and selected, rather than substituting a different
preset's id. Nothing is written back to disk by this change -- the row
only writes when the user actually picks a different option, same as
before.
Verified: full ninja build (204/204 targets, 0 errors) and meson test
(8/8 pass) in a debian:forky podman container on ULTRA.
Assisted-by: Claude Code:claude-sonnet-5
AI-scope: root-caused the display/state mismatch from Mirko's report,
authored the custom-interval preservation and its label formatter, and
verified the build/test result quoted above.
|
Addressed in 7520aee: a stored interval outside the five fixed presets now gets its own entry in the selector showing the real value (e.g. "Every 2 hours (custom)"), inserted in chronological order, instead of silently falling back to the 10-minute preset id for display. WallpaperRotator was never actually using the wrong value -- this was purely a settings-page display bug -- and nothing is written back to disk unless the user picks a different option. Verified with a full ninja build (204/204, 0 errors) and meson test (8/8) in a debian:forky container. |
|
@mirkobrombin please take another look. |
7520aee to
068872d
Compare
Summary
Adds a Wallpaper Source selector to Desktop settings alongside the existing wallpaper grid, plus Rotate Wallpapers and Rotation Interval controls:
WallpaperCollections(new core class) parses the.collectionKeyFile registry (Id/Name/Artist/Dir/Type) already used to describe installed wallpaper packs/providers, in "first root wins" priority order across search roots.WallpaperRotationState(new core class) reads/writes the same plain-text state files (~/.config/ncz-wallpaper/{collection,rotate-enabled,rotate-interval}) the shipped rotation daemon already polls -- this is the UI's side of existing shared state, not a new mechanism or IPC.WallpaperGallery(new core class, extracted fromdesktop_page.vala's inline scan) scopes the wallpaper grid to only the currently-selected source, replacing the previous behavior of showing every backgrounds path and every collection mixed together. It also fixes two boundary bugs found in review: a "recent" wallpaper from a different source no longer leaks into the current source's grid, and a nested, separately-registered pack is no longer recursively pulled into its parent's scan.desktop_page.vala: wires the above into aSelectionRow(source),SwitchRow(rotate on/off), andSelectionRow(interval), and scopespopulate_grid()'s background scan thread to the selected source's directory.Validation evidence
Built via a real container build (
build-singularity.sh,SINGULARITY_SOURCE_DIRoverride) on an aarch64 build host -- not justmeson compilein isolation. Confirmed in the stagedsingularity-desktopbinary viastrings/grep, not inferred from a clean exit:Manually verified end-to-end on real Sky1/labwc hardware: switching sources rescopes the grid, rotate toggle and interval persist and are honored by the existing rotation daemon.
tests/wallpaper_collections_test.vala,tests/wallpaper_rotation_state_test.vala, andtests/wallpaper_gallery_test.valacover empty registries, malformed KeyFiles, missingDir=, duplicate IDs across search roots, and the source-boundary-and-recents edge cases.Compatibility
No change to the on-disk state file format or the rotation daemon -- purely additive on the UI side of an existing shared contract.
Rollback
Revert this branch's commits; no schema/state-file migration to undo.
AI assistance: disclosed