Repository navigation
cmux-next: place a background picture by hand (drag, zoom, reset) - #21743
teamleaderleo wants to merge 30 commits into
Conversation
…t chat Every wide painting framed the same fixed point (0.5, 0.45), so with the sidebar and a docked agent chat open the subject often sat under them, and wide or short windows cut faces off. Each work now names its subject box in art/manifest.json, and the art zooms, slides or shrinks (over a blurred fill) so that box lies in the part of the window the columns leave visible. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…out, not the App CmuxNextApp may not grow (check-app-size). The sidebar and each workspace's layout root now tell the window's backdrop where they cover the art (WindowMaterialView.setCover), and the App is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Settings > Background gets Add from a Link (https only, judged by its bytes, 50 MB image / 500 MB video caps), a remove button on each of your images (not the one in use), what they take on disk, and Show in Finder. A picked, dropped or linked file must decode or play before it is copied in. Dropping a file on Settings no longer needs experimental appearance (the page already said to). The ops live in BackdropImagesPageProvider (CmuxNextPages); CmuxNextApp shrinks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r edge correctly Review fixes for the subject crop: - keepsSubject is measured on the final placement, never assumed. - A shrunk or slid image keeps the subject in the span first (the cover clamp overrode it), and the shrink fit counts the subject's height too. - No vertical fallback that cannot fail; portrait subjects are the head and upper body (top 70%%), which fit a 21:9 window. - The catalog test asserts the subject frame lies in the visible span. - Only a column that comes or goes springs the art; resizes and live window resizes move it at once. Fill filters are made per view. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eir name in their folder Review fixes for Add from a Link: - The link's name is sanitized (no folders, colons or leading dots), and the renamed file must stay in the download's own folder (an encoded ../ climbed out). - A delegate-driven data task replaces download(from:delegate:), whose task delegate never saw progress: the declared length and the running count are checked against the cap for the declared type, so a huge body never lands. - Redirects off https are refused; timeouts are a minute idle, ten in all. - The temporary folder is removed on every failure. - Remove refuses the background in use natively, not just on the page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…leo/next-place-background
Settings > Background > Adjust Placement… fades the window's content and puts an overlay over it: drag moves the picture, scroll or pinch zooms (1x-4x), Reset goes back to the automatic subject framing, Done or Escape ends. The placement (the point at the middle of the visible span, and the zoom) is kept per background in background-placement.json, and every window follows it. It starts from the framing on screen, so the first drag does not jump. CmuxNextApp is unchanged: the op rides BackdropImagesPageProvider. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Probably yours: CI failed on
Failures:
Matched log linesNot re-run automatically: Written by red: cmux-next CmuxNextApp size cap; no finished base run on 16f36fe to compare; do not merge until green; see lane-rules
Agents: |
|
Bead |
…re they start Re-review fixes: LinkDownload keeps its state in a Mutex and is plainly Sendable (no new unchecked conformance for the crash ratchet). A cancel that comes before the task exists marks the download, and the task is cancelled as soon as it is made, instead of invalidating the session first (making a task on an invalidated session raises). A body that starts like a picture gets the picture cap whatever type the server declared. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eo/next-place-background
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Found 34 test failures on Blacksmith runners: Failures
...and 24 more test failures. View all on Blacksmith: run 39373729814, run 39373729802
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light.svg)
|
Not verified: macOS jobs did not run: compile and app tests were skipped on CI passes on Written by |
|
|
…ace window, saved in order Review fixes for Adjust Placement: - Compile: the store notification's object is compared by identity (the batch Swift run stopped on 'sending store risks causing data races'). - Saves run on one serial queue, in order; an unreadable file is kept beside. - The overlay opens in the workspace window (WindowManager.active), never the Settings window; one at a time app-wide; it ends when its window shows another background, and fades views added while it is open. - Keyboard: arrows nudge (Shift: further), + and - zoom, 0 resets. - A drag past an edge has no dead travel; windows only re-place for their own background's change; removing a picture drops its placement; the page's 'no picture' note clears when the background changes. - init(coder:) returns nil instead of fatalError (crash ratchet). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
…ace-background # Conflicts: # Packages/macOS/CmuxNext/Sources/CmuxNextDesign/BackdropArtPlacement.swift # Packages/macOS/CmuxNext/Sources/CmuxNextDesign/WindowMaterialView.swift # Packages/macOS/CmuxNext/Tests/CmuxNextDesignTests/BackdropArtPlacementTests.swift
…dow comes forward Re-review fix: the center's clamp had its bounds swapped, so for art that covers the window every drag or arrow pinned it to an edge. The bounds are now [middle / size, (middle - view + size) / size], and the test checks the exact travel of a drag and the stop at the edge. Placing brings the target window forward again, so from a separate Settings window keys reach it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
The fleet compile found JSONValue (CmuxNextSettings) and revealInBackground (CmuxNextCompat) used without their imports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eo/next-place-background
A public initializer's default argument may only use publicly imported API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eo/next-place-background
…utes in Double Fleet compile fixes: BackdropArt has no id (its selection does), and the travel assertions mixed CGFloat and Double. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gallery-coverage failed: BackdropLink had no entry. The Settings entry now covers it, with a variant that opens the link form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eo/next-place-background
The batch Swift run crashed (SIGTRAP in dispatch_assert_queue): the import extension took the module's main-actor default, so usage()'s reduce closure asserted the main queue when the page called it from a detached task. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eo/next-place-background
The batch Swift run failed aLinkNameCannotClimbOutOfTheDownloadFolder: deletingPathExtension keeps "...png" whole on macOS 26, so the stem was "png". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The batch Swift run failed aDragMovesTheArtAndIsKeptForThatBackground. AppKit sends no KVO change for subviews, so a view added while placing kept full opacity; each window update now fades new ones. The test drags from the middle (the automatic framing can sit at an edge) and expects a drag down to move the center up the picture, as the code does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eo/next-place-background
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
 [Medium impact] The change affects background framing in the Settings and workspace-window workflow. Do not merge yet: the webview build remains broken, and Chromium panes can block placement. Findings
