GNOME Bugzilla – Bug 676854
timeline: Add a new "finished" signal
Last modified: 2012-05-31 22:31:40 UTC
The "completed" signal is not a good API, as it doesn't track repeat counts, leading to doing the tracking manually. Add a new signal that does the tracking for you.
Created attachment 215012 [details] [review] timeline: Add a new "finished" signal The finished signal is emitted when the timeline has been completely exhausted, removing the need to track the repeat count independently in a completed signal handler.
I don't like "finished" much. we could have a ::stopped signal, with a boolean value for signalling the difference between "the stop() method was called" and "the timeline finished after all its repeats", something like: void (* stopped) (ClutterTimeline *timeline, gboolean finished); the ::completed signal should also be deprecated at some point, given the broken semantics - but that's for another time.
another point for having an explicit ::stopped signal is that currently calling clutter_timeline_stop() will call pause() + rewind(), which will emit the ::paused signal before rewinding the timeline. having a ::stopped signal would allow connecting to a single signal to actually get notification of an interruption - either programmatic or because of the end of the animation.
I still think there should be two separate signals. The goal of something like "finished" was: static void animation_finished (ClutterTimeline *timeline, ClutterActor *actor) { clutter_actor_destroy (actor); } static void flash (void) { ClutterActor *flasher = clutter_actor_new (); ClutterTimeline *timeline; clutter_actor_set_background_color (flasher, 0xFFFFFFFF); /* stretch to width/height of screen */ clutter_actor_set_opacity (flasher, 0.0); clutter_actor_push_easing_state (flasher); clutter_actor_set_opacity (flasher, 1.0); timeline = CLUTTER_TIMELINE (clutter_actor_get_transition (flasher, "opacity")); clutter_timeline_set_repeat_count (timeline, 3); clutter_timeline_set_auto_reverse (timeline, TRUE); g_signal_connect (timeline, "finished", animation_finished, flasher); }
(In reply to comment #4) > I still think there should be two separate signals. The goal of something like > "finished" was: I understand the goal, but in the wild I have experienced code that wanted to do something after stopping the timeline - like reversing the animation; instead of having two signals, we could conflate both the cases into one to simplify the handlers, for instance: static void animation_stopped (ClutterTimeline *timeline, gboolean isFinished, ClutterActor *actor) { if (isFinished) clutter_actor_destroy (actor); else set_actor_to_initial_state (actor); } static gboolean stop_timeline (ClutterActor *actor, ClutterEvent *event, ClutterTimeline *timeline) { clutter_timeline_stop (timeline); } static void changeState (void) { ClutterActor *actor = clutter_actor_new (); ClutterTimeline *timeline; set_actor_to_initial_state (actor); clutter_actor_save_easing_state (actor); timeline = set_actor_to_final_state (actor); clutter_timeline_set_repeat_count (timeline, 3); clutter_timeline_set_auto_reverse (timeline, TRUE); g_signal_connect (timeline, "stopped", animation_stopped, actor); g_signal_connect (actor, "key-press-event", stop_timeline, timeline); }
Yeah, that actually looks pretty neato.
Created attachment 215253 [details] [review] timeline: Add a new "stopped" signal The ::stopped signal is emitted when the timeline has been completely exhausted or when the timeline has been programmatically stopped by using clutter_timeline_stop(); the notification at the end of the timeline run allows to write handlers without having to check whether the current repeat is the last one, like we are forced to do when using the ::completed signal. Based on the patch by: Jasper St. Pierre <jstpierre@mecheye.net>
Created attachment 215254 [details] [review] timeline: Add a new "stopped" signal The ::stopped signal is emitted when the timeline has been completely exhausted or when the timeline has been programmatically stopped by using clutter_timeline_stop(); the notification at the end of the timeline run allows to write handlers without having to check whether the current repeat is the last one, like we are forced to do when using the ::completed signal. Based on the patch by: Jasper St. Pierre <jstpierre@mecheye.net>
Review of attachment 215254 [details] [review]: ::: clutter/clutter-actor.c @@ +17831,3 @@ * transition = clutter_actor_get_transition (actor, "rotation-angle-y"); + * g_signal_connect (transition, "stopped", + * G_CALLBACK (on_transition_finished), leftover "finished" @@ +17835,3 @@ * ]| * + * will call the <function>on_transition_finished</function> callback when leftover "finished" ::: clutter/clutter-timeline.c @@ +796,3 @@ + G_STRUCT_OFFSET (ClutterTimelineClass, completed), + NULL, NULL, + _clutter_marshal_VOID__BOOLEAN, Still using the deprecated marshalling generator instead of the generic libffi one? @@ +930,3 @@ ClutterMasterClock *master_clock; + is_playing = !!is_playing; uhhh @@ +937,2 @@ priv->is_playing = is_playing; + unintended whitespace? ::: examples/basic-actor.c @@ +51,2 @@ static void +on_transition_finished (ClutterTransition *transition, leftover "finished" @@ +77,3 @@ transition = clutter_actor_get_transition (actor, "rotation-angle-y"); + g_signal_connect (transition, "stopped", + G_CALLBACK (on_transition_finished), leftover "finished"
(In reply to comment #9) > ::: clutter/clutter-timeline.c > @@ +796,3 @@ > + G_STRUCT_OFFSET (ClutterTimelineClass, completed), > + NULL, NULL, > + _clutter_marshal_VOID__BOOLEAN, > > Still using the deprecated marshalling generator instead of the generic libffi > one? where did you see that the marshalling generator was deprecated? it's not. we even improved the generation of marshallers through varargs to avoid GValue boxing and unboxing if there are no signal handlers outside of the class closure for GLib 2.32. the ffi-based marshaller is also slower in profiles than the autogenerated code. > @@ +930,3 @@ > ClutterMasterClock *master_clock; > > + is_playing = !!is_playing; > > uhhh wonders of C. that's how you do boolean checks.
Attachment 215254 [details] pushed as de4d70a - timeline: Add a new "stopped" signal
pushed with the fixes from the issues outlined in comment 9
(In reply to comment #10) > > @@ +930,3 @@ > > ClutterMasterClock *master_clock; > > > > + is_playing = !!is_playing; > > > > uhhh > > wonders of C. that's how you do boolean checks. You changed it from something equivalent.
(In reply to comment #10) > (In reply to comment #9) > > ::: clutter/clutter-timeline.c > > @@ +796,3 @@ > > + G_STRUCT_OFFSET (ClutterTimelineClass, completed), > > + NULL, NULL, > > + _clutter_marshal_VOID__BOOLEAN, > > > > Still using the deprecated marshalling generator instead of the generic libffi > > one? > > where did you see that the marshalling generator was deprecated? it's not. http://git.gnome.org/browse/glib/tree/gobject/gmarshal.list
(In reply to comment #13) > You changed it from something equivalent. not really; basically everywhere we do an immediate conversion using !! instead of checking against FALSE; I noticed that during the patch because I was already touching that section. yes, it should have been a separate commit; no, I didn't care enough to do that. (In reply to comment #14) > > where did you see that the marshalling generator was deprecated? it's not. > > http://git.gnome.org/browse/glib/tree/gobject/gmarshal.list those are the ones generated by GObject; it doesn't imply that the whole system is being deprecated - otherwise it would have been.