feat(ui): backgrounds from your own pictures - #223
aryamthecodebreaker wants to merge 7 commits into
Conversation
|
Having the image rotation will add too much load, it will effect the performance of the app. As multiple images added to the rotation will be cached. |
|
Please split this into two PRs. They are unrelated changes, and the detector fix should not have to wait on the background feature. The CLI version detection fix is a real bug and a small, self-contained change. Open it on its own against main and I will review and take it. The custom backgrounds part needs more work before it can be considered. Image rotation is still a load concern with multiple images cached, and the clarity slider re-adds an intensity control that was removed on purpose. |
Extends the wallpaper feature with a 'custom' selection beside the bundled scenes: - A native multi-select picker adds JPG/PNG/WebP/GIF/AVIF files from the user's computer to a library shown in Appearance, capped at 40. - A toggle cycles that library on a user-set interval in minutes. It lives in a headless watcher mounted in AppProviders so rotation keeps running while the user is in a session, not only on the settings screen. - A Background clarity slider sets how plainly the scene reads through the app's glass. 0 is exactly the current look; the top end leaves a trace of tint and no blur. The engine reads the file, never the renderer: backgrounds sit outside every project root, so the path jail does not apply and the user's own saved list is the allowlist instead. wallpaper.read refuses a path that is not in it, re-checks the extension, and caps the file at 24MB because the image is inlined as a data URL. wallpaper.css keeps its present values as fallbacks for the four clarity custom properties, so a build that never sets clarity paints as it does now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clarity slider thins the plate that keeps text readable, so past the middle of the range text starts competing with the picture behind it. A halo fades in behind the text as that happens, drawn from the theme's own background color: dark behind light text in dark themes, light behind dark text in light ones, so both schemes gain contrast instead of one losing it. Fully transparent at clarity 0, so the default frosted look carries no shadow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things went wrong once the plate thinned all the way out over a bright picture: - The composer and popovers followed the window plate down to a few percent of tint and dissolved into the scene. They are targets, not backdrop, so their tint now stays high while the pane behind them goes clear; with the blur they already carry from glass.css they read as a dark glossy plate resting on the picture. - One soft text shadow disappeared against a busy, bright photo. The halo is three stacked layers now: a tight opaque pass that holds the glyph edges, and a wider one that pulls the picture down around the text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7089252 to
a32b329
Compare
|
Split as asked: the CLI version detection fix is now #228 against main, and this PR is background work only. I have moved it to draft while the caching problem is fixed. You were right about the load, and it is worse than I assumed: Devin also found a write race worth mentioning on its own: adding the first image starts a library write and a wallpaper write at the same time, and I have left a note in the description about the clarity slider rather than arguing it here. Short version: the removed control switched between discrete per-wallpaper looks, this is one continuous value over the same single recipe, and 0 is exactly today's appearance. But a user photo behind the default 72% plate is a grey wash, so something has to give for the feature to be worth having. If you would rather have no user-facing control, I will derive it per image from brightness instead. |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Slow CLI versions disappear
When a concurrent Windows probe exceeds five seconds, probeVersion kills a healthy CLI before it returns. Update enrichment cannot compare that null version, so updateAvailable remains null.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
The halo was three stacked soft shadows. A blur wide enough to read over a busy photo spreads into the counters and between letters, so text gained contrast and lost its edge — legible, but soft. Four hard one-pixel offsets trace the glyph instead, with one small blur left to pad the contour so it does not look stencilled. Same contrast, sharp edge, and one fewer shadow layer to paint. Reduced transparency now drops the halo outright: that mode restores an almost opaque plate, so the outline compensating for a thin one is noise in the mode asking for less of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rity Readability. The clearest setting took the window plate down to 6% tint and no blur, and over bright or busy photos text was hard to read even with the outline. It now keeps about a third of the plate and a light blur: the tint pulls any photo toward the theme's tone behind the text, and the blur strips the fine detail sitting directly behind glyphs, which is what actually fights reading. The picture stays recognisable; inputs and overlays stay well above the pane so the composer is always findable. Memory. imageCache kept every data URL rotation ever visited. A data URL costs about 4/3 of its file, so a valid 40-image library at the 24 MiB reader limit held over a gigabyte for the whole session. The cache now keeps only the picture on screen and, while rotating, the next one (decoded ahead so the change has no blank frame) — two at most, however large the library. Failed reads were cached as null forever, so one read that failed while a drive slept left that picture blank until restart; they are dropped now. Adding or removing pictures started the library write and the selection write together. Both go through SettingsStore.update, which builds from its in-memory copy and writes through one temp file, so they could overwrite each other or fail a rename. The selection now follows the library write. Leaving Appearance inside the clarity debounce cancelled the only pending write: the preview kept the value and a restart reverted it. The page now flushes it on the way out. The picker saved files over the reader's size limit, which then sat in the library as blank backgrounds; it now drops them at pick time. Also removes an indexRef that was assigned but never read, and mojibake in three comments left by an earlier encoding slip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The caching concern is fixed, along with the rest of the review, and readability is up. Load. The background cache now holds at most two pictures — the one on screen and the next — however large the library is. Before, it kept every picture rotation had visited, which for a valid 40-image library meant over a gigabyte held all session. There is a test pinning the count at two for 40 images. Readability. The clearest setting no longer takes the plate to a trace of tint. It keeps about a third of the plate plus a light blur behind the text, and text has a crisp outline rather than a soft glow. Photos stay recognisable, and text stays readable over bright ones. Also in: the library/selection write race, oversized picks that could never load, clarity lost when leaving the page mid-drag, and failed reads cached forever. One correction worth flagging: the review bot marked the cache and failed-read findings resolved on the previous revision, before either was fixed. Please judge them against the new commit rather than that status. Taking this out of draft. The clarity slider question is still yours to call — details in the description, including the no-slider alternative. |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| // SettingsStore.update, which builds from its in-memory copy and writes | ||
| // through a single temp file, so starting them together could let one | ||
| // overwrite the other or fail the other's rename. | ||
| void update({ appearance: { customWallpapers: next } }).then( |
There was a problem hiding this comment.
🟡 Rapid image removals lose changes
Rapid library edits launch overlapping update calls from the same stale images snapshot. SettingsStore shares one temporary file across asynchronous saves. One removal can fail or restore an image the user removed.
Learn more
Each remove button computes a complete replacement library from the images array rendered before either request finishes. Both replacements then enter SettingsStore.update, which has no queue and writes through one shared .tmp path. Concurrent saves can overwrite that temporary file, make one rename fail, or leave disk state and #current reflecting different requests. The rejected request is only logged, while the UI can continue showing the image the user removed.
Example: With [a.jpg, b.jpg, c.jpg], the user quickly removes a.jpg and b.jpg. The requests carry [b.jpg, c.jpg] and [a.jpg, c.jpg]; one can fail at rename or replace the other, leaving one deleted image in the library.
Recommended fix: Serialize settings mutations in SettingsStore, or disable/coalesce library controls while a replacement is pending. If coalescing in the renderer, derive each queued replacement from the latest confirmed or optimistic library rather than the render-time images snapshot.
Was this helpful? React with 👍 or 👎 to provide feedback.
| void pending.then((dataUrl) => { | ||
| if (dataUrl === null && imageCache.get(path) === pending) imageCache.delete(path) |
There was a problem hiding this comment.
🟡 Failed static backgrounds never retry
When readImage returns null for a non-rotating image, cache eviction does not rerun the painting effect. The background stays blank until selection changes or restart, even after the file becomes readable.
Learn more
The cache correctly forgets a null result, but the only caller for a static image lives in an effect keyed by wallpaper, index, library, and rotation. Deleting the cache entry changes none of those dependencies. Rotation eventually retries the path, but a one-image library has no interval, so a sleeping drive or mid-copy file remains blank after it recovers.
Example: Ari starts with one background on an unavailable external drive. wallpaper.read returns null, then the drive wakes; Ari keeps the theme background until the user switches wallpapers or restarts.
Recommended fix: After a null result, schedule a bounded retry that updates hook state while the same path remains selected. Cancel the retry when the wallpaper, library, or component changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
The outline was applied to every text node on the plate. It exists for text sitting directly on the thinned plate, but it also landed on white labels over accent and danger buttons and badges, where a theme-colored contour reads as dirt rather than contrast, and on the composer, inputs, popovers, surface cards and code, which already carry their own contrast. text-shadow inherits, so clearing it on those surfaces clears it for everything inside them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| .ari-composer-shell, | ||
| .ari-glass-input, | ||
| .ari-glass-overlay, | ||
| [class*='bg-surface-'], |
There was a problem hiding this comment.
🟡 Hover variants suppress idle text outlines
Elements with hover:bg-surface-* match [class*='bg-surface-'] while idle and lose their outline. Transparent buttons and rows then lose contrast against busy wallpapers.
Learn more
CSS attribute substring matching examines the entire literal class attribute. Tailwind variant prefixes remain in that attribute regardless of interaction state, so hover:bg-surface-2 and focus-visible:bg-surface-2 satisfy this selector continuously. Many transparent controls use these variants, including the titlebar controls. The selector therefore removes the halo from text that still sits directly on the thinned plate.
Example: A titlebar control carries hover:bg-surface-2/60 but has no idle background. Before the pointer enters it, the substring selector already applies text-shadow: none; the control text remains directly over the wallpaper without the intended outline.
Recommended fix: Match only active base background utilities rather than arbitrary class substrings. Enumerate the supported bg-surface-* tokens and opacity forms, or mark genuinely filled surfaces with a dedicated semantic class or data attribute.
Was this helpful? React with 👍 or 👎 to provide feedback.
The comment named background-clarity.ts, which never existed; the clarity mapping and its legibility floor live in custom-background.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Split as asked: the CLI version detection fix is #228 against main, and this PR is the background work only.
What this adds
wallpapergains a'custom'selection beside the bundled scenes:The engine reads the files, never the renderer. Backgrounds live outside every project root, so the path jail does not apply; the saved library is the allowlist instead, and
wallpaper.readrefuses any path not already inappearance.customWallpapers.The load concern — fixed
You were right, and it was worse than I first assumed:
imageCachekept every data URL rotation ever visited, so a valid 40-image library at the 24 MiB reader limit held over a gigabyte for the session.The cache now holds at most two pictures — the one on screen and, while rotating, the next one, decoded ahead so the change has no blank frame.
retainedPathsdecides what is kept and is tested directly, including that the count stays at two for a 40-image library. Memory is now flat in library size rather than linear.The rest of the review findings are in the same commit:
SettingsStore.update, which builds from#currentand writes through one shared.tmp. The selection now follows the library write, with a test that holds the first write open and checks the selection has not moved.One note on the review bot: it marked the cache and failed-read findings resolved on the previous revision, before either was actually fixed. They are fixed now in
9654d8d; please check that commit against the findings rather than the bot's status.Readability
The clearest setting originally took the plate down to a trace of tint with no blur. That looked striking and failed at the one job the window has: over a bright or busy photo, text was hard to read even with an outline.
It now keeps about a third of the plate and a light blur at the top of the range. The tint pulls any photo toward the theme's own tone behind the text, and the blur strips the fine detail directly behind the glyphs, which is what actually fights reading, while the picture stays recognisable. Text carries a crisp one-pixel outline in the theme's background color rather than a soft glow, which spread into the letter counters and made text read soft. Inputs, popovers and menus stay well above the pane, so the composer is always findable.
On the clarity slider
Understood, and the test named "offers no visibility control — one uniform glass look per wallpaper" is still in the tree and still passing. Two reasons I think this is a different control, and then it is your call.
The removed control switched between discrete per-wallpaper looks, each a different recipe. This is one continuous value over the single plate recipe, so there is still exactly one look, and 0 is byte-for-byte today's appearance — the fallbacks in
wallpaper.cssmean a build that never sets it paints identically.The practical reason: bundled scenes are chosen to sit behind a 72% plate, and user photos are not. A bright photo behind the default plate is a grey wash, which makes the feature pointless for the pictures people actually pick. And the range now has a legibility floor, so the slider cannot take text below readable.
If you would rather have no user-facing control, the alternative is deriving clarity per image from its brightness and dropping the slider. Say which you prefer and I will build that.
How verified
Windows 11, Node 24.19, pnpm 10.33; exercised in
pnpm devwith a three-picture library rotating.Full
pnpm verifywas run on the earlier revision (engine failing only its knowngit-servicetimeout under load, passing alone). For this commit I ran the suites covering what changed rather than the whole tree again. The size filter indialog.pickImageshas no unit test: that handler calls Electron's native dialog, and there is no harness for it inrpc.ts.🤖 Generated with Claude Code