fix(workspace): Enable animation for theme background images - #14618
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Niyas Hameed.
|
|
Every PR must be linked to a same-repo issue before Oz can review it. Next step: open or find a same-repo issue describing this change, then link it to this PR by adding See the contribution guidelines for the full readiness model. Powered by Oz |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
Next step: open or find a same-repo issue describing this change, then link it to this PR by adding Closes #123 to the PR description (or using the "Development" sidebar on GitHub). A maintainer will mark the issue ready-to-implement when it is ready. Once it is marked, comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
932afa0 to
0507341
Compare
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @niyasrad on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Hi @niyasrad — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
af2639c to
733d9d6
Compare
There was a problem hiding this comment.
isn't this functionally identical to the previous code? was this only refactored out into a separate function to add all of the tests (which i don't think we get any value from)?
There was a problem hiding this comment.
Yes, realized that wasn't worth it. Reverted the extraction and dropped the tests that only existed to exercise it. Kept the ones that test get_current_frame(). Thank you for catching this!
| .cover() | ||
| .with_opacity(opacity_ratio) | ||
| .with_corner_radius(window_corner_radius) | ||
| .enable_animation_with_start_time(std::time::Instant::now()) |
There was a problem hiding this comment.
using Instant::now() here means that the animation restarts every time we re-invoke render() on this view, right? i think we should be storing the instant on Workspace (see the crates/warpui/examples/animated_images/root_view.rs example) so that there's a fixed start time.
There was a problem hiding this comment.
Yes, that makes sense! I've moved the instant onto Workspace, set once at construction following the pattern you linked.
0611ac7 to
7973aa8
Compare
|
@vorporeal thanks for catching both of those, appreciate the review. I've made the changes, let me know if this addresses it or if there's more to tweak 😃 |
|
@niyasrad thx for fix, rly waiting for it so that I could set an animated background (^◕.◕^) |
|
Hi @niyasrad — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
vorporeal
left a comment
There was a problem hiding this comment.
sorry for delayed response! was on vacation for a couple weeks.
| import_modal: ViewHandle<ImportModal>, | ||
| theme_chooser_view: ViewHandle<ThemeChooser>, | ||
| previous_theme: Option<ThemeKind>, | ||
| background_image_animation_start_time: std::time::Instant, |
There was a problem hiding this comment.
std::time::Instant::now() panics on wasm - please import instant::Instant and use Instant::now() instead.
There was a problem hiding this comment.
Yes, didn't realize std::time::Instant panics on wasm. Switched to instant::Instant, same as image.rs and the animated_images example.
Add enable_animation_with_start_time() call to the workspace background rendering path. Without this, animated GIF backgrounds freeze on their first frame instead of playing. This follows the established pattern used elsewhere in the codebase (e.g. changelog_section.rs:153).
Add regression coverage for the animated image frame advancement fix. The original bug was that paint_animated_image() would ignore the started_at field and always recompute Instant::now(), making elapsed_time ~0 and freezing animations on the first frame. The fix adds enable_animation_with_start_time() to set started_at, which paint_animated_image() then uses to compute elapsed time correctly. This commit includes: 1. Six solid tests directly verifying AnimatedImage::get_current_frame() frame advancement/wrapping over elapsed time with real GIF/WebP fixtures 2. Four regression tests for the paint_animated_image fix: - Tests that verify enable_animation_with_start_time() sets started_at - Tests that verify started_at is actually used for frame selection 3. Refactor: Extract elapsed-time computation into a pure helper method compute_elapsed_time_ms(&self, now: Instant) that can be unit-tested directly. This method respects self.started_at and would fail if someone reintroduces the bug (e.g., ignoring self.started_at or recomputing Instant::now() fresh). The new regression tests directly verify that compute_elapsed_time_ms uses self.started_at correctly, and would catch the regression if someone changed the logic to ignore started_at.
…setting in render() Calling Instant::now() inline in render() reset the animation's start time on every repaint, so elapsed time stayed ~0 and the background gif never advanced past its first frame. It also made paint_animated_image schedule a repaint every remaining_delay ms indefinitely, since started_at was always Some. Store the Instant once on Workspace at construction (same pattern as crates/warpui/examples/animated_images/root_view.rs) so elapsed time grows correctly across repaints.
Remove four tests that only checked started_at's Option state without exercising compute_elapsed_time_ms or paint_animated_image at all (two were mislabeled and didn't test what their names claimed). Keep the three tests that assert concrete elapsed-time values and would catch a regression to recomputing Instant::now() instead of using started_at. Also drop comments that just restated the adjacent assert/expect message or the next line's code.
…tests Per review: the extraction was functionally identical to the previous inline code and only existed to make elapsed-time computation unit-testable. Reverting it back to the original inline form in paint_animated_image, and removing the three tests that only existed to exercise it. Frame-timing behavior is still covered by the get_current_frame tests in image_cache_tests.rs, which exercise the same logic through the public API. Also drops the doc comment on background_image_animation_start_time.
…Instant std::time::Instant::now() panics on wasm32 targets. This codebase uses the instant crate as a cross-platform wrapper (a plain alias for std::time::Instant on native, a JS-clock-backed type on wasm) - already the convention in image.rs, changelog_section.rs, and the animated_images example this fix followed. Swap both the field type and its initializer to match.
7973aa8 to
ecbbc60
Compare
i am vorporeal and i approve this pr
…dev#14618) Adds the missing `enable_animation_with_start_time()` call to the workspace background rendering path (`app/src/workspace/view.rs`). Animated GIF theme backgrounds currently freeze on their first frame because the animation start time is never initialized at this call site. Follows the pattern used in `crates/warpui/examples/animated_images/root_view.rs`, which stores a fixed `Instant` on the view at construction and reuses it across `render()` calls instead of recomputing it inline. Closes warpdotdev#14923 (animated GIF theme backgrounds freeze on first frame). Also related to warpdotdev#5178 (`.gif` background support) — see [comment there](warpdotdev#5178) for context. This doesn't add `.gif` picker support in the theme UI (a separate change), but fixes the animation-freeze bug that would otherwise affect any `.gif` background, since the file-path deserialization doesn't restrict extensions today. - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). _Note: warpdotdev#14923 has been triaged (`bug`, `repro:high`) but is not yet labeled `ready-to-implement` — flagging this openly rather than checking it off._ - [ ] I have manually tested my changes locally with `./script/run` `cargo check -p warp` passes. I was not able to complete a full local `cargo build` + visual/screenshot verification — my dev machine has 7.7GB RAM, well under what's needed to comfortably build this project (Warp's CI runners use 64GB). I'd appreciate help verifying the visual animation behavior before merge. None yet — see testing note above. - [ ] Warp Agent Mode - This PR was created via Warp's AI Agent Mode <!-- CHANGELOG-BUG-FIX: Fixed animated GIF background images freezing on their first frame in the terminal window --> --------- Co-authored-by: David Stern <david@warp.dev>
Description
Adds the missing
enable_animation_with_start_time()call to the workspace background rendering path (app/src/workspace/view.rs). Animated GIF theme backgrounds currently freeze on their first frame because the animation start time is never initialized at this call site. Follows the pattern used incrates/warpui/examples/animated_images/root_view.rs, which stores a fixedInstanton the view at construction and reuses it acrossrender()calls instead of recomputing it inline.Closes #14923 (animated GIF theme backgrounds freeze on first frame). Also related to #5178 (
.gifbackground support) — see comment there for context. This doesn't add.gifpicker support in the theme UI (a separate change), but fixes the animation-freeze bug that would otherwise affect any.gifbackground, since the file-path deserialization doesn't restrict extensions today.Linked Issue
ready-to-specorready-to-implement.Note: #14923 has been triaged (
bug,repro:high) but is not yet labeledready-to-implement— flagging this openly rather than checking it off.Testing
./script/runcargo check -p warppasses. I was not able to complete a full localcargo build+ visual/screenshot verification — my dev machine has 7.7GB RAM, well under what's needed to comfortably build this project (Warp's CI runners use 64GB). I'd appreciate help verifying the visual animation behavior before merge.Screenshots / Videos
None yet — see testing note above.
Agent Mode