Chantier/studio gpui - #314
Merged
Merged
Conversation
… state contract The studio cannot be published. It depends on dioxus-primitives and dioxus-attributes through a git dependency pinned at DioxusLabs/dioxus-components@02801f27, and cargo refuses to publish any crate carrying one. Hence publish = false, and no cargo install rustmotion-studio. That dependency will not become publishable: dioxus-primitives on crates.io is 0.0.0, a name reservation with no code, untouched since April 2025. The whole gpui-kit tree, by contrast, resolves to crates.io alone — 854 packages, zero git sources — and compiles here in 49 s. This is the scaffolding commit of the rewrite, not the rewrite: it swaps the manifest and writes the one contract six parallel workstreams would otherwise each invent. Under Dioxus the editor state was ten scattered Signal<T>; gpui owns state in an Entity<T> and notifies explicitly, so the shape has to be decided once, up front. Selection replaces a (u32, String, String) tuple whose two Strings were interchangeable at the type level — nothing stopped a pointer being passed where a kind was meant. The crate does not build between this commit and WS-1: a framework swap has no smaller atom. What the partition does buy is everything after that. The frozen core — scenario/*, properties, prefetch, audio, frames, library/data, some 5 200 lines carrying 121 unit tests — contains exactly two references to dioxus, both single prelude imports in hook functions already assigned to a workstream. So the intelligence of the studio (optimistic editing, the self-write ledger, the debounce rebased onto fresh disk content, undo/redo, the diff, the frame cache, the audio clock, the schema-driven registry) is not being rewritten at all, and its tests hold every later workstream honest. image and smallvec become direct dependencies: RenderImage::new takes an image::Frame inside a SmallVec, and gpui-kit re-exports neither, so the types only unify if this manifest names the same major versions. gpui-kit is pinned exactly. These are 0.x crates that ship breaking changes in patch slots — 0.6.2 removed the dock tiles canvas — and 0.7.0 drops the three Root::render_*_layer calls this app needs, with no documented replacement. Refs #306
The user wants no comments in rustmotion-studio — inline, doc and module alike. This strips the 14 files of the frozen core, the part no workstream of the gpui-kit chantier owns. Each workstream strips the files it owns in its own commit; the view modules still to be rewritten will be written comment-free from the start rather than written and then stripped. Scope is the studio crate only. rustmotion, rustmotion-core, rustmotion-components and rustmotion-html are published, and emptying their doc comments would empty their docs.rs pages. 759 deletions against 3 insertions, and all three insertions are lines that merely lost a trailing comment — read_style's #[allow(dead_code)], a return in audio::prepare, an undo call in a history test. No line of code changed meaning. The stripper is string-literal aware, so the // inside "https://example.com", inside raw strings, and after an escaped quote all survived; rustfmt then proved every file still parses. What the deleted prose explained is not lost, only moved. The load-bearing rationale — why RENDER_STACK is 32 MiB (untagged serde buffers a frame per nesting level, and the default 2 MiB stack overflowed the prefetch workers), why the playhead follows the audio clock rather than a timer, why resolve_flush replays onto fresh disk content instead of a snapshot, and the CSS completeness-by-construction invariant — is written up in #306 and the workstream issues, which are prose-friendly and are where someone reconstructing a decision actually looks. From here the rule is upstream of the writing: when a comment feels necessary, rename the binding or extract a named function, so the explanation sits in an identifier that cannot drift from the code. Refs #306
The rule is only useful where every session and every sub-agent will meet it before writing a line. Left in a conversation it would be re-litigated once per session; left in a commit message it would never be found at all. Scoped deliberately to rustmotion-studio. The other four crates are published, and the same rule applied there would empty their docs.rs pages. The clap note is there because it is the one place where the rule changes behaviour rather than style: the derive turns doc comments into --help text, so deleting them silently empties rustmotion-studio --help. schemars reads them into schema descriptions the same way, which costs nothing today only because the studio derives its schema from types defined in other crates. Refs #306
) * feat(studio): boot the shell on gpui-kit with a light/dark/system theme Boot: gpui_kit::application().run(...) + gpui_kit::init(cx) inside it - there is no Application::new() in this kit. The window opens from cx.spawn(async move |cx| { cx.open_window(...) }) so the state entity and the theme can be set up with a live window and App context in hand before the first paint. Root wrapper: gpui_component::Root::render mounts the app's view but NOT the dialog/sheet/notification layers - the app has to composite those itself via Root::render_dialog_layer/_sheet_layer/ _notification_layer. Isolated all three calls in app/overlays.rs and nowhere else: gpui-kit 0.7.0 removes them with no documented replacement, so when that migration lands it is a one-file patch instead of a hunt across every screen. StudioRoot (app/root.rs) is currently a placeholder - it renders "Library" or "Editor" depending on StudioState.view, since library::view and editor::view (WS-3/WS-4) are not ported yet. What is live and testable ahead of them: the window, the theme, and the overlay layer stack. Theme: gpui-component's ThemeMode only has Light/Dark, and theme::init (run once by gpui_kit::init) hard-sets Light with nothing observing the OS. ThemePref::System is built on top in theme/mod.rs: - apply() resolves System via Theme::sync_system_appearance, which calls Theme::change internally - so it already re-projects onto the gpui-base layer (scrollbars, resize handles); no separate Theme::sync_base call is needed on this path. - watch_system_appearance() re-checks state.theme_pref on every window.observe_window_appearance callback (not a value captured at subscribe time), so it only re-resolves against the OS while the live preference is still System - an explicit Light/Dark choice made after subscribing is never clobbered by a later OS flip. - theme::set() is the pref-switching entry point the topbar's theme button (WS-4) will call once that screen exists; nothing calls it yet, hence the #[allow(dead_code)]. Persistence (theme/persist.rs): same shape as library::data::load_recents/push_recent - a JSON value under dirs::config_dir()/rustmotion/, here theme.json. Fixes the theme resetting to System on every launch. Window sizing (app/window.rs): 75% of the primary display, centered, same target as the old Dioxus shell's one-shot use_effect resize - but set on WindowOptions.window_bounds before the window opens via Bounds::centered(None, size, cx), so there is no first-frame resize flash. Falls back to a fixed 1280x800 when no display can be found, still centered, rather than the old silent no-op. Token mapping for screens styling off cx.theme() (gpui_component:: ActiveTheme) in place of the old --rm-* CSS custom properties (formerly THEME_CSS in app/root.rs): --rm-bg -> background --rm-surface -> popover (raised panel / card fill) --rm-surface-2 -> secondary (a step below popover) --rm-surface-3 -> muted (hover / selected fill) --rm-border -> border --rm-border-2 -> ring (stronger, focus-adjacent edge - no exact equivalent, this is guidance not a contract) --rm-text -> foreground --rm-text-strong -> popover_foreground / primary --rm-text-muted -> muted_foreground --rm-accent -> accent / primary --rm-on-accent -> accent_foreground / primary_foreground --rm-error -> danger (NOT destructive - that name does not exist on ThemeColor) --rm-overlay-hover -> muted (or accent at reduced opacity) --rm-overlay-border -> ring Nothing here re-brands the kit's default palette (no custom ThemeColor/ThemeConfig) - that is a separate, larger decision left to whichever workstream first needs brand-accurate colors. Also: gpui-component added as a direct Cargo.toml dependency (reported separately, not included in this diff) - ActiveTheme/Theme/ThemeMode/ Root are used by their real crate path throughout this code, which needs gpui-component in the extern prelude; gpui-kit only re-exports it as gpui_kit::component. * feat(studio): wire the crate back together on gpui-kit WS-1 restored the shell; this restores the crate around it. These are the convergence points the partition reserves to the orchestrator — module registration, the manifest, and the two frozen-core files whose only Dioxus tie was a hook. No workstream writes them, precisely so six parallel rewrites cannot each reshape the wiring under one another. Three Dioxus sites are removed rather than ported, because the thing they did no longer exists. use_prefetch_publisher and use_export_poll polled Dioxus signals; gpui notifies explicitly, so WS-2 and WS-6 replace them with state updates. ExportToast hand-rolled a floating div; the kit has a notification system, so WS-6 uses it. diff_panel.rs loses everything but DiffSide, the one piece a frozen file (app/state.rs) and a kept file (prefetch.rs, via FrameKey.side) both need. That file was a third Dioxus site the decomposition had not anticipated — it claimed exactly two, having grepped a file list that omitted it. WS-1 caught it. frame_for_change, the pointer-to-first-frame lookup deleted with the rest, is real logic rather than view code and is worth WS-6 rebuilding rather than reinventing. editor and library carry a crate-level #[allow(dead_code)] because each now mixes code with a live caller against code whose only caller is a view file excluded from its mod.rs until its workstream lands. Rustc cannot tell "orphaned pending a rewrite" from "genuinely dead", and a per-item allow would mean editing files that are frozen or owned elsewhere. Both allows come off once WS-2 through WS-6 have landed; library/mod.rs also stops re-exporting render_thumbnail and ScenarioEntry, which WS-3 puts back in one line. palette was declared default-features = false, and its default set is the only thing supplying std. Without it palette has no source for the Round/Sqrt/Abs trait impls on f32 — there is no libm fallback at 0.7.6 — so the dependency failed to build with 18 errors regardless of anything in this crate. It went unnoticed because an incremental target directory kept serving an artefact built before the flag was added. Pre-existing, unrelated to gpui-kit, and fixed here because nothing compiles until it is. Cli::file and Cli::dir move their descriptions into #[arg(help = ...)]. Under the no-comment rule their doc comments would simply have been deleted, and clap derives --help from exactly those, so rustmotion-studio --help would have gone quietly empty. The rule is about prose in the source, not about dropping behaviour. Verified on the integrated tree: cargo fmt --all --check, cargo clippy -p rustmotion-studio --all-targets -D warnings, and cargo test -p rustmotion-studio all exit 0, with 114 unit and 7 integration tests passing and 2 soak diagnostics ignored. Refs #306, #307 * ci(studio): install gpui's link-time dependencies instead of dioxus's The first CI run of the chantier failed with `unable to find library -lxkbcommon-x11`, and failed in a way worth recording: clippy was green. clippy type-checks but never links, so a missing system library is invisible to it and surfaces only in the test job, seventeen minutes later. Anyone reading "clippy pass, test fail" on this repo should suspect a linker dependency first. webkit2gtk, gtk-3 and xdo go. They were there for dioxus desktop, which no longer exists in this workspace, and nothing else in it pulls glib. In their place gpui needs xkbcommon-x11 for keyboard handling, plus wayland and xcb-cursor for the two display backends it builds on Linux. fontconfig, freetype and asound are untouched: the first two belong to Skia's text stack and the third to cpal, which rodio pulls in for preview audio. Neither had anything to do with the UI framework. publish.yaml gets the same list, and its explanatory block is rewritten rather than extended. It described the GTK/WebKit stack and glib-2.0 as the reason the packages were needed, which stopped being true with this change. A release workflow whose comment explains a dependency that is no longer there is worse than one with no comment: it sends the next reader looking for a glib failure that cannot happen. Refs #306 * feat(studio): put frames on screen through gpui's image path Closes #308. Part of #306. The JPEG encode disappears. It never existed for quality or size — it existed because the frame had to cross into a webview, and an <img> tag needs an encoded byte stream. gpui takes decoded pixels directly, so the encode, the wry asset handler and the /frame/{idx}?v={rev} URL all go, and with them a round trip through a codec on every displayed frame. What replaces it is `RenderImage::new(smallvec![Frame::new(buffer)])` wrapped in `img(ImageSource::Render(..))`. That variant bypasses every software cache in gpui — it hands the Arc straight back — and `RenderImage::new` mints a fresh monotonic ImageId per call, so a frozen preview from a stale cache entry is not merely unlikely here, it is unrepresentable. The old path needed no-cache headers on every response to get the same guarantee. Frames are written opaque. gpui's image pipeline blends with SourceAlpha / OneMinusSourceAlpha — straight alpha — while Skia produces premultiplied, and reconciling the two would mean a division pass per pixel per frame. It is unnecessary: `render_frame_v2_scaled` clears the whole framebuffer with the scenario background before anything paints, and the JPEG path it replaces already discarded alpha via to_rgb8(). Forcing 255 in the same loop that swaps R and B costs nothing and sidesteps the mismatch entirely. `swap_frame` hands the evicted frame to `cx.drop_image`. This is the one place in the rewrite where forgetting a line leaks half a gigabyte of VRAM per second: gpui's sprite atlas has no eviction, and each 1920x1080 frame owns its own ~8 MB Metal texture. The drop happens in the update path, never during paint, because update and paint are turns of the same single-threaded loop and cannot interleave — an argument from the executor model, not something a test exercises, since no live window is available without a test-support feature this crate does not enable. `render_frame_rgba_deep` mirrors the existing `render_frame_deep` rather than calling `render_frame_rgba` directly. The 32 MiB stack is not decoration: untagged serde buffers a frame per level of scene nesting, and the 2 MiB default overflowed the prefetch workers. The new path runs on the background executor pool, which has no better stack than they did. `prefetch_target_for` is split out of `publish_prefetch_target` so the target it builds can be tested as a value. Its own test previously waited two seconds for a real worker thread to render, then asserted on frame 0 — which `prefetch_window` never schedules, since a playing prefetcher only looks forward from the playhead. It also keyed on a global atomic that a sibling test mutates. It could not pass, and would have raced if it could. * feat(studio): rebuild the diff panel, annotations and export status on gpui-kit Closes #312. Part of #306. Three panels, and one of them stops being hand-rolled. The export toast was an absolutely-positioned div with its own dismiss timer and close button; gpui-kit has a notification system, so `ExportWatcher` pushes into it instead, keyed by a marker type so a running export's progress replaces its own toast in place rather than stacking. That watcher polls the export slot at 150 ms rather than being driven from the encode thread, and the reason is a frozen boundary, not laziness: `start_export` runs on a plain std::thread with no window handle, and `export_slot()`'s mutex has no notification hook. Pushing from inside would mean modifying code this workstream was told not to touch. The idiom is also the library's own — `NotificationList::start_advancing` in gpui-component polls its lifecycle the same way, with the same `cx.spawn_in` + timer shape. `frame_for_change` comes back with tests it never had. It maps a change's pointer to a playable frame so clicking a diff entry scrubs to where the element actually lives. Two details in it are load-bearing and were previously recorded nowhere: `start_at` is floored at zero, because a negative value would walk the playhead backward into the previous scene; and the result is clamped to the last frame, because a late `start_at` — or one left over from a scene that has since been shortened — would otherwise index past the end of the task list. It used to live inline in a Dioxus component, which is why it had no coverage; it now has five tests. The annotation capture box reads its target from `EditorState.selected` rather than taking pointer and kind as constructor props. The props version put a stale-data footgun in the caller's hands: whoever mounts the box would have to remember to update it on every selection change, and forgetting would file a comment against the wrong element in silence. Reading at submit time makes that unrepresentable, and matches how the box already read the playhead. Diff badges use theme tokens instead of the literal #22c55e the CSS carried, so the panel follows light and dark without a second palette. Persistence is untouched: JSON sources rewrite the scenario and record an undo snapshot, HTML sources write the sidecar and never touch the HTML, and a corrupt sidecar is still an error rather than something to overwrite. Left for integration with WS-4: `TOPBAR_HEIGHT` is px(40.), transcribed from the old CSS, and needs checking against the real topbar once it exists. * fix(studio): use as_chunks_mut for the rgba-to-bgra swap CI's clippy rejected `chunks_exact_mut(4)` under `clippy::chunks_exact_to_as_chunks`, while the same command passed locally. The lint is not the interesting part; the gap is. This machine runs rustc 1.97.1 from July, CI pins stable as of today, and the lint landed in between. So a local green does not close a workstream here — it only makes one ready for CI to judge. Worth stating plainly, because every earlier failure in this chantier reproduced locally (the linker one failed on both sides) and this is the first that could not. Refs #306 * feat(studio): rebuild the property inspector on gpui-kit Closes #311. Part of #306. The largest piece of the chantier: 2 225 lines in one file become 2 856 across four, split along the line that matters — what depends on the UI framework and what does not. `sections.rs` carries the curated schema and its value helpers with zero gpui in it, so the tables that decide which control appears for which property can be tested as data. `write.rs` carries the write path. `controls.rs` is the only place an Entity is created. `mod.rs` assembles the panel. The write path keeps both of its halves, and they stay independent: an edit applies optimistically in memory so the canvas moves within one render, and separately queues a Mutation behind a 250 ms debounce that, when it fires, re-reads the file from disk and replays the queue onto *that* content. Replaying onto a snapshot would be simpler and would silently destroy an agent's edit landing mid-window; `tests/audit_ws_g.rs` simulates exactly that race and is green. Dioxus's cancellable Task becomes `Option<gpui::Task<()>>`, where replacing the field drops and cancels the previous one — same semantics, and this time the cancellation is the language's rather than the framework's. Entities are created once per selection, not per render. `ensure_controls_for_selection` runs at the top of every render but rebuilds only when the selected pointer changes, so typing, dragging a slider or picking a colour never destroys the widget holding the cursor. The one deliberate exception is structural edits — adding or removing a list entry, switching a Fill between solid and gradient — which change how many controls exist and so must rebuild; `force_rebuild` marks those explicitly rather than letting the rebuild gate guess. `apply_open_change` comes back with its test. It was the one test this chantier had lost, deleted with the in-house colour picker, and it encodes a rule the kit does not: `ColorPickerState` owns its own `open` flag and emits no open/close event, so panel-wide exclusivity is enforced by observing each picker's `is_open()` and force-closing the others. Scrolling the panel still closes the expanded one. On the curated schema, which is what sank the discarded first attempt: 44 Field entries and 64 assembled rows, both matching the original and both now locked by a test. The earlier "45 against 47" alarm turned out to be a miscount — a raw grep for `Field {` also catches the struct definition and the `Mutation::Field` literals in the write path. Counting the thing rather than the string is what a test buys here, and the registry's own completeness invariant (every CssStyle property lands in exactly one section, unmapped ones falling to Advanced) is untouched and still locked. Bold/Italic and the alignment group render as text labels rather than icons. The Lucide variants exist in gpui-kit-assets but the embedded default bundle does not carry them, and a missing icon fails silently at runtime rather than at compile time — not something to gamble on without a window to look at. Swapping them back is one line once the asset registration grows. 159 tests pass, up from 144. * feat(studio): rebuild the library home on gpui-kit and mount it Closes #309. Part of #306. Thumbnails stop being JPEGs. The frozen `data::render_thumbnail` encodes to JPEG because the old wry handler served them to an <img> tag; feeding gpui that would mean decoding straight back to the pixels the renderer already had. The library now calls `build_frame_tasks` + `render_frame_rgba_deep` at scale 0.25 and hands the result to WS-2's `frame_from_rgba`, so the whole codec round trip disappears and the two image paths in the app are one path. That makes the grid a real memory surface, since every decoded thumbnail owns a Metal texture in an atlas with no eviction. It is bounded to exactly what is visible: `sync_thumbnails` diffs the current section-and-search result against what is held, renders what appeared, and hands what vanished to `cx.drop_image`. Switching folders or narrowing the search frees the rest. The grid computes its own column count from the viewport each render, because `grid_cols` takes a fixed number and there is no per-track minmax — so `repeat(auto-fill, minmax(240px, 1fr))` becomes an arithmetic floor over the available width, which resizes like the CSS did rather than approximating it with flex wrap. `StudioRoot` stops being a placeholder for the library half and mounts the screen. It also gains `cx.observe` on the shared state: `view` lives in `StudioState`, so a card click flipping it to `Editor` changes a field the root had read but was not subscribed to, and the screen would not have repainted. One bug worth recording, because it was in a test rather than in code: the first version of the recents test read the developer's real `~/.config/rustmotion/recent.json` and passed or failed depending on what that machine happened to have opened. It now sets `LibraryState.recents` directly and covers both branches. A test that depends on the machine it runs on is worse than no test — it reports the environment, not the behaviour. Also noticed, not acted on: `LibraryState.thumb_cache` appears to have no callers left. It cached JPEG bytes for the wry handler that no longer exists. It sits in a frozen file and belongs to its own change, not this one. 166 tests pass, up from 159. * feat(studio): land the editor shell and connect the prefetch cache to the canvas Closes #310. Part of #306. The last of the six workstreams, and the one that assembles the others: topbar, canvas, hit overlay, transport and shortcuts, mounting the inspector, the diff panel, the annotations panel, the export watcher and the frame surface. The app opens on the library, opens a scenario into the editor, and both screens boot clean. Shortcuts move from bubbled key events to gpui actions bound under a `key_context`. The old code kept Space out of text fields by calling `stop_propagation` on every container that might contain one — a rule enforced at each site and silently broken by forgetting one. gpui resolves bindings by context specificity instead, so an `Input` owning `left`/`right`/`cmd-z` under its own context wins over the editor's while focused, without the editor knowing text fields exist. `playback_action` keeps its four tests and stays the single source of truth; only its key type changes, since gpui has no `Key` enum and hands over a plain string. The larger fix here was not in WS-4's own code. The prefetch workers filled a frame cache that nothing ever read: `FrameCache::get` had no callers anywhere in the crate. The workers rendered every frame in the window ahead of the playhead, JPEG-encoded it and stored it, while the canvas independently re-rendered the same frame in RGBA — so the pool was not merely useless, it competed for CPU with the render the UI was waiting on. It surfaced only when the transitional crate-level `#[allow(dead_code)]` came off. The cause is a gap in the decomposition rather than in either workstream. `FrameKey` was frozen as a shared contract, but nothing in either brief said the canvas must consult the cache before rendering, so WS-2 built the producer, WS-4 built the consumer, and each was internally correct. WS-4 even defined its own local `FrameKey` tuple for render deduplication, which is exactly what a missing contract looks like from the inside. The canvas now builds the real `FrameKey` — model generation for side B, baseline source hash for side A, matching what the workers key on — and decodes a cache hit rather than re-rendering. Decoding a JPEG costs a few milliseconds against tens for a Skia render, and it keeps the cache's memory profile and its tested eviction policy untouched. Caching raw RGBA would be faster still, at 8 MB a frame against a cap of 120; that trade deserves its own change. 171 tests pass, up from 166. * refactor(studio): drop the dioxus-era leftovers the rewrite made unreachable Part of #306. The transitional `#[allow(dead_code)]` on `editor`, `library`, `state` and the theme module existed because, mid-chantier, each mixed live code with code whose only caller was a view file not yet ported. All six workstreams have landed, so the allows come off — and with them off, the compiler names what the rewrite left behind. `scenario::Theme` goes. It mapped Dark/Light/System onto the CSS class names `rm-dark`/`rm-light`/`rm-system`, which is a sentence that only parses in a webview. `ThemePref` in `app/state.rs` replaced it, and nothing had referenced the old one since WS-1. `render_thumbnail` goes. It encoded frame 0 to JPEG for the wry asset handler that served `/thumb/{i}`; the library now renders straight to RGBA through the same path as the canvas, so the encode had no consumer. `seg_button` and `num_display` go the same way — superseded during the inspector rewrite by `fill_seg_button` and by `properties::display_number`. Three declarations keep a narrow, deliberate allow rather than being stripped: `SelectionData`, `ColorWidget` and `ScalarWidget` each carry fields the inspector populates and never reads. Removing them means proving, field by field, that nothing in 2 857 lines of freshly written code wanted them — which is a review, not a cleanup, and belongs to its own change rather than to the tail of this one. The allow is on the exact declaration so a future reader sees the scope, not a blanket over the module. What this pass is really worth is the defect it exposed rather than the lines it deleted: the crate-level allow was also hiding `FrameCache::get`, uncalled, which meant the entire prefetch pool was rendering frames nothing consumed. That is fixed in the preceding commit. A blanket allow does not merely tolerate dead code, it conceals live code that stopped being reachable. * fix(studio): make the editor actually render, and stop blocking the window on assets Part of #306. Four defects, all found by opening the app and looking at it. Every one of them survived a green `fmt`/`clippy`/`test` run and a check that the process stayed alive for six seconds — which is exactly as much as that check is worth. Two of them made the editor useless. **No icons anywhere.** `gpui_kit::application()` was called without `.with_assets(..)`, so no asset source was registered and every icon resolved to nothing — silently, because a missing icon is not an error. It registers `AllAssets` rather than the curated set: WS-4 and WS-5 had each fallen back to text labels for icons the default bundle does not embed (mute, theme-system, baseline, diff, comments, export, bold, italic, alignment), and the full Lucide catalog removes the reason for those fallbacks. **A blank canvas.** The aspect-ratio box carried `aspect_ratio` plus `max_w_full`/`max_h_full` and no concrete dimension. In flexbox, a box with only maximum constraints resolves to zero, and the frame inside it is `size_full` — 100% of nothing. Measured before and after by printing the resolved bounds: 0 × 0, then 1222 × 687. The frame had been rendering correctly the whole time and being painted into a box with no area. **The scrubber pinned at maximum.** `EditorView` is constructed once, in `StudioRoot::new`, before any scenario is open — so the slider was built with the empty scenario's frame count, its `max` froze at 1, and every `set_value` clamped there. `SliderState::max` is a consuming builder with no runtime setter, so the fix is not to update it but to stop depending on it: the slider now runs on a normalized 0..1 fraction, converted at both ends. That also unbinds the slider's lifetime from the scenario's, so the same bug cannot return through that path when a different file is opened. **Up to a second of dead time before the window appeared.** `prefetch_icons` and `preextract_video_frames` ran synchronously ahead of `gpui_kit::application().run(..)`. Measured: 53 ms for component-showcase, 30 ms for depth-3d-showcase, **909 ms for mega-showcase** — during which there is no window at all, so it reads as the app being slow to start rather than slow to load. They move to a background thread that bumps the model generation when done, and the canvas shows a spinner while `frame` is `None` instead of empty space. mega-showcase now opens immediately and paints its first frame in 53 ms. The scrubber conversion gets four tests, including the round trip and the one-frame scenario where the denominator is zero. 175 tests pass, up from 171. * fix(studio): give the hit map its own stack before running it off the main thread Part of #306. The preceding commit moved hit-map computation onto `cx.background_spawn` to stop it freezing the UI for 75 ms per frame change. gpui's background executor runs on libdispatch worker threads, which get **544 KiB** of stack. The hit map goes through `render_scene_hits` → `prepare_scene` → `deserialize_children`, and untagged serde recurses once per level of scene nesting, buffering each level through `ContentDeserializer`. It overflowed and took the process with it: `EXC_BAD_ACCESS` in a stack guard region, roughly thirty seconds into a session. This crate already knew. `RENDER_STACK` is 32 MiB and `render_frame_deep` exists precisely because the prefetch workers hit the same wall on Rust's 2 MiB default — and 544 KiB is four times worse than what broke them. What was missing is that `frame_hits` never had a `_deep` counterpart, because until now it only ever ran on the main thread, where macOS hands out 8 MiB. Moving it off that thread removed the one thing that was holding it up, and nothing in the type system said so. `frame_hits_deep` mirrors the two existing wrappers. All three call sites that reach the engine's recursion now own a 32 MiB stack: the hit map and the canvas frame on the background executor, and the library thumbnails. The regression test spawns a thread with exactly libdispatch's 544 KiB and calls through `frame_hits_deep` against `mega-showcase.json`. It is worth noting how it fails if the fix is reverted: a stack overflow aborts rather than unwinds, so `join()` never returns and the whole test binary dies. Loud, if not graceful — which is the right trade for a defect whose signature is a process disappearing. 175 tests pass. * fix(studio): reset the editor when a different document is opened Part of #306. Opening a project, returning to the library and opening another one showed the first project's frame for an instant. `EditorView` is built once, in `StudioRoot::new`, and every field it holds outlives the document it describes. The flicker was the visible part. The rest of the state survived too, and one piece of it is worse than cosmetic: `selected` carries a JSON pointer like `/scenes/2/children/3`, which resolves perfectly well in the *new* document and addresses a different element. An inspector edit would have landed on the wrong node without anything looking wrong. The playhead also stayed where it was — frame 412 of a document that may only have 200 — and diff mode stayed armed against a baseline belonging to the previous file. `sync_document` compares the model's path against the one the view last drew and resets when it changes. The comparison is on the path, not the generation, deliberately: a hot reload of the *same* file must keep showing the current frame while the new one renders, or every keystroke in the inspector would flash a spinner. Only a different document clears. `reset_for_new_document` is split out as a pure function over `EditorState`, and it hands the stale frame back rather than dropping it. The caller has the `Window` and must pass it to `cx.drop_image`; `#[must_use]` makes ignoring the return value a warning. The obligation that leaks ~8 MB of VRAM per miss is now in the signature instead of in someone's memory. What is deliberately kept: preview quality, the Inspect toggle and the Comments toggle. Those are choices about how the user works, not about the file. Four tests cover the split, one per class of state — including the selection, which is the one that was quietly dangerous. 179 tests pass. * perf(studio): decode audio on the warmup thread, not in the click handler Part of #306. Opening a project froze the UI for over two seconds. The instrumented trace puts the whole delay before the editor even begins to draw: [OPEN+ 2.13s] sync_document: new document detected Loading audio: out/showcase/assets/track.wav `StudioModel::new` called `analyze_scenario_audio`, which decodes every track and runs an FFT over it — 24.7 MB of wav for that project. It ran inside `open_scenario`, on the UI thread, *before* the view switched to the editor. So the freeze was in the library, with the library still on screen, and the editor's spinner could not appear by construction: the thing meant to cover the wait was downstream of it. This also explains why the earlier measurements missed it. They were taken on `examples/*.json`, none of which carry audio; the project that froze lives under `out/`. A sample that excludes the failing case measures nothing. The analysis moves onto the thread that already warms up icons and video frames — renamed `spawn_scenario_warmup` since it is no longer only prefetch — and publishes `audio_error` plus a generation bump when it lands. Deferring is safe because the analysis writes into a global cache the renderer reads: the first frame paints without waveform data and the bump repaints it. An `Arc::ptr_eq` guard drops the result if the model has been replaced meanwhile, so a slow analysis cannot attach its findings to a document nobody opened. The watcher's reload path gets the warmup too. It calls `StudioModel::new` like every other path, and would otherwise have lost audio analysis entirely. Measured on the same project: `sync_document` at **183 ms** instead of 2.13 s, with the spinner up at 183 ms and the frame at 356 ms. "Loading audio" now appears in the log after the spinner rather than before it. The regression test does not time anything — a timing assertion would be flaky. It builds a model whose audio track does not exist: had the analysis run inline, the missing file would have set `audio_error`. That the field is `None` is proof the work was deferred. 179 tests pass. * fix(studio): let the user move the playhead while the video is playing Part of #306. Scrubbing during playback snapped straight back. The playhead has two writers with opposite intents and no way to tell them apart: the clock advancing time, and the user asking to be somewhere else. Both just assigned `state.current`. While a scenario has audio, the clock takes its next position from `audio::position_frame` — the sound card's idea of where we are, which a scrub does not move. So the tick after a scrub restored the old frame, and `audio_armed` stayed true, so the sound was never repositioned either. The playhead could not be moved at all while playing. The count matters for how this is fixed. Nine places write `state.current`: the clock, the scrubber, two step buttons, four keyboard actions, Present, a diff entry, and an annotation's "go to frame". Eight of the nine are seeks, and patching each to re-arm the audio would work until someone adds a tenth. So the clock detects instead. It remembers the frame it last wrote; if what it reads back differs, something else moved the playhead, and it adopts that position and re-arms the sound there rather than overwriting it. No call site has to know, and a tenth writer is covered on the day it appears. `advance_playhead` is a pure function over (current, last written, audio position, total) returning the next frame and whether to re-arm. Seven tests cover it, and they are where the intent lives now: a forward scrub, a backward one, undisturbed playback with and without audio, the first tick of a session where there is no previous write to compare against, the wrap past the last frame that has to restart the track rather than let it run on, and a seek past the end being clamped. Undisturbed playback still follows the audio clock rather than a timer. That is deliberate and unchanged: a timer ticking at 1/fps drifts against the sound card, and by the end of a long scenario the picture no longer matches what you hear. 188 tests pass. * fix(studio): stop aborting on playback keys, and pause when the user seeks Part of #306. Two changes, one a crash and one a behaviour the user asked for. **Any playback key killed the process.** Not a panic — an abort: panicked at gpui-pre-0.3.6/src/window.rs:5175: called `Option::unwrap()` on a `None` value panic in a function that cannot unwind thread caused non-unwinding panic. aborting. `Window::request_animation_frame` starts with `self.current_view()`, which reads the stack of entities being rendered and unwraps it. It is only valid inside a render or prepaint pass. `apply_playback_action` called it at the end of every transport action, and those run during key-event dispatch — outside any render. Because the panic unwinds through Objective-C frames, which cannot unwind, it aborted rather than failing. The call is simply removed: `EditorView::render` already requests the next animation frame while playing, which is the documented place for it. Toggling play notifies, notifying renders, and the render requests the frame. Nothing is lost, and the transport keys stop being fatal. **Moving the cursor on the timeline now stops playback**, so restarting is always an explicit act. The codebase already worked this way for the step buttons, which set `playing = false`; the scrubber did not, which is why dragging during playback fought the clock. `seek_from_user` is the one path for "the user asked to be somewhere else": it sets the frame and clears `playing`. Three call sites use it — the scrubber, a diff entry, and an annotation's "go to frame". The last two are inspection jumps, and leaving the video running after clicking "show me this element" would be strange. Home and End are deliberately left alone. They are transport controls rather than inspection jumps, and jumping to the start of a playing scenario reads as a restart, not as a request to stop. It returns whether anything changed, and that return is why dropping the cursor on the frame already displayed still counts: the frame did not move but the transport did, so the view has to repaint or the play button would keep showing pause. Three tests cover exactly that distinction. 191 tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.