Page MenuHomePhabricator

Closed, ResolvedPublic


remove beta tag & release it officially.

Hermet created this task.Nov 17 2019, 11:14 PM
Hermet triaged this task as Normal priority.
bu5hm4n added a subscriber: bu5hm4n.Dec 4 2019, 7:51 AM

I think we had a discussion when this was introduced, that the API in here needs to implement the player interface etc.

jsuya edited subscribers, added: jsuya; removed: bu5hm4n.Dec 10 2019, 9:24 PM

@bu5hm4n You're right, I will make a patch soon to implement the Efl.Player interface.

jsuya added a comment.Dec 11 2019, 9:18 PM

Implements Efl.Player interface
Chnaged API
.speed.set => Efl.Player.playback_speed.set
.speed.get => Efl.Player.playback_speed.get
.progress.get => Efl.Player.playback_progress.get
.play => Efl.Player.playing.set(true)
.stop => Efl.Player.playing.set(false)
.pause => Efl.Player.paused.set(true)
.resume => Efl.Player.paused.set(false)
New feature API

segfaultxavi moved this task from Stabilized to Evaluating on the efl: api board.Dec 17 2019, 2:31 AM

I know we have discussed this before but I cannot find the ticket. Why is this called Animation_View when View is supposed to be reserved for MVVM widgets?

jsuya added a comment.Dec 18 2019, 3:32 AM

On other platforms that implement lottie animation, the class name is defined as AnimationView. This name definition follows it.

I think we can redefine Animation_view as AnimationView.
But I think, renaming the class is a big risk. Do you have a better idea?

You cannot compare EFL and Android / IOS here. "View" is like the basic building block in these Frameworks for any widget. View in EFL is something else.

  • Animation_Vector_Image
  • Animation_Image
  • Animation_Container

@segfaultxavi is most of the time better when it comes to names.

Well, being consistent with other toolkits is important, but being consistent with ourselves is more important :)
We have fought very hard in other areas to avoid using the word "View" when the widget was not related to MVVM.
Why do you think renaming the class is a big risk?

Let's think about a better name. This is the description of this class:

Animation view is designed to show and play animation of
vector graphics based content. It hides all @Efl.Canvas.Vg.Object details
but just open an API to read vector data from file. Also, it implements
details of animation control methods of Vector.

The current name clearly fails to mention "vector graphics". Why not Efl.Ui.Vg_Animation?
Or Efl.Ui.Vg, since the content does not need to be animated, right?

Another topic: This class has a method to report duration, which is already in the Efl.Playable interface. To keep consistency, this class should implement Efl.Playable.

jsuya added a comment.Dec 20 2019, 3:47 AM

I have made a patch that renames Efl.Ui.Animation_View to Efl.Ui.Vg_Animation. D10939
And I'll also implement Efl.Playable

jsuya added a comment.Dec 26 2019, 9:45 PM

I made a patch that implements Efl.Playerable. (D10953)
Please review this

zmike added a subscriber: zmike.Dec 30 2019, 8:35 AM

Current state with the D10870 series applied since I'm getting back into things:

class Efl.Ui.Vg_Animation
├ (P) autoplay
├ (P) autorepeat
├ (P) frame
├ (P) default_view_size
├ (P) state
├ (P) frame_count
├ (P) min_progress
├ (P) max_progress
├ (P) min_frame
├ (P) max_frame
├ (M) playing_sector
├ (M) value_provider_override
├ (E) play,start
├ (E) play,repeat
├ (E) play,done
├ (E) play,pause
├ (E) play,resume
├ (E) play,stop
├ (E) play,update