SummaryAdds manual placement for still background pictures, with drag, zoom, reset, and per-picture saves.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
Settings["Settings: Adjust Placement"] --> Overlay["BackdropAdjustOverlay in parent window"]
Overlay --> Store["BackdropPlacementStore"]
Store --> Notification["didChange"]
Notification --> Windows["WindowMaterialView updates"]
Browser["Chromium child window"] --> Covers["Covers parent views and takes input"]
Covers --> Overlay
Reviews (4) · Last reviewed commit: "Merge branch 'feat-cmux-next' into leo/n..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async addBackdropLink(url: string): Promise<string | null> { | ||
| const reply = await this.request("cmux.settings.backdrop.image.link", { url }); | ||
| if (!reply.ok) return reply.error.code || "cmux.settings.backdrop.download_failed"; | ||
| await Promise.all([this.refresh(), this.refreshHost()]); | ||
| return null; |
There was a problem hiding this comment.
SettingsStore now declares addBackdropLink, removeBackdropImage, revealBackdropImage, and backdropUsage twice. TypeScript rejects these duplicate implementations, so the webview type check and build fail.
Remove the duplicate methods and keep only the new adjustBackdrop method. Preserve the original addBackdropLink, which also records the change for undo.
| } | ||
| // Brought forward: from a separate Settings window, the keys and pointer come here. | ||
| window.makeKeyAndOrderFront(nil) | ||
| window.makeFirstResponder(overlay) |
There was a problem hiding this comment.
begin(in:) gives the overlay keyboard focus without telling the window's focus model that an overlay opened. In a real ShellWindow, this reports the new responder. FocusResponderClassifier calls this view windowOrNone, which tells the focus model to put focus back on the current page or terminal immediately.
The overlay therefore loses the keyboard as it opens. Register placement with the existing overlay focus flow, and close that session in done().
| let scale = f.width / (c.width * imageSize.width) | ||
| let cover = max(viewSize.width / imageSize.width, viewSize.height / imageSize.height) | ||
| return BackdropUserPlacement(centerX: u, centerY: v, zoom: scale / cover) |
There was a problem hiding this comment.
userPlacement(from:) cannot preserve automatic framing that slides beyond the window edge or shrinks below the cover fit. Those are supported results, including the bundled Böcklin case already covered by a test. The returned value clamps zoom to at least 1, and the manual renderer clamps the image back inside the cover bounds.
The first tiny drag therefore changes the framing abruptly instead of starting from what is on screen. Preserve these automatic starting states, or make the transition explicit before accepting a gesture.
| public func zoom(by factor: CGFloat) { | ||
| guard factor.isFinite, factor > 0 else { return } | ||
| save(BackdropUserPlacement(centerX: placement.centerX, centerY: placement.centerY, zoom: placement.zoom * factor)) |
There was a problem hiding this comment.
zoom(by:) keeps the old center even when zooming out makes that center impossible to show. The renderer slides the image back, but move(dx:dy:) still starts from the old center.
For a matching image and window shape, move to centerX = 0.125 at 4×, zoom out to 2×, then drag left slightly. The visible center is already 0.25, so the first drag only corrects the stored center and does not move the picture. Start each drag from the actual rendered placement, including after zoom or resize.
| /// Waits for every save queued so far (tests). | ||
| public func flush() { | ||
| Self.writes.sync {} |
There was a problem hiding this comment.
flush() adds a test-only helper to production Sources/. Its comment names tests as its purpose, and its only callers are in BackdropUserPlacementTests. The repository rule prohibits new test-only helpers in shipping source.
Move this helper into the test target and expose only the internal state needed through @testable import.
Rule Used: Flag Swift files under a production Sources path (matching /Sources/ and not under /Tests/) that add a test-only or debug-only seam: a #if DEBUG (or other test-build-guarded) extension/member exposing internal state only for tests or a debugg... (source)
| Self.writes.async { | ||
| try? FileManager.default.createDirectory(at: file.deletingLastPathComponent(), withIntermediateDirectories: true) | ||
| try? data.write(to: file, options: .atomic) | ||
| } |
There was a problem hiding this comment.
BackdropPlacementStore introduces a custom background DispatchQueue for ordinary file saves. The repository's concurrency rule explicitly requires Swift concurrency for this work.
Put the ordered writes on an owned actor and expose an async completion point. Preserve the guarantee that older saves cannot overwrite newer ones.
Rule Used: Flag new legacy async patterns in cmux-owned Swift where Swift concurrency is the correct shape: DispatchQueue.global for ordinary async work, new Combine app state, completion-handler APIs fully under cmux control, or fire-and-forget Tasks with mean... (source)
| overlay.updateObserver = NotificationCenter.default.addObserver(forName: NSWindow.didUpdateNotification, object: window, | ||
| queue: .main) { [weak overlay] _ in | ||
| MainActor.assumeIsolated { overlay?.fadeAdded() } // main-proof: observer on queue: .main | ||
| } |
There was a problem hiding this comment.
begin(in:) registers a block observer that only done() removes. Closing the window during placement does not call done(), and the overlay has no other cleanup path. The notification center keeps the unused registration after the overlay goes away.
End placement when the window closes, or remove the observer when the overlay is torn down.
| try? FileManager.default.createDirectory(at: file.deletingLastPathComponent(), withIntermediateDirectories: true) | ||
| try? data.write(to: file, options: .atomic) |
There was a problem hiding this comment.
BackdropPlacementStore.set silently ignores folder and file write errors. If the disk is full or the folder is not writable, the picture moves and Done appears to keep it, but the placement disappears after relaunch. Report failed saves through the app’s logging or error path instead of dropping both errors with try?.
| public static let shared = BackdropPlacementStore(file: BackdropArtDownloads.defaultFolder.map { | ||
| $0.deletingLastPathComponent().appendingPathComponent("background-placement.json") | ||
| }) |
There was a problem hiding this comment.
BackdropPlacementStore.shared adds a new singleton for mutable placement data. The repository rule requires this state to have an owner and be injected, rather than reached globally. A view can use an injected store while removing a picture still changes the global store.
Construct one store in AppServices and pass it to WindowMaterialView and BackdropImagesPageProvider, including the remove action.
Rule Used: Flag new ambient global state in production Swift: a top-level (file-scope) func used as API, a top-level mutable var or a stub class/struct holding a global flag/once-token, a caseless enum/empty struct used purely as a static func/static let namesp... (source)
| {!own && | ||
| current !== "none" && | ||
| current !== "desktop" && | ||
| !backdrops.find((choice) => choice.id === current)?.video && ( |
There was a problem hiding this comment.
The !own check hides Adjust Placement whenever This Workspace is selected. If the global background is None and a workspace has its own still picture, neither scope offers the button: All Workspaces checks None, and This Workspace always hides it.
Offer adjustment for the workspace's still picture too. Saving per picture does not require separate per-workspace placements.
| for view in content.subviews where view !== backdrop { overlay.fade(view) } | ||
| content.addSubview(overlay, positioned: .above, relativeTo: nil) |
There was a problem hiding this comment.
If Settings shares a window with a visible Chromium browser pane, that page sits above BackdropAdjustOverlay and takes clicks, drags, and scrolls in its area. The overlay is added to the parent window's view tree, below Chromium child windows. Fading the parent's subviews does not bring it above the page, which can also cover the placement controls.
Present placement through WindowOverlayHost, and hide or mask the browser content while placing.
|
|
|
|
|
|
|
|
|
Closing: superseded by #22562. The picture now aspect-fills the window and resizes with it, like a macOS wallpaper, so it needs no hand placement. |
|
Placing a background picture by hand is not landing: The rest of the batch (collages, every Apple aerial, video playback and Chief picture motion settings) lands in #22033. I'm leaving this PR open for its owner. Agent session: cosmic-gray |
Phase 6 of the backgrounds plan (briefs/next-backgrounds-plan.md): place a background picture by hand.
Stacked on #21709 and #21731. It needs the placement math from the first and the Settings page provider from the second, so this branch is #21731's head with #21709 merged in. Review only the last commit, 93f4995. Once both merge, this PR shows only that commit.
What changes
BackdropAdjustOverlayover the frontmost window that shows a picture, and fades the window's content to 12% so the art shows.cmux.settings.backdrop.not_adjustable).BackdropUserPlacement) is the image point at the middle of the visible span, plus a zoom past the cover fit.userPlacement(from:)), so the first drag doesn't jump.background-placement.json, next to the Backgrounds folder (BackdropPlacementStore), and written off the main actor. Every window showing that picture follows it live, throughdidChange.cmux.settings.backdrop.image.adjustonBackdropImagesPageProvider, and the overlay lives in CmuxNextDesign. There's no catalog entry (WINDOW); a palette and right-click "Adjust Background…" comes with phase 5.Strings
All 21 locales:
backdrop.adjust.hint,backdrop.adjust.reset,backdrop.adjust.done(Design)settingsWindow.backdropPicker.adjust,adjustHelp,notAdjustable(Settings page)Tests
CmuxNextDesignTests/BackdropUserPlacementTests:CmuxNextAppTests/BackdropAdjustOverlayTests:CmuxNextPagesTests/BackdropImagesPageProviderTests: adjust refuses or places.hostCards.test.tsx: the button runs the op, and isn't shown for None. All 129 settings tests pass.Verification
vp fmtandvp lintare clean, andtschas no settings errors.lint-ios-package-conventions.shis clean. CmuxNextApp source lines are unchanged.full-cior next-batch run, which I've asked the coordinator about.Not in this PR
WINDOW
None.
Changelog
Added: Place a background picture by hand: drag to move it, scroll or pinch to zoom, and reset to the automatic framing.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Settings > Background now lets you place a still background picture by hand and add your own images from a link. Adjust Placement… fades the window's content (including views added while placing) and overlays a drag-to-move, scroll-or-pinch-to-zoom (1×–4×), reset-to-automatic-framing session; Done, Return, or Escape ends it and restores the content. Arrow keys nudge (Shift nudges further), +/− zoom, and 0 resets.
background-placement.json, followed live by every window showing the picture; it starts from the on-screen framing so the first drag doesn't jump, and removing a picture drops its placement.CmuxNextAppis unchanged: the ops rideBackdropImagesPageProviderand the overlay lives inCmuxNextDesign.Written for commit cacfd74. Summary will update on new commits.