Page MenuHomePhabricator

all the uses of efl_animation should use a parent object that traces back to the mainloop
Closed, ResolvedPublic

Description

this affects at LEAST:

src/lib/evas/canvas/efl_animation*.c
src/bin/elementary/test_efl_anim*.c

all the efl_add()'s here use a NULL parent. this is "wrong" for efl land. this should be a parent that is in the tree that works back to the loop object (the main loop object). so parent should be the main loop object returned by efl_main_loop_get(), or even better should be a widget or a window object etc. - something visible that eventually has the main loop as a parent. if it's a window then the animation system can in future work out the correct animation framerate based on which display the window is on (if it spans 2 displays... well... we're in trouble. so we'll just pick one). either way... NULL as a parent is wrong. so try and use the window or parent widget if available first, and if not, then use the main loop. you can make animation objects that are children of other animation objects... keep that in mind.

raster created this task.Jan 3 2018, 9:30 PM
Jaehyun_Cho added a subscriber: jpeg.

@raster

Thanks for your comment. I understand your point.
I have a question about your comment.
What if application does not set animation's parent in efl_add()? If so, should efl_animation set efl_main_loop_get() as its parent internally?

raster added a comment.Jan 4 2018, 7:50 AM

IMHO if parent isn't used then its an error. realistically IMHO even construction should fail. so the finalize method might have to at least check? as i mentioned in an email today, this is something we need for eo/eolian - extra tagging/properties that indicate an object MUSt have a parent than traces back to the mainloop (most objects need this).

Jaehyun_Cho added a subscriber: Jaehyun_Cho.

This task is done by the taxi2se's commit 588995da317520fc92660b89b0864bb1f25abc09
Thank you taxi2se :)

jpeg added a comment.Jan 16 2018, 8:53 PM

Okay then please close this if your saying is correct.
@taxi2se also please address Dave's concerns on the ML.

I don't think this is fixed. i still see:

interp = efl_add(EFL_INTERPOLATOR_LINEAR_CLASS, NULL);

and friends in src/bin/elementary/test_efl_anim_interpolator.c. src/bin/elementary/test_efl_anim_scale.c too. related to animation anyway. part of the same work.

@raster
sorry I miss it. I will take care of it.

jpeg added a comment.Jan 16 2018, 9:35 PM

you know what? make sure animation objects can't be created without a parent (try to find the evas from this parent. if null, then finalize or constructor should return null) - also change the test cases also.

for now ... i think it'd be bad to do this in every class manually. it really should be done globally for eo ... but we need to first be able to have warnings and then fix them all i think, as well as somehow implement exceptions for the few object classes like loop that can have a null parent. i think it'd be too early for this. just grep -r efl's tree for efl_add with NULL will point out a lot of the examples :)

For now, I've submitted a patch to call all efl_add with parent in elementary_test. (119c41d7f2f13da0e27ee0d4d879301aa36f90c3)

jpeg added a comment.Jan 17 2018, 2:02 AM

It's not really a big task. It's already the case for all things related to evas as evas object constructor looks for the evas.
Though you are perfectly right that a proper hint in EO would make sense.

bu5hm4n added a project: Restricted Project.Jun 10 2018, 12:29 PM
zmike edited projects, added Restricted Project; removed efl.Jun 11 2018, 6:51 AM
bu5hm4n edited projects, added efl: main loop; removed Restricted Project, Restricted Project.Jun 11 2018, 9:27 AM
Jaehyun_Cho closed this task as Resolved.Jun 13 2018, 9:56 PM

This has been resolved.

The parent parameter of all efl_add() calls in efl animation are properly set instead of NULL.