I am afraid I need some more time (1 or 2 days) to give you opinion about this animation refactorying work.

I am still collecting opinions and feedback but need more time to conclude them.

I will give you opinion as soon as possible.

I propose an unification of all animation flags in D10645 which should clarify things.
If you make this patch depend on D10645 (and rebase) you should be able to rename animation_object to just animation. How does that sound, @bu5hm4n @Jaehyun_Cho @zmike @cedric?

I share the collected opinions about refactoring of Canvas Animation as follows.

Let's define each approach as follows.

Strategy 1 : "Efl.Canvas.Object class provides animation control methods by using new mixin".
Strategy 2 : "Efl.Canvas.Animation class provides animation control methods".

1. From class design aspect
   • If class extension is required to be considered, Strategy 2 is recommended.
     • If new animation control methods are added?
     • If animation is applied to a certain class which does not inherit from Efl.Canvas.Object? (e.g. that certain class also needs to add animation control methods)
     • If a new class similar to animation is added and applied to Efl.Canvas.Object? (e.g. a new class whose methods look similar to animation control methods)
   • Otherwise, Strategy 1 would be fine.
     • But Efl.Ui.Animation_View's methods should be re-designed to distinguish from Efl.Canvas.Object's animation control methods.

2. From usability
   • If Strategy 1 is applied,
     • It is recommended to simplify @method animation_start because parameters speed and start_pos are not familiar to users.
     • To set a single Animation instance to multiple Efl.Canvas.Object's animation_start, Animation instance should not be owned by Efl.Canvas.Object instance.
   • If Strategy 2 is applied,
     • A single Animation instance should not be applied to multiple Efl.Canvas.Object instance. So method "Clone" is required to clone Animation instance with same properties.
     • If an Animation instance is called by animation_start of Efl.Canvas.Object while already it is called by other Efl.Canvas.Object's animation_start, then it might cause malfunction. (by changing target of Animation instance)
   • For both cases,
     • Animation class properties such as from_degree, to_degree may need to be set in constructor time. Otherwise, the change of animation properties of running animation might cause malfunction.
     • Animation instance is deleted by developer manually.

3. From performance / memory consumption aspect
   • If Strategy 1 is applied,
     • Efl.Canvas.Object's class table size is increased and it affects to the class table sizes of all classes related to Efl.Canvas.Object class.
   • If Strategy 2 is applied,
     • Efl.Canvas.Animation is required for each animation effect control. So that number of eo size is more consumed.
     • Strategy 2 consumes relatively more memory than Strategy 1 but the memory consumption does not look big enough.
   • For both cases,
     • Performances are the same.

Thanks for this analysis of the situation. @bu5hm4n and I have been talking about it and we agree on almost everything, but we'd like to make some clarifications:

1. Class design

• In Strategy 1, if new methods are needed, they can be added to the mixin, and all Canvas objects will automatically benefit from them.
• Why would anybody want to apply a Canvas.Animation to an object which is NOT a Canvas.Object? Can this work?
• Regarding Efl.Ui.Animation_View: This class has two different playbacks going on, the external Efl.Canvas.Object animation and the internal Lottie animation. Therefore, we need two sets of controls, which need to be clearly separated. All mixin methods are prefixed with animation_ whereas Animation_View methods have no prefix. I think this (plus documentation) should be enough to separate them.
• Additionally, as you already said, Efl.Ui.Animation_View needs some refactoring. It should implement the Efl.Player interface (and the Efl.Player interface can be extended with any missing functionality, like Sectors). This should also separate the two playback controls. Also, the word View is reserved for MVVM classes.
• Regarding the need to add another class with “methods that look similar to Animation”, I suppose you are talking about Animation_View. If another class similar to animation needs to be added, won’t it be more convenient to extend Canvas.Animation with some more methods instead?

2. Usability

• When providing an Animation object to a Canvas object, the caller only passes a reference to animation_start. So if N different callers are calling animation_start(efl_ref(animation_obj)), then everything will be fine.
• If the caller does not care about the Animation object anymore, it can pass its reference to animation_start(animation_obj) (without calling efl_ref). In this way when the animation finishes and removes its reference the Animation object will automatically be destroyed (because nobody else is holding references).
• Transferring all the animation properties to constructor properties, for convenience, has always been the plan.

3. Performance / memory consumption

