GNOME Bugzilla – Bug 705829
Allow users to specify duration and mode of go_to animation
Last modified: 2013-08-14 15:39:11 UTC
Created attachment 251334 [details] [review] view: add extra ensure_visibility and go_to functions In the gnome-maps when travelling from one location to another you perform two animated journeys. One to ensure that the source and the destination are both visible and one to travel to the destination. This results in two separate animations, each with it's own ease in and ease out. To make the travelling smoother control over the animation mode could be of help. For instance setting it to LINEAR or using ease in for the first journey and ease out on the second. See patch below for implementation suggestion. It adds new functions: champlain_view_go_to_with_duration champlain_view_go_to_with_animation_mode champlain_view_go_to_full and: champlain_view_ensure_visibility_with_duration champlain_view_ensure_visibility_with_animation_mode champlain_view_ensure_visibility_full as well as new enums: typedef enum { CHAMPLAIN_ANIMATION_EASE_IN = 0, CHAMPLAIN_ANIMATION_EASE_OUT, CHAMPLAIN_ANIMATION_EASE_IN_OUT CHAMPLAIN_ANIMATION_LINEAR } ChamplainGoToAnimationMode; Thanks
Zeeshan Ali had some comments when I posted this as a gnome-maps bug: "[...] I am also wondering if it would make sense to simply add properties to ChamplainView to set duration and animation mode instead of adding '_full' variants of all functions?" Since duration at the moment is calculated dynamically with help from the zoom-level and it feels like something that can/could be different for each animation I think that it should be a function argument. The animation mode I'm not that sure of. It might make more sense as a property. What do you think?
(In reply to comment #1) > Zeeshan Ali had some comments when I posted this as a gnome-maps bug: Thanks for providing my input as well. :) > "[...] I am also wondering if it would make sense to simply add properties to > ChamplainView to set duration and animation mode instead of adding '_full' > variants of all functions?" > > Since duration at the moment is calculated dynamically with help from the > zoom-level and it feels like something that can/could be different for each > animation I think that it should be a function argument. Well, i don't think apps do different animations on the map at the same time so if they need different duration/mode for separate animation, they could just set the props before starting the animation. We could also have separate props for duration/mode for each animation function (e.g 'zoom-in-duration', 'zoom-out-duration' etc) but I'm not sure that is worth it.
Created attachment 251351 [details] [review] view: add properties for animation mode and duration This is how it could look with properties instead of _full functions. In order to not break anything an additional boolean property, PROP_CALCULATE_GOTO_DURATION, is introduced. If this is false then animation duration will be taken from the property otherwise it will be take from the property PROP_GOTO_DURATION. The animation mode will be taken from PROP_GOTO_ANIMATION_MODE which defaults to the current EASE_IN_OUT. Thanks.
Created attachment 251354 [details] [review] view: add properties for animation mode and duration v2 Forget a commit --amend.
Review of attachment 251351 [details] [review]: ::: champlain/champlain-view.c @@ +104,3 @@ PROP_BACKGROUND_PATTERN, + PROP_GOTO_ANIMATION_MODE, + PROP_CALCULATE_GOTO_DURATION, I dont think we need a separate boolean for this. Duration can have special value of '0' that would imply libchamplain automatically calculating it for you.
Yeah, I thought of that. And maybe that is the way to go. I was discouraged by this code in champlain-view.c: if (duration == 0) { champlain_view_center_on (view, latitude, longitude); return; } But that can only happen if zoom_level is 0 if I'm reading the code right, so that could be worked around, surely.
Created attachment 251377 [details] [review] view: add properties for animation mode and duration v3 This version removes the PROP_CALCULATE_GOTO_DURATION and uses the value 0 of PROP_GOTO_DURATION as a special value that means that duration is calculated from zoom-level.
Review of attachment 251377 [details] [review]: Looks really good otherwise! ::: champlain/champlain-view.c @@ +104,3 @@ PROP_BACKGROUND_PATTERN, + PROP_GOTO_ANIMATION_MODE, + PROP_GOTO_DURATION These names suggests these are specific to 'goto' animations only but the name of the properties themselves suggests these are generic animation props. I think the latter is the case so we can just remove the '_GOTO' from these. @@ +930,3 @@ + "Go to animation mode", + "The mode of animation when going to a location", + CHAMPLAIN_TYPE_GOTO_ANIMATION_MODE, This should be CLUTTER_TYPE_ANIMATION_MODE. @@ +938,3 @@ + * + * The duration of an animation when going to a location. + * A value of 0 means that the duration is calculated based on zoom-level. In one case (in the existing code), the calculation is not based on zoom-level and I think app developer doesn't need to know this anyways so I suggest we just say "A value of 0 means that the duration is calculated automatically for you." @@ +1474,3 @@ + return CLUTTER_LINEAR; + } +} This won't be needed if we just use/expose ClutterAnimationMode directly. ::: champlain/champlain-view.h @@ +62,3 @@ + CHAMPLAIN_GOTO_ANIMATION_EASE_OUT, + CHAMPLAIN_GOTO_ANIMATION_EASE_IN_OUT +} ChamplainGotoAnimationMode; These correspond directly to clutter's animation mode so I don't think we need to introduce these. I'm guessing you wanted to keep 'clutter' an internal detail, which sounds good but thing is that its already exposed through other API.
(In reply to comment #8) > Review of attachment 251377 [details] [review]: > > Looks really good otherwise! > > ::: champlain/champlain-view.c > @@ +104,3 @@ > PROP_BACKGROUND_PATTERN, > + PROP_GOTO_ANIMATION_MODE, > + PROP_GOTO_DURATION > > These names suggests these are specific to 'goto' animations only but the name > of the properties themselves suggests these are generic animation props. I > think the latter is the case so we can just remove the '_GOTO' from these. > I specified GOTO because there is a concept of zoom animation as well. That sets it's own mode and duration in show_zoom_actor in champlain-view.c: clutter_actor_set_easing_mode (zoom_actor, CLUTTER_EASE_IN_OUT_QUAD); clutter_actor_set_easing_duration (zoom_actor, 350); zoom animation has it's own property (boolean) PROP_ANIMATE_ZOOM. But the naming didn't feel all right for me either, so I am up for suggestions. You still think it's allright to drop the goto? Or should the GOTO be added to the name of the properties? (I also wondered if GOTO_DURATION should be GOTO_ANIMATION_DURATION since the duration is not connected to the goto, but to the animation. If there is no animation there is no duration. > @@ +930,3 @@ > + "Go to animation mode", > + "The mode of animation when going to a location", > + CHAMPLAIN_TYPE_GOTO_ANIMATION_MODE, > > This should be CLUTTER_TYPE_ANIMATION_MODE. > Agreed! > @@ +938,3 @@ > + * > + * The duration of an animation when going to a location. > + * A value of 0 means that the duration is calculated based on zoom-level. > > In one case (in the existing code), the calculation is not based on zoom-level > and I think app developer doesn't need to know this anyways so I suggest we > just say "A value of 0 means that the duration is calculated automatically for > you." > Agreed. > @@ +1474,3 @@ > + return CLUTTER_LINEAR; > + } > +} > > This won't be needed if we just use/expose ClutterAnimationMode directly. > > ::: champlain/champlain-view.h > @@ +62,3 @@ > + CHAMPLAIN_GOTO_ANIMATION_EASE_OUT, > + CHAMPLAIN_GOTO_ANIMATION_EASE_IN_OUT > +} ChamplainGotoAnimationMode; > > These correspond directly to clutter's animation mode so I don't think we need > to introduce these. I'm guessing you wanted to keep 'clutter' an internal > detail, which sounds good but thing is that its already exposed through other > API. You are right and I see and agree. Thanks for review!
Created attachment 251461 [details] [review] view: add properties for animation mode and duration v4 Updated after review. This patch still has the GOTO-prefix but has harmonized the naming of the properties and the variables. Thanks.
One thing that makes me wonder if it only should be animation mode that should be a property is this comment in champlain-view.c: /* FIXME: make public after API freeze */ static void champlain_view_go_to_with_duration (ChamplainView *view, gdouble latitude, gdouble longitude, guint duration) /* In ms */ If the plan is to go_to_with_duration a public function then that makes a duration property kind of moot. Jiří?
(In reply to comment #11) > One thing that makes me wonder if it only should be animation mode that should > be a property is this comment in champlain-view.c: > > /* FIXME: make public after API freeze */ > static void > champlain_view_go_to_with_duration (ChamplainView *view, > gdouble latitude, > gdouble longitude, > guint duration) /* In ms */ > > If the plan is to go_to_with_duration a public function then that makes a > duration property kind of moot. This has been there for a very long time and i think with these new props, we make this function (or at least the FIXME comment) redundant. If we do the opposite, we'll have to introduce a champlain_view_go_to_with_mode() and champlain_view_go_to_with_duration_and_mode() etc and we already decided not to go that way. I'd suggest we just remove this function along with this patch.
Review of attachment 251461 [details] [review]: Seems good to push already. ::: champlain/champlain-view.c @@ +192,3 @@ + ClutterAnimationMode goto_animation_mode; + guint goto_animation_duration; Nitpick: I think for these private fields, we don't need the '_animation' part but I leave this to you.
Comment on attachment 251334 [details] [review] view: add extra ensure_visibility and go_to functions I guess this is obsolete now with the other patch.
Created attachment 251545 [details] [review] view: add properties for animation mode and duration v5 I agree about the goto_with_duration But I think we will just remove the FIXME comment. The static go_to_with_duration is still in used to set a constant duration while scrolling it seems. Patch update does this. And also takes you up on your nitpick on the private variables. Thanks for review work!
Patch pushed with minor change to commit log to make it fit in 50 chars.
Looks good to me too, thanks for the patch!