Page MenuHomePhabricator

efl_ui_animation_view: introduce a new widget for controlling vector animation.
ClosedPublic

Authored by Hermet on Tue, Jul 30, 11:57 PM.

Details

Summary

This is a new convenient ui control that designed to load vector resources
-svg, json-, and control animations if it supports.

Please turn on evas-loaders-json in meson options,
if you'd like to use Lottie Animation.

Co-authored-by: JunsuChoi <jsuya.choi@samsung.com>

@feature

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.
Hermet created this revision.Tue, Jul 30, 11:57 PM

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/

Hermet requested review of this revision.Tue, Jul 30, 11:57 PM
Hermet updated this revision to Diff 23781.Wed, Jul 31, 12:00 AM

remove json build option.

will add sample and test code patches.
any feedback welcomed.

bu5hm4n added inline comments.
src/lib/elementary/efl_ui_animation_view.c
278

I do not like that a new widget uses old widgets inside, shouldn't this use some animator object from the efl_* namespace ?

318

I think this should check if there was an error.

605

Other calls to this object are done with efl API, can all of them be done with efl_ prefixed APIs ? :)

src/lib/elementary/efl_ui_animation_view.eo
12

This object seems to implement Efl.Gfx.View, can you add this object to the spec test suite for Efl.Gfx.View?

12

In general, there are interfaces like efl_player or efl_playable, where i think those should be implemented here.

202

Can you write what the difference to the size of the object is, I do not understand that right now :)

src/lib/elementary/efl_ui_animation_view_private.h
12

I have to say, i do not like self objects in the private data.

32

I would use "efl_data_scope_safe_get()" here, otherwise you would need to check the type.

Hermet planned changes to this revision.Thu, Aug 1, 4:00 AM
Hermet added inline comments.
src/lib/elementary/efl_ui_animation_view.c
278

When this widget was implemented, efl interfaces were unstable, even now.
Transit functionality is much mature than it.

Maybe we could change it, once it's really stable and useful. But I think it's not the mandatory point right moment,

318

let me check.

605

ok,

src/lib/elementary/efl_ui_animation_view.eo
12

let me check.

12

I don't think so. When I looked it, efl_player interfaces totally depends on the video player interface.

202

like image resolution, vector resources could be specified by designers.
This returns the size value.

src/lib/elementary/efl_ui_animation_view_private.h
12

If it needs to frequently accesss,
why not??

32

ok, let me see.

Hermet added inline comments.Thu, Aug 1, 4:08 AM
src/lib/elementary/efl_ui_animation_view_private.h
12

Come to think of it,
This vg is not the self object(Animation_View) but separate efl_canvas_vg_object.

Hermet updated this revision to Diff 23868.Fri, Aug 2, 1:27 AM

updated

Hermet added inline comments.Fri, Aug 2, 1:28 AM
src/lib/elementary/efl_ui_animation_view.c
318

updated

605

updated.

src/lib/elementary/efl_ui_animation_view.eo
12

Could you please elaborate how I could add spec test suite for Efl.Gfx.View?

202

renamed it to "default_view_size".

src/lib/elementary/efl_ui_animation_view_private.h
32

updated

zmike requested changes to this revision.Fri, Aug 2, 5:53 AM
zmike added a subscriber: zmike.

Why does this have a legacy API? I don't think we should be adding more legacy API at this point...

Also, you shouldn't use @since in the eo docs considering this widget is not stable, and thus the API is not "released".

This revision now requires changes to proceed.Fri, Aug 2, 5:53 AM
In D9451#175836, @zmike wrote:

Why does this have a legacy API? I don't think we should be adding more legacy API at this point...

I think still we should take care of Legacy and Interfaces both, if many legacy apps(exisiting apps) want it and requires keeping using legacy world.

Also, you shouldn't use @since in the eo docs considering this widget is not stable, and thus the API is not "released".

