GNOME Bugzilla – Bug 687583
Programmatic animation control of panel.AnimatedIcon
Last modified: 2012-11-17 21:30:34 UTC
The AnimatedIcon does not have an API for controlling the animation but relies on the :visible property changes to start and stop a timeout used to update the frame. This has the inconvenient of having a side effect when visible is set to true multiple times, and is not really the API expected from such component. Switch to a start/stop API instead.
Created attachment 228045 [details] [review] panel: programmatic anim. control of AnimatedIcon The AnimatedIcon does not have an API for controlling the animation but relies on the :visible property changes to start and stop a timeout used to update the frame. This has the inconvenient of having a side effect when visible is set to true multiple times, and is not really the API expected from such component. Switch to a start/stop API instead. Also, update to the first frame at startup while we are at it, since this is the expected behavior.
Created attachment 228046 [details] [review] panel: programmatic anim. control of AnimatedIcon A tab vs space issue fixed.
Review of attachment 228046 [details] [review]: I was just about to publish my review for the indentation issue! Thanks. This looks fine.
Cool, can you commit for me until I get an account?
The following fix has been pushed: e04a4c3 panel: programmatic anim. control of AnimatedIcon
Created attachment 228113 [details] [review] panel: programmatic anim. control of AnimatedIcon The AnimatedIcon does not have an API for controlling the animation but relies on the :visible property changes to start and stop a timeout used to update the frame. This has the inconvenient of having a side effect when visible is set to true multiple times, and is not really the API expected from such component. Switch to a start/stop API instead. Also, update to the first frame at startup while we are at it, since this is the expected behavior.
This patch doesn't work correctly. Please see bug 687653.
Created attachment 228132 [details] [review] panel: Fix a regression from the previous commit The TextureCache is loaded asynchronously, so at the first update we do not have the number of images yet and this._frame gets NaN. We simply do not update in the constructor, like it used to be before.
Comment on attachment 228132 [details] [review] panel: Fix a regression from the previous commit This is not the correct way of fixing it.
Created attachment 228195 [details] [review] panel: programmatic anim. control of AnimatedIcon This is a followup to Jasper suggestion to add a load event callback in bug 687653. The AnimatedIcon API is cleaner. Notes: - it fixes the regression and the previous race - it starts at frame 0 (before this bug it started at 1) - there is no callback to know if loading failed, is that even useful? - stop() does not reset to the first frame; it depends how the animation is used, but this could be a seen as a bug
Review of attachment 228195 [details] [review]: ::: js/ui/panel.js @@ +86,2 @@ + this._isLoaded = false + this._isPlaying = false Missing semis. @@ +95,3 @@ + play: function() { + if (this._timeoutId == 0) + this._timeoutId = Mainloop.timeout_add(ANIMATED_ICON_UPDATE_TIMEOUT, Lang.bind(this, this._update)); Don't do this if it hasn't loaded yet. This needs to only happen when isLoaded && isPlaying are both true, meaning you need to do this when we call play(), or when it finishes loading. @@ +111,3 @@ _showFrame: function(frame) { + if (!this._isLoaded) + return; We should never get here in this case. @@ +131,3 @@ + _animationsLoaded: function() { + this._isLoaded = true; + this._showFrame(0); You need to ensure that it's playing before you do this. ::: src/st/st-texture-cache.c @@ +1106,3 @@ + + if (data->load_callback != NULL) + (*data->load_callback)(NULL, NULL); Uh, no. This should be: data->load_callback (cache, data->user_data); @@ +1162,3 @@ * @grid_height: Height in pixels + * @load_callback: (scope async) (allow-none): Function called from the main loop when + * the image is loaded and ready to use, or %NULL This needs to also take a @user_data that we pass back when we get the load_callback. The description here should also be flush with the left side, plus or minus a few spaces. "called from the main loop" is redundant -- callbacks are assumed to be passed from the context of the calling thread if not explicitly specified.
Created attachment 228250 [details] [review] panel: programmatic anim. control of AnimatedIcon The AnimatedIcon does not have an API for controlling the animation but relies on the :visible property changes to start and stop a timeout used to update the frame. This has the inconvenient of having a side effect when visible is set to true multiple times, and is not really the API expected from such component. Also, there is a race if it is displayed before the images are loaded: there is no child yet and thus we get this._frame = NaN which leads to a crash. Switch to a play/stop API instead, and add a load event callback to the TextureCache.load_slice_image to exactly know when we can start using the images.
Do you know from looking the patch why using user_data fails? If I pass a string for instance, I get nothing, there is no crash, I just get undefined parameters (no parameter).
(In reply to comment #13) > Do you know from looking the patch why using user_data fails? > If I pass a string for instance, I get nothing, there is no crash, I just get > undefined parameters (no parameter). user_data is used by gjs internally to store state that it needs to.
Review of attachment 228250 [details] [review]: This looks good to me; minor rename, here. ::: src/st/st-texture-cache.c @@ +1075,3 @@ ClutterActor *actor; + GFunc load_callback; + gpointer user_data; load_callback_data
The following fix has been pushed: c48a246 panel: programmatic anim. control of AnimatedIcon
Created attachment 229251 [details] [review] panel: programmatic anim. control of AnimatedIcon The AnimatedIcon does not have an API for controlling the animation but relies on the :visible property changes to start and stop a timeout used to update the frame. This has the inconvenient of having a side effect when visible is set to true multiple times, and is not really the API expected from such component. Also, there is a race if it is displayed before the images are loaded: there is no child yet and thus we get this._frame = NaN which leads to a crash. Switch to a play/stop API instead, and add a load event callback to the TextureCache.load_slice_image to exactly know when we can start using the images.