After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 687583 - Programmatic animation control of panel.AnimatedIcon
Programmatic animation control of panel.AnimatedIcon
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-11-04 18:46 UTC by Stéphane Démurget
Modified: 2012-11-17 21:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
panel: programmatic anim. control of AnimatedIcon (3.10 KB, patch)
2012-11-04 18:51 UTC, Stéphane Démurget
none Details | Review
panel: programmatic anim. control of AnimatedIcon (3.11 KB, patch)
2012-11-04 18:53 UTC, Stéphane Démurget
accepted-commit_now Details | Review
panel: programmatic anim. control of AnimatedIcon (3.11 KB, patch)
2012-11-05 14:06 UTC, Stéphane Démurget
none Details | Review
panel: Fix a regression from the previous commit (1013 bytes, patch)
2012-11-05 15:45 UTC, Stéphane Démurget
rejected Details | Review
panel: programmatic anim. control of AnimatedIcon (7.18 KB, patch)
2012-11-05 22:25 UTC, Stéphane Démurget
needs-work Details | Review
panel: programmatic anim. control of AnimatedIcon (7.74 KB, patch)
2012-11-06 14:30 UTC, Stéphane Démurget
committed Details | Review
panel: programmatic anim. control of AnimatedIcon (7.76 KB, patch)
2012-11-17 21:30 UTC, Stéphane Démurget
committed Details | Review

Description Stéphane Démurget 2012-11-04 18:46:51 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.
Comment 1 Stéphane Démurget 2012-11-04 18:51:39 UTC
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.
Comment 2 Stéphane Démurget 2012-11-04 18:53:50 UTC
Created attachment 228046 [details] [review]
panel: programmatic anim. control of AnimatedIcon

A tab vs space issue fixed.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-11-04 18:57:38 UTC
Review of attachment 228046 [details] [review]:

I was just about to publish my review for the indentation issue! Thanks.

This looks fine.
Comment 4 Stéphane Démurget 2012-11-04 19:28:14 UTC
Cool, can you commit for me until I get an account?
Comment 5 Stéphane Démurget 2012-11-05 14:06:53 UTC
The following fix has been pushed:
e04a4c3 panel: programmatic anim. control of AnimatedIcon
Comment 6 Stéphane Démurget 2012-11-05 14:06:57 UTC
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.
Comment 7 Rui Matos 2012-11-05 15:26:41 UTC
This patch doesn't work correctly. Please see bug 687653.
Comment 8 Stéphane Démurget 2012-11-05 15:45:34 UTC
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 9 Stéphane Démurget 2012-11-05 22:13:28 UTC
Comment on attachment 228132 [details] [review]
panel: Fix a regression from the previous commit

This is not the correct way of fixing it.
Comment 10 Stéphane Démurget 2012-11-05 22:25:12 UTC
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
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-11-05 22:31:28 UTC
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.
Comment 12 Stéphane Démurget 2012-11-06 14:30:07 UTC
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.
Comment 13 Stéphane Démurget 2012-11-06 14:33:20 UTC
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).
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-11-06 16:05:05 UTC
(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.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-11-17 20:00:18 UTC
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
Comment 16 Stéphane Démurget 2012-11-17 21:30:26 UTC
The following fix has been pushed:
c48a246 panel: programmatic anim. control of AnimatedIcon
Comment 17 Stéphane Démurget 2012-11-17 21:30:34 UTC
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.