Well, I don't think it's the matter because the interface is tagged with beta, we could update the since date later (Or if you argue then we can remove it, Do you like it?). 
In point of legacy API, those apis could be released officially regardless of interface. The widget legacy apis actually used earlier in Tizen a year ago with lottie feature, I think we could target to release this in 1.23, if it's stable enough. maybe we can make it sure before.
Hermet requested review of this revision.Sun, Aug 4, 11:08 PM

Updated comments.

bu5hm4n requested changes to this revision.Mon, Aug 5, 1:16 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_ui_animation_view.c
320

This does not seem correct, it returns false when there is an error. I do not think you want to return an error when it was successfully loaded ?

src/lib/elementary/efl_ui_animation_view.eo
12
12

efl_player.eo has every function that is defined here (sometimes they are called a little bit different, but i do not think that the naming is an issue)

Additionally, this here is beside video the only real user of the "media player interface" if it does not fit the usecase, then the interface needs to be adjusted. But IMO this should definitily implement this interface.

src/lib/elementary/efl_ui_animation_view_private.h
12

The comment moved, because a line at the top was removed. The original comment here was about the "obj" field.

I do not see any value in adding obj to the private field, it is actually quite dangerous. If you get the private data from a object, it will be NULL if the object is deleted / wrong etc. if you just have the pd pointer, you cannot check that, and you will likely just access it and crash. So from my POV we should establish a "best practice" here, where we always pass the object alone (if there is no possibility to pass obj and pd next to each other) and get the pd from it, after that, NULL-check pd, and *then* access it. This makes our code a lot more safer, and in case a callback is leaked and called after the object died, you will get just an error instead of an crash.

tldr; having a field like "obj" in private data is dangerous, lets not do that.

This revision now requires changes to proceed.Mon, Aug 5, 1:16 AM
bu5hm4n added inline comments.Mon, Aug 5, 1:29 AM
src/lib/elementary/Elementary.h
354

Please include this in efl_ui.h

zmike resigned from this revision.Mon, Aug 5, 5:59 AM
In D9451#175836, @zmike wrote:

Why does this have a legacy API? I don't think we should be adding more legacy API at this point...

I think still we should take care of Legacy and Interfaces both, if many legacy apps(exisiting apps) want it and requires keeping using legacy world.

Also, you shouldn't use @since in the eo docs considering this widget is not stable, and thus the API is not "released".

Well, I don't think it's the matter because the interface is tagged with beta, we could update the since date later (Or if you argue then we can remove it, Do you like it?). 
In point of legacy API, those apis could be released officially regardless of interface. The widget legacy apis actually used earlier in Tizen a year ago with lottie feature, I think we could target to release this in 1.23, if it's stable enough. maybe we can make it sure before.

I'm not sure that reviewing legacy API is the best use of time when we're trying to move quickly on unified API, which is why I brought this up. I think we should be focusing on unified APIs now and trying to encourage people to try building apps without legacy wherever possible. Adding more legacy API like this just adds more codepaths to the repo which will likely never be used or adequately tested.

I don't think either of the points I raised are blockers for this patch, I just wanted to make sure we were aware of what we were doing here.

Hermet added a comment.Mon, Aug 5, 7:05 PM
In D9451#176006, @zmike wrote:
In D9451#175836, @zmike wrote:

Why does this have a legacy API? I don't think we should be adding more legacy API at this point...

I think still we should take care of Legacy and Interfaces both, if many legacy apps(exisiting apps) want it and requires keeping using legacy world.

Also, you shouldn't use @since in the eo docs considering this widget is not stable, and thus the API is not "released".

Well, I don't think it's the matter because the interface is tagged with beta, we could update the since date later (Or if you argue then we can remove it, Do you like it?). 
In point of legacy API, those apis could be released officially regardless of interface. The widget legacy apis actually used earlier in Tizen a year ago with lottie feature, I think we could target to release this in 1.23, if it's stable enough. maybe we can make it sure before.