• The performance of both Strategies is not equal. The method "clone", heavily used in Strategy 2, will create a lot of objects. Creation and destruction of these objects will cost time. For complex animations (multiple groups chained into each other) this is even going to be heavier. And due to the complexity of these animation objects (different types, tree-style hierarchy), this is not going to be easily optimizable.
• However, when we are going for the mixin, we can optimize the class size allocation (which will also improve quite a lot of other things).
• Moreover, the mixin only requires one additional pointer in the private data (All the data that is needed during the animation is stored only when animation is really happening). So we do not really have an overhead in private data.
• The extra methods required for Canvas objects in Strategy 2 are stored in the class vtable, so having more objects does not increase the amount of memory.

Given all the above, we think that Strategy 1 (the mixin) is the most convenient. Plus, it's already implemented :)

I think API-wise I have no more comments for this patch (pending the end of the discussion).

I think this is pretty fine like it is now, there are 2 more things that need to migrate away from player, but that does not block this revision here.

@segfaultxavi @bu5hm4n
Thank you very much for the comments! :)

In D10615#199204, @segfaultxavi wrote:

==== 1. Class design

• Why would anybody want to apply a Canvas.Animation to an object which is NOT a Canvas.Object? Can this work?

This comment makes sense because we named Efl.Canvas.Animation and we support animation to canvas object.

• Regarding Efl.Ui.Animation_View: This class has two different playbacks going on, the external Efl.Canvas.Object animation and the internal Lottie animation. Therefore, we need two sets of controls, which need to be clearly separated. All mixin methods are prefixed with animation_ whereas Animation_View methods have no prefix. I think this (plus documentation) should be enough to separate them.
• Additionally, as you already said, Efl.Ui.Animation_View needs some refactoring. It should implement the Efl.Player interface (and the Efl.Player interface can be extended with any missing functionality, like Sectors). This should also separate the two playback controls. Also, the word View is reserved for MVVM classes.

@jsuya @Hermet
Could you consider implementing Efl.Player interface to Efl.Ui.Animation_View? Or could you consider change the current APIs names of Efl.Ui.Animation_View?

• Regarding the need to add another class with “methods that look similar to Animation”, I suppose you are talking about Animation_View. If another class similar to animation needs to be added, won’t it be more convenient to extend Canvas.Animation with some more methods instead?

The example is Animation class and Animator class supported by View class in Android.
Animation class and Animator class are similar but they have distinct features each other. So the original comment was about introducing a new class which supports new animation features and requires different interface (i.e. properties and methods) from Canvas.Animation.
BTW, since we don't know the future demands, there is no absolutely right solution and we can just expect~ :)

==== 2. Usability

• When providing an Animation object to a Canvas object, the caller only passes a reference to animation_start. So if N different callers are calling animation_start(efl_ref(animation_obj)), then everything will be fine.
• If the caller does not care about the Animation object anymore, it can pass its reference to animation_start(animation_obj) (without calling efl_ref). In this way when the animation finishes and removes its reference the Animation object will automatically be destroyed (because nobody else is holding references).

The original collected feedback was also based on C# usages because now we focus on C# bindings very much.
So from C# usage, 'animation_start(efl_ref(animation_obj))' is not supported and not common way. C# users cannot understand this.
It seems that this is designed for the convenience of C users.
However, animation is more likely used multiple time. So usually the animation pointer (handle) is kept and re-used again. (even in C as well)
Therefore, I think we should let developers delete their animation instance by themselves like other widgets or objects.

• Transferring all the animation properties to constructor properties, for convenience, has always been the plan.

It's great! :)

==== 3. Performance / memory consumption

• The performance of both Strategies is not equal. The method "clone", heavily used in Strategy 2, will create a lot of objects. Creation and destruction of these objects will cost time. For complex animations (multiple groups chained into each other) this is even going to be heavier. And due to the complexity of these animation objects (different types, tree-style hierarchy), this is not going to be easily optimizable.

Good point about cloning complex animation objects. It makes sense.

Given all the above, we think that Strategy 1 (the mixin) is the most convenient. Plus, it's already implemented :)

"it's already implemented :)" This is really powerful and what I was scared most! ;)

Thank you very much for the comments and considerations! :)
Only one thing I need to ask you in Strategy 1.
Could you consider not automatically delete animation instance in animation_start? (The comments of 2. Usability)
As far as I know, there is no such way like "efl_ref" in C# and it seems that the auto deletion concept would not be easy to be accepted by C# users.

