Page MenuHomePhabricator

evas: make efl_canvas_animation abstract
ClosedPublic

Authored by bu5hm4n on Jan 11 2019, 5:35 AM.

Details

Summary

it seems that this class does not have a meaning when created just like
this. Other classes using it are even abstract, which means, this class
should also be abstract. This is done in order to support that a
abstract class should only contain abstract

ref T7240

Depends on D7600

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n created this revision.Jan 11 2019, 5:35 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

bu5hm4n requested review of this revision.Jan 11 2019, 5:35 AM
segfaultxavi accepted this revision.Jan 11 2019, 8:22 AM

If it builds and examples can be executed (I haven't tested) means that this change is safe enough, right?

This revision is now accepted and ready to land.Jan 11 2019, 8:22 AM
cedric accepted this revision.Jan 11 2019, 10:35 AM

Make sense indeed.

bu5hm4n updated this revision to Diff 18408.Jan 14 2019, 1:23 PM
bu5hm4n edited the summary of this revision. (Show Details)

rebase.

Closed by commit rEFLea9ff9f547b5: evas: make efl_canvas_animation abstract (authored by bu5hm4n, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyJan 15 2019, 8:38 AM
This revision was automatically updated to reflect the committed changes.
Jaehyun_Cho added a comment.EditedJan 29 2019, 4:14 AM

@bu5hm4n @segfaultxavi @cedric

As you said, Efl.Canvas.Animation does not have its own animation properties (e.g. scale, alpha, etc.)
However, with Efl.Canvas.Animation_Player, Efl.Canvas.Animation can be used as a custom animation.
e.g.

void _anim_running_cb(void *data, const Efl_Event *event)
{
   Efl_Canvas_Animation_Player_Event_Running *event_running = event->info;
   double progress = event_running->progress;
   //do custom animation
}

{
   ...
   Efl_Canvas_Animation *anim = efl_add(EFL_CANVAS_ANIMATION_CLASS, win);
   ...

   Eo *player = efl_add(EFL_CANVAS_ANIMATION_PLAYER_CLASS, win,
                                   efl_animation_player_target_set(efl_added, btn),
                                   efl_animation_player_animation_set(efl_added, anim));
   efl_event_callback_add(player, EFL_ANIMATION_PLAYER_EVENT_RUNNING, _anim_running_cb, NULL);
   ...
}

Moreover, as you know, it is not a problem that abstract class inherits from regular class.

Although Efl.Canvas.Animation_Scale can do custom animation, the class name does not sound good for custom animation.

What do you think about making Efl.Canvas.Animation a regular class?

@Jaehyun_Cho I do not understand the example... you are not instantiating any EFL_CANVAS_ANIMATION, right?
That example cannot work if Efl.Canvas.Animation is made abstract?

@Jaehyun_Cho I do not understand the example... you are not instantiating any EFL_CANVAS_ANIMATION, right?
That example cannot work if Efl.Canvas.Animation is made abstract?

The example are using child class of Efl.Canvas.Animation, aren't they?

@segfaultxavi @cedric
Sorry, my example was incorrect as @cedric said.

...
Efl_Canvas_Animation *anim = efl_add(EFL_CANVAS_ANIMATION_CLASS, win);
...

We can use Efl.Canvas.Animation class for custom animation with Efl.Player class' event callbacks.
In short, Efl.Canvas.Animation can be used for custom animation so I think the Efl.Canvas.Animation is deserved to be a regular class instead of abstract class.

@segfaultxavi @cedric @bu5hm4n
What do you think about changing Efl.Canvas.Animation to be a regular class?

@Jaehyun_Cho Okay - now the example makes a lot more sense. The patch was primary done to support that all parents of abstract classes are abstract classes, i don't mind a revert, since this issue turned up to be no issue. Do you push a revert of this patch ?

@bu5hm4n
Thank you for the opinion.
If @segfaultxavi and @cedric do not disagree, then I will push a revert of this patch.

Yeah, revert the patch, and thanks for the explanation!

I've submitted the revert patch. (f23f3074daa3e9da5b37cc168df5d6b003e0ec62)
Thank you all~

Thank you! :)