I'm not sure that reviewing legacy API is the best use of time when we're trying to move quickly on unified API, which is why I brought this up. I think we should be focusing on unified APIs now and trying to encourage people to try building apps without legacy wherever possible. Adding more legacy API like this just adds more codepaths to the repo which will likely never be used or adequately tested.

I don't think either of the points I raised are blockers for this patch, I just wanted to make sure we were aware of what we were doing here.

I got your point, I agree on avoiding adding legacy stuff further but you must know this, this animation_view has been used in tizen since 1 year ago. As we think of interfaces progress back, it was very unstable and fragile.
In last year, there was no guarantee that when interfaces would be released officially, If I added only interfaces without legacy for this animation_view, who'd like to use it?
They had to wait this simple feature for more than year. Even now it must be in beta. Nobody could use it, it might bring them give up of this.

Supporting Legacy is not mandatory but we could take care of legacy if it's possible unless Interface is out to go officially.

Hermet added a comment.Mon, Aug 5, 7:05 PM

added comment.

Hermet added inline comments.Mon, Aug 5, 9:27 PM
src/lib/elementary/Elementary.h
354

eh? where is efl_ui.h? i can't find it.

src/lib/elementary/efl_ui_animation_view.c
320

oh, i mistook, let me correct it.

src/lib/elementary/efl_ui_animation_view.eo
12

If you argue efl_animation_view should implement efl_player,
efl_player should be split two interfaces -efl_player and efl_media_player- then efl_media_player should take care media stuff such as volume control functions. or it should be unified with efl_ui_video.

This should be prerequisite and must be an another task to be progressed separately alongside with this animation_view.
If once that's done, this could implement efl_ui_animation additionally. No plan for this yet, so I think this is out of this patch.

But before that, I popped a question through my mind.
What is the guaranteed pros if user derive efl_player or efl_media_player or in this case?

If I answer myself, it just for consistent naming. nothing more. very stupid. means, efl_player interface is abused.

Currently, efl_ui_image proves this, since it inherits efl_player though it couldn't satisfy volume stuff.
Nowhere triggers theses interfaces in promise. In oop world, conceptually this is bad symptom.

Actually, efl_player has no any cases in working inside efl frameworks by any promise triggers with other classes.
We just follow efl_player interface, but has no reason.
It just make user have tight usage with strong limitation without any reasons.

src/lib/elementary/efl_ui_animation_view_private.h
12

I got your point. I would change this for keeping our efl rule.

But you should know this, Crashing app and breaking app are both matters. (here broken apps means the app which has unexpected sequence by something wrong)

Or maybe broken apps would be worse than just stopping apps immediately by crashing because crashing is very obvious to detect the issue and find it eailer, debug it easily.
just avoid issue by checking null or invalid is very dangerous since it just mimic apps look working fine and hide the potential issues.

First of all, if the object is invalid, apis/event won't work properly, accessing internal data (obj in private data) own't be reached at all. it shouldn't be happened.
Secondly, as the event data, passing object is necessary anyway. somewhere the object must be kept for animation.

Though i will change this, but i don't see any coming practical safe in this case. We only need to prevent abused obejct access by users(apps), not internal.

Hermet updated this revision to Diff 23915.Mon, Aug 5, 9:56 PM

updated code as to follow review comments.

Hermet updated this revision to Diff 23916.Mon, Aug 5, 10:48 PM

added efl_ui_animation_view.h in Efl_Ui.h

Hermet added inline comments.Mon, Aug 5, 10:49 PM
src/lib/elementary/Elementary.h
354

added.

Hermet updated this revision to Diff 23917.Mon, Aug 5, 10:59 PM

added spec test.

Hermet updated this revision to Diff 23918.Mon, Aug 5, 11:02 PM

updated