In D10615#199450, @Jaehyun_Cho wrote:

snip

In D10615#199204, @segfaultxavi wrote:

==== 2. Usability

• When providing an Animation object to a Canvas object, the caller only passes a reference to animation_start. So if N different callers are calling animation_start(efl_ref(animation_obj)), then everything will be fine.
• If the caller does not care about the Animation object anymore, it can pass its reference to animation_start(animation_obj) (without calling efl_ref). In this way when the animation finishes and removes its reference the Animation object will automatically be destroyed (because nobody else is holding references).

The original collected feedback was also based on C# usages because now we focus on C# bindings very much.
So from C# usage, 'animation_start(efl_ref(animation_obj))' is not supported and not common way. C# users cannot understand this.
It seems that this is designed for the convenience of C users.
However, animation is more likely used multiple time. So usually the animation pointer (handle) is kept and re-used again. (even in C as well)
Therefore, I think we should let developers delete their animation instance by themselves like other widgets or objects.

The C# should be something like (ignoring the other parameters for brevity):

var anim = new AnimObj();

widget.StartAnimation(anim);
anotherWidget.StartAnimation(anim);
yetAnotherWidget.StartAnimation(anim);
....

As StartAnimation receives their animation object by @move, the C# binding will automatically call efl_ref for it.

In D10615#199480, @lauromoura wrote:

As StartAnimation receives their animation object by @move, the C# binding will automatically call efl_ref for it.

@lauromoura
It would be a good idea! We need to check if it would be fine for all other @move cases. (e.g. @property manager of Efl.Ui.Spotlight.Container)
But I think it would be fine for other @move cases as well :)

In D10615#199492, @Jaehyun_Cho wrote:

In D10615#199480, @lauromoura wrote:

As StartAnimation receives their animation object by @move, the C# binding will automatically call efl_ref for it.

@lauromoura
It would be a good idea! We need to check if it would be fine for all other @move cases. (e.g. @property manager of Efl.Ui.Spotlight.Container)
But I think it would be fine for other @move cases as well :)

This is already done for all @move Efl.Object parameters, like setting the manager in the Container and setting the animation in this patch.

@segfaultxavi @bu5hm4n
I missed asking you one more thing in the original comment.

Is it OK if Efl.Canvas.Object provides animation_start with single parameter Efl.Canvas.Animation instance? i.e. animation_start(Efl.Canvas.Animation animation)

To cover pause and revert(reverse) animation with above, how about the following way?

Adds @method animation_revert(void) to Efl.Canvas.Animation

• If animation is already started, then this method pauses the animation and starts the animation in reverse way from the current progress.
• If animation is not started yet, then this method does nothing.

What do you think?

In D10615#199493, @lauromoura wrote:

This is already done for all @move Efl.Object parameters, like setting the manager in the Container and setting the animation in this patch.

Sorry I forgot that was already implemented. Thank you for the information! :)

In D10615#199494, @Jaehyun_Cho wrote:

Adds @method animation_revert(void) to Efl.Canvas.Animation

This would be very useful, but I think it is too specific. Being able to provide a speed and a starting point would allow to "revert" animations and many other things.

I am OK with adding animation_start(anim) if we also add animation_start_full(anim, speed, start_progress).
However, if we finish task T8474, we could have only one animation_start() method with default values, so in C# you could use StartAnimation(anim) or StartAnimation(anim, speed, starting_pos). I think this would be the nicest solution.

To revert an animation, in C#, you just need to do:

obj.StartAnimationFull(old_anim, -1.0, obj.AnimationProgress);

In D10615#199506, @segfaultxavi wrote:

However, if we finish task T8474, we could have only one animation_start() method with default values, so in C# you could use StartAnimation(anim) or StartAnimation(anim, speed, starting_pos). I think this would be the nicest solution.

To revert an animation, in C#, you just need to do:

obj.StartAnimationFull(old_anim, -1.0, obj.AnimationProgress);

If T8474 is completed, it would be wonderful!
Then, as you said, we should not separate animation_start(anim) at all.

Let's use animation_start(anim, speed, progress) as the current patch and let's discuss if each parameter is good enough or not to be used later. Because the patch is already completed!

I am going to merge this on the 20th of November, if there is no voice raised against this.