Some comments (and I don't mind doing work here to resolve them if we want to make changes):

  • Shouldn't Efl.Player have these signals? They seem like something every user of the interface would want.
  • Same with autoplay and autorepeat potentially?

Otherwise I think this looks good? Let's try to get this hashed out within the next week or so. Thanks @jsuya for addressing all the concerns so far!

Ugh, yeah, all these things belong to the Efl.Player interface :(

zmike added a subscriber: woohyun.Jan 6 2020, 7:04 AM

Waiting on @woohyun or @jsuya to comment on this...

jsuya added a comment.Jan 8 2020, 5:05 PM

I made testcases(D11022) related to this. and I have to update it.
I had a small problem with my development environment. So I couldn't update tc patch.
anyway I will update and confirm it as soon as possible.

zmike added a comment.Jan 9 2020, 5:16 AM

@jsuya We're specifically waiting on feedback regarding the namespacing issue that I cited with the events and a couple methods which seem like they should be merged down into efl.player. Do you have any comments on this?

jsuya added a comment.Jan 9 2020, 9:59 PM

Hi @zmike

├ (E) play,start
├ (E) play,done
├ (E) play,pause
├ (E) play,resume
├ (E) play,stop
├ (E) play,update

├ (E) play,repeat

We can move the event to the Efl.Player as you say. But for this efl_canvas_video(emotion), elm_image and elm_image_zoomable must also be modified.
Do you want to implement this work in this task?


autoplay and autorepeat are different behaviors. It is described in docs
In some ways, however, the two names can confuse the same behavior.
(maybe people can think autoplay is to play again automatically when it's done. Is that correct?)

Do you want to change name to autoplaystart or autostart?

Regarding possible autoplay - autorepeat confusions:
I think autoplay is clear enough, and a common way of referring to this feature.
We can rename autorepeat to looping or repeating. This does not clash with anything else in our current API.

jsuya added a comment.Jan 10 2020, 1:15 AM

thank you for comments. I made D11061

zmike added a comment.Jan 13 2020, 6:37 AM

My point regarding autoplay/autorepeat was that they could potentially be moved to efl.player, not that they could be combined.

I've done the work to remove the events from this and put them in efl.player, so this is taken care of. We can independently evaluate the implementation of those events in existing efl.player interface users once we stabilize those events.

My remaining question is whether it's useful to have autoplay and autoplay/looping (which should actually maybe be playback_loop for consistency) as interface properties in efl.player. Looking at existing users (image, video, animation), I think autoplay would definitely be useful in all three. autorepeat is likely to be useful in image at a minimum for gif playback as well as animation, but I'm not sure I can see it being used in video at all?

I'm OK with stating in the video docs that autorepeat is ignored. Then again, it does not look very hard to implement either...

Do we have any plans to move and implement autoplay and looping to Efl.Player?
Is there anything else I need to change in Efl.Ui.Vg_Animation before merging D10870?

zmike added a comment.Jan 17 2020, 6:22 AM

Moving to efl.player is not very difficult, but we do not yet have an agreement about whether it would be useful to do so. We need opinions!

zmike added a comment.Jan 21 2020, 9:49 AM

To be clear, I don't see an issue with marking this stable for the release, but I want to avoid setting the precedent of marking things stable in the tree and then continuing to do lots of work on them, as this could easily spiral out of control and lead to us releasing incomplete API.

I think @zmike and I have already said that it seems useful to move autoplay to Efl.Player, and that it shouldn't be difficult to move looping too.
Nobody said anything against this idea, so I guess that's the way forward.

Also, if looping is moved to Efl.Player, I agree with @zmike that it should be renamed to playback_loop, for consistency with the other properties there.

I made that move autoplay and looping to Efl.Player and implements in Efl.Ui.Vg_animation.

but I didn't implements it in Efl.Ui.Image and Efl.Ui.Image_Zoomable( set @empty marker). I expect someone to implement this.
(I don't think this has anything to do with releasing a vector animation widget. I hope it will be done in a new task.)

zmike added a comment.Jan 28 2020, 6:32 AM

Great, thanks!

I created T8589 which can be completed separately.

class Efl.Ui.Vg_Animation
├ (P) frame
├ (P) default_view_size
├ (P) state
├ (P) frame_count
├ (P) min_progress
├ (P) max_progress
├ (P) min_frame
├ (P) max_frame
├ (M) playing_sector
├ (M) value_provider_override

This is the current state. I think this is reasonable?

I don't feel like making more objections :)

Nothing from my side either.

I merged a patch to remove beta. (D10870). Thank you for helping to make this task work well.

bu5hm4n closed this task as Resolved.Jan 31 2020, 5:15 AM

So this here is done i guess.