I think the overall widget looks fine beside the fact that elm_transit is used (which is IMO not good, but i might be able to life with it :P~). However, the other topic is Efl.Player: Volume and mute can just be generally ignored and return always 0 and true. The more important part here is, that we have a unified interface for playable entities. If they are splitted, what is the difference between this play here and the play in efl_player (same applies to all the other properties)? What are we going to do to not have colliding names ?
In general, if we do not have a common ground, we cannot build up specification tests for it - we cannot build up helper classes, we are super limited in future actions we can take.

Hermet added a comment.Tue, Aug 6, 8:39 PM

I think the overall widget looks fine beside the fact that elm_transit is used (which is IMO not good, but i might be able to life with it :P~). However, the other topic is Efl.Player: Volume and mute can just be generally ignored and return always 0 and true. The more important part here is, that we have a unified interface for playable entities. If they are splitted, what is the difference between this play here and the play in efl_player (same applies to all the other properties)? What are we going to do to not have colliding names ?
In general, if we do not have a common ground, we cannot build up specification tests for it - we cannot build up helper classes, we are super limited in future actions we can take.

If you argue volume and mute could be generally ignored, why it has to be specified in the base interface ? It should not be mandatory. it should be removed there. The efl_ui_multimedia_player which implements this player interface could add extra own volume and mute control features.
We don't need to give insane interfaces to many other animation/playerable ui controllers which doesn't fit with sound controls.

Secondly, there is no any reasonable methods avoid colliding player concept. We couldn't stop users if they break our naming rules, behaviors and so on because there is no any use-case which this player works in integrated with other components(specifically, animation frameworks) as the interface in our engine and frameworks.
That's why Animation_View could successfully built up without this player interface. It doesn't harm other components nor break any compatibility.

I feel canvas animation and player interfaces went wrong way. These should be re-designed from the scratch, there had to be specification for working animation framework and player together.
If so, there must be a reason that user would like to use these basic interfaces for custom animation and playing animation controls stuff.

I think the overall widget looks fine beside the fact that elm_transit is used (which is IMO not good, but i might be able to life with it :P~). However, the other topic is Efl.Player: Volume and mute can just be generally ignored and return always 0 and true. The more important part here is, that we have a unified interface for playable entities. If they are splitted, what is the difference between this play here and the play in efl_player (same applies to all the other properties)? What are we going to do to not have colliding names ?
In general, if we do not have a common ground, we cannot build up specification tests for it - we cannot build up helper classes, we are super limited in future actions we can take.

Currently our efl player has no any specification for behaviors as the interface.
If you convince your idea that this must implement efl.player, then please show me any practical case why this efl.player must have spec, or you can use any showcases in android, ios what other platforms does this similar case.

In my experience for the oop development, efl.player interface shouldn't for this case.
If it's necessary, the player design is up to users idea and that player must have designed to have couple with efl.frame_controller. That's all.

for you information, this is what animation_view in android version, you can refer,
https://github.com/airbnb/lottie-android/blob/master/lottie/src/main/java/com/airbnb/lottie/LottieAnimationView.java

and ios as well.
https://github.com/airbnb/lottie-ios/blob/master/lottie-swift/src/Public/Animation/AnimationView.swift

Hermet updated this revision to Diff 24051.Tue, Aug 13, 1:37 AM

updated some doc syntax for c# generation.

Hermet updated this revision to Diff 24052.Tue, Aug 13, 1:49 AM

updated some doc syntax for c# generation.

@bu5hm4n Im considering use player but currentl efl_player is not satisfied with this animation_view, it might need to some adjustment but moement it has a dependencies with other components,
I don't like to change all of them and it needs some time to proper review,
Since all were in beta, we could improve them step by step , So I think you could verify this, then I will improve them from this.

bu5hm4n accepted this revision.Tue, Aug 20, 5:03 AM

Sure - that sounds good :) Thank you :)

This revision is now accepted and ready to land.Tue, Aug 20, 5:03 AM
This revision was automatically updated to reflect the committed changes.