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 660307 - patches animation.hg
patches animation.hg
Status: RESOLVED WONTFIX
Product: cluttermm
Classification: Other
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: cluttermm-maint
cluttermm-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-27 23:30 UTC by Ian Martin
Modified: 2014-04-15 07:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patches animation.hg (5.69 KB, patch)
2011-09-27 23:30 UTC, Ian Martin
needs-work Details | Review

Description Ian Martin 2011-09-27 23:30:01 UTC
Created attachment 197619 [details] [review]
patches animation.hg

animation.hg required a templated wrap for bind() and bind_interval.  Also added a refreturn to get_timeline 
a refreturn to get_alpha
added documentation for bind(), bind_interval(), and the class.
Comment 1 Murray Cumming 2014-03-14 09:51:03 UTC
Review of attachment 197619 [details] [review]:

Sorry for taking so long to even notice that this patch was here.

::: clutter/src/animation.hg
@@ +54,2 @@
   _WRAP_METHOD(void set_alpha(const Glib::RefPtr<Alpha>& alpha), clutter_animation_set_alpha)
+  _WRAP_METHOD(Glib::RefPtr<Alpha> get_alpha() const, clutter_animation_get_alpha, refreturn)

These should be in a separate patch ideally.

@@ +56,3 @@
   _WRAP_METHOD(void completed(), clutter_animation_completed)
+  _IGNORE(clutter_animation_bind, clutter_animation_bind_interval)
+ /** 

A blank line before the comment would be nice.

@@ +62,3 @@
+   * Given a property to bind, attaches an end value to it and 
+   * adds it to the animation.
+   * @param property_name : the recognised name for the property.

You don't need the : with @param.

@@ +66,3 @@
+   * is the programmers responsibility to ensure this is an 
+   * appropriate type.
+   * @return A RefPtr to the Animation.

We don't generally bother mentioning the "A RefPtr to the" part in documentation. That would get tedious quickly,

@@ +85,3 @@
+   Glib::RefPtr<Animation> bind_interval(
+                                 Glib::ustring property_name,
+                                 const ValueType& start_value,

ustring should be passed by const reference.

@@ +110,3 @@
 
+template <class ValueType>
+  Glib::RefPtr<Animation> Animation::bind(

In other places in gtkmm, we let gmmroc generate, for instance, a bind_value() to directly use the C function and then add a smaller bind<>() template method that calls it. That leaves less hand-written code in the template.

For instance, thought this uses Variant rather than Value:
https://git.gnome.org/browse/glibmm/tree/gio/src/action.hg#n238

I think I'd prefer that.

@@ +156,3 @@
+  Glib::RefPtr<Clutter::Animation> pWrapper = 
+                            Glib::RefPtr<Clutter::Animation> (this);
+  reference();

This reference() seems odd. It needs an explanatory comment at least.
Comment 2 Ian Martin 2014-03-14 10:07:30 UTC
OK, thanks.  I worked on this a good while back.  From memory Chris Kuhl was maintaining it back then, and I got onto something else before I could get it addressed.

 The animation framework ends up using a fair amount of templating, because in C the functions accept any value; in C++ the type of the property creates problems making a usable interface.  I don't know how best to address this, as the framework is too generalised- in particular, it's possible to animate boolean values.
I'm not working on this file any more; the animation API has all changed, and the Animation and Animator classes are deprecated for either easing functions within Actor or the Animatable interface.  However, I'll take the comments on board when I get to the classes that use it.
Comment 3 Murray Cumming 2014-04-15 07:44:10 UTC
Let's forget about this because ClutterAnimation is deprecated now anyway.