GNOME Bugzilla – Bug 695882
Various fixes to the new gnome-shell backgrounds
Last modified: 2013-03-14 21:48:41 UTC
Ray and I sat down together today and debugged all this out. This fixes various issues that people have seen with backgrounds in the gnome-shell code. Depends on bug #695881
Created attachment 238929 [details] [review] background: Fix updating existing images that need loading Due to weird and strange JS scoping semantics, if we are in a callback, "i" won't be captured and when the callback is called, we'll have the wrong index, causing addImage to be called instead of updateImage.
Created attachment 238930 [details] [review] background: Set the correct index on images The pattern is underneath, so we need to add one. It turns out that ClutterGroup doesn't care about that, but we need to do this to remove deprecations in mutter.
Created attachment 238931 [details] [review] background: Fix math to calculate the interval from the duration The math here before was incorrect. This is still wrong, as we're looking at the total duration of the animation rather than the next step.
Created attachment 238932 [details] [review] background: Rename animationUpdate to updateAnimation This is more consistent.
Created attachment 238933 [details] [review] background: Keep the state of key frame files on Animation This gets us from a mix of pure getters and class state to just class state.
Created attachment 238934 [details] [review] background: Fix a bug with empty animations Arrays always evaluate to true, even empty arrays, so we need to check the length to make sure we have no files.
Created attachment 238935 [details] [review] background: Look at the duration per-step We need to look at the duration of the current step of the slideshow to determine when to next queue an event, rather than the full slideshow duration.
Review of attachment 238929 [details] [review]: ++
Review of attachment 238930 [details] [review]: ::: js/ui/background.js @@ +357,3 @@ let actor = new Meta.BackgroundActor(); actor.content = content; + this.actor.insert_child_at_index(actor, index + 1); probably should have a comment explaining what the magic + 1 means. Note, if we later decide to elide the pattern actor in cases where it's completely covered, this + 1 will no longer be right (but that's neither here nor there, I guess)
Review of attachment 238931 [details] [review]: ::: js/ui/background.js @@ +445,2 @@ let interval = Math.max(ANIMATION_MIN_WAKEUP_INTERVAL * 1000, + timePerStep); this was just call kinds of wrong before.
Review of attachment 238932 [details] [review]: This one i'm pretty indifferent about. To me, you enqueue "things" and an "animation update" is a thing. anyway doesn't matter either way. go for it.
Review of attachment 238933 [details] [review]: sure
Review of attachment 238934 [details] [review]: ++
Review of attachment 238935 [details] [review]: yup, definitely need this for things to update frequently enough.
Attachment 238929 [details] pushed as 1759111 - background: Fix updating existing images that need loading Attachment 238930 [details] pushed as c4470fd - background: Set the correct index on images Attachment 238931 [details] pushed as eeea855 - background: Fix math to calculate the interval from the duration Attachment 238932 [details] pushed as b351536 - background: Rename animationUpdate to updateAnimation Attachment 238933 [details] pushed as 3a27d0b - background: Keep the state of key frame files on Animation Attachment 238934 [details] pushed as 8edd7ad - background: Fix a bug with empty animations Attachment 238935 [details] pushed as 5fecd07 - background: Look at the duration per-step