Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Carsten Haitzler]

On Wed, 18 Apr 2018 12:38:46 -0400 Cedric Bail  said:

> On April 18, 2018 12:48 AM, Carsten Haitzler  wrote:
> > On Tue, 17 Apr 2018 19:33:02 -0400 Cedric Bail ced...@ddlm.me said:
> > > On March 29, 2018 10:58 AM, Carsten Haitzler ras...@rasterman.com wrote:
> > > > On Thu, 29 Mar 2018 13:17:43 -0400 Cedric Bail ced...@ddlm.me said:
> > > > > On March 28, 2018 8:06 PM, Carsten Haitzler ras...@rasterman.com
> > > > > wrote:
> > > > > > On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me said:
> > > > > > > On March 27, 2018 10:32 PM, Carsten Haitzler ras...@rasterman.com
> > > > > > > wrote:
> > > > > > > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL
> > > > > > > > cedric.b...@free.fr said:
> > > > > > > > 
> > > > > > > > Ideally yes, but at least for the _example, it requires to
> > > > > > > > spend some more time to move them to use EFL_MAIN, which should
> > > > > > > > be done in a different patch not related to fixing the usage of
> > > > > > > > efl_add.
> > > > > > 
> > > > > > some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but
> > > > > > moving to efl_add_ref() isn't the "right direction" for most of
> > > > > > these.
> > > > > 
> > > > > Yeah, I haven't been a big fan of efl_main_loop_get and did tend to
> > > > > avoid it. I should use it more often in the future.
> > > > 
> > > > it'd be good to stop doing that... :) long term at least. the slow and
> > > > steady steps are to fix our parenting in efl and examples. :) i fixed it
> > > > for you - or well i fixed everything i saw you changed. it may not be
> > > > perfect, but it's a step in the right direction imho :) point out if i
> > > > got something wrong. i didn't do "Extensive" work like you say and
> > > > convert to EFL_MAIN or within efl modify code to pass parents around
> > > > many levels down or something.
> > > 
> > > So I have been working on finishing correcting the lifecycle of eo object
> > > by making sure that efl_invalidate does happen before any efl_destructor
> > > code (Currently in master, it is possible to get efl_invalidate to be
> > > called after the object destructor as it is called inside
> > > efl.object.destructor). This means that efl_del and efl_parent_{get,set}
> > > are a typical wrong piece of code in the destructore. The patch
> > > 2fb5cc3ad09f6aaf82b5d1131ac5ed22ed848bd4 is wrong in a lot of place that
> > > I had to give up fixing it due to time constraint (basically it assume
> > > our use before invalidate was efl_add/efl_del, while the closest to our
> > > pattern was efl_add_ref with efl_unref in the desstructor). I have a
> > > branch devs/cedric/lifecycle with all the change in it including a revert
> > > of your patch. I can't land this patch at the moment as the old eo base
> > > efl_future lifecycle is a complete wreck with this change, so I will
> > > focus back on removing efl_future. If you had time in the mean time to
> > > check your patch and see how to fix it, it would be great.
> > 
> > wait wait. first. some objects. actually a fair few objects need to know
> > their parents to find their loop provider etc. an they need to do this on
> > "shut down" to delete stuff etc. from here. so my question will be:
> >
> > where should an object do this "shut down" in the invalidate or in
> > destructor. you seem to say that it mys be in invalidate which means
> > invalidate effectifely is the "meat" of the destructor, just the object
> > keeps itself around with some minimal content until destructor is called.
> > so what you are doing is ensuring there is ALWAYS the sequence of
> > invalidate then destroy, right?
> 
> So yes, invalidate should always happen before the destructor.
> 
> > so i'm not sure what's wrong with that patch - can you detail it? i simply
> > moved to having a parent instead of no parent in many cases, and used
> > efl_del again like it used to be which should clear out the only reference
> > to the object by unparenting it etc. etc. ...
> 
> The problem is that efl_del in a destructor will always be either wrong or
> bad. Wrong as in you are not the parent/owner of that reference and are

wait a sec... at least at the time of the patches in question here, efl was
still basically all using destructor only. no invalidate existed (or was used).
it's still basically not used except for canvas objects, vg node objects, base
class, smart layout objects, model containers and efl app. so let's say its
essentially not used. the invalidate stuff was put into the efl tree after the
patches in question here, so at least my partial revert of your changes (back
to using parents and efl del) was correct at the time, or as correct as was
possible. is this correct? was there anything wrong at that stage?

now calling efl_del in a destructor... i am unsure why it would often or always
be wrong? if you are going to manually destroy some object (a child or some
other linked object). at least in a world where invalidate 

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Cedric Bail]

On April 18, 2018 12:48 AM, Carsten Haitzler  wrote:
> On Tue, 17 Apr 2018 19:33:02 -0400 Cedric Bail ced...@ddlm.me said:
> > On March 29, 2018 10:58 AM, Carsten Haitzler ras...@rasterman.com wrote:
> > > On Thu, 29 Mar 2018 13:17:43 -0400 Cedric Bail ced...@ddlm.me said:
> > > > On March 28, 2018 8:06 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > > > On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me said:
> > > > > > On March 27, 2018 10:32 PM, Carsten Haitzler ras...@rasterman.com
> > > > > > wrote:
> > > > > > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr
> > > > > > > said:
> > > > > > > 
> > > > > > > Ideally yes, but at least for the _example, it requires to spend 
> > > > > > > some
> > > > > > > more time to move them to use EFL_MAIN, which should be done in a
> > > > > > > different patch not related to fixing the usage of efl_add.
> > > > > 
> > > > > some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but
> > > > > moving to efl_add_ref() isn't the "right direction" for most of these.
> > > > 
> > > > Yeah, I haven't been a big fan of efl_main_loop_get and did tend to 
> > > > avoid
> > > > it. I should use it more often in the future.
> > > 
> > > it'd be good to stop doing that... :) long term at least. the slow and
> > > steady steps are to fix our parenting in efl and examples. :) i fixed it
> > > for you - or well i fixed everything i saw you changed. it may not be
> > > perfect, but it's a step in the right direction imho :) point out if i got
> > > something wrong. i didn't do "Extensive" work like you say and convert to
> > > EFL_MAIN or within efl modify code to pass parents around many levels down
> > > or something.
> > 
> > So I have been working on finishing correcting the lifecycle of eo object by
> > making sure that efl_invalidate does happen before any efl_destructor code
> > (Currently in master, it is possible to get efl_invalidate to be called 
> > after
> > the object destructor as it is called inside efl.object.destructor). This
> > means that efl_del and efl_parent_{get,set} are a typical wrong piece of 
> > code
> > in the destructore. The patch 2fb5cc3ad09f6aaf82b5d1131ac5ed22ed848bd4 is
> > wrong in a lot of place that I had to give up fixing it due to time
> > constraint (basically it assume our use before invalidate was
> > efl_add/efl_del, while the closest to our pattern was efl_add_ref with
> > efl_unref in the desstructor). I have a branch devs/cedric/lifecycle with 
> > all
> > the change in it including a revert of your patch. I can't land this patch 
> > at
> > the moment as the old eo base efl_future lifecycle is a complete wreck with
> > this change, so I will focus back on removing efl_future. If you had time in
> > the mean time to check your patch and see how to fix it, it would be great.
> 
> wait wait. first. some objects. actually a fair few objects need to know their
> parents to find their loop provider etc. an they need to do this on "shut 
> down"
> to delete stuff etc. from here. so my question will be:
>
> where should an object do this "shut down" in the invalidate or in destructor.
> you seem to say that it mys be in invalidate which means invalidate 
> effectifely
> is the "meat" of the destructor, just the object keeps itself around with some
> minimal content until destructor is called. so what you are doing is ensuring
> there is ALWAYS the sequence of invalidate then destroy, right?

So yes, invalidate should always happen before the destructor.

> so i'm not sure what's wrong with that patch - can you detail it? i simply
> moved to having a parent instead of no parent in many cases, and used efl_del
> again like it used to be which should clear out the only reference to the
> object by unparenting it etc. etc. ...

The problem is that efl_del in a destructor will always be either wrong or bad. 
Wrong as in you are not the parent/owner of that reference and are trying to 
destroy it manually during destructor. Bad as in the child object has already 
lost its parent during the invalidate call on the parent, and if it is the only 
reference you had on it, it will be dead when reaching the destructor. So 
during destructor calling efl_del is likely accessing a dead object. Basically 
if we do efl_add, we should not need to call efl_del in the majority of case, 
just NULL the reference in the structure during invalidate, that's it. It does 
simplify the code a lot actually, but it differ from the code pattern we have 
and require more work than just replacing efl_add_ref/efl_unref by 
efl_add/efl_del.

> if you reverted the patch you would have lost all the "correct" parenting so
> i'm not sure that that is going to be better at all, unless you kept that and
> it's a partial revert really. i haven't looked at your branch yet, but it's
> kind of not making a lot of sense in this email right now. i am not sure what
> needs fixing where and how from this email at all, 

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Carsten Haitzler]

On Tue, 17 Apr 2018 19:33:02 -0400 Cedric Bail  said:

> On March 29, 2018 10:58 AM, Carsten Haitzler  wrote:
> > On Thu, 29 Mar 2018 13:17:43 -0400 Cedric Bail ced...@ddlm.me said:
> > > On March 28, 2018 8:06 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > > On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me said:
> > > > > On March 27, 2018 10:32 PM, Carsten Haitzler ras...@rasterman.com
> > > > > wrote:
> > > > > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr
> > > > > > said:
> > > > > Ideally yes, but at least for the _example, it requires to spend some
> > > > > more time to move them to use EFL_MAIN, which should be done in a
> > > > > different patch not related to fixing the usage of efl_add.
> > > > 
> > > > some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but
> > > > moving to efl_add_ref() isn't the "right direction" for most of these.
> > > 
> > > Yeah, I haven't been a big fan of efl_main_loop_get and did tend to avoid
> > > it. I should use it more often in the future.
> > 
> > it'd be good to stop doing that... :) long term at least. the slow and
> > steady steps are to fix our parenting in efl and examples. :) i fixed it
> > for you - or well i fixed everything i saw you changed. it may not be
> > perfect, but it's a step in the right direction imho :) point out if i got
> > something wrong. i didn't do "Extensive" work like you say and convert to
> > EFL_MAIN or within efl modify code to pass parents around many levels down
> > or something.
> 
> So I have been working on finishing correcting the lifecycle of eo object by
> making sure that efl_invalidate does happen before any efl_destructor code
> (Currently in master, it is possible to get efl_invalidate to be called after
> the object destructor as it is called inside efl.object.destructor). This
> means that efl_del and efl_parent_{get,set} are a typical wrong piece of code
> in the destructore. The patch 2fb5cc3ad09f6aaf82b5d1131ac5ed22ed848bd4 is
> wrong in a lot of place that I had to give up fixing it due to time
> constraint (basically it assume our use before invalidate was
> efl_add/efl_del, while the closest to our pattern was efl_add_ref with
> efl_unref in the desstructor). I have a branch devs/cedric/lifecycle with all
> the change in it including a revert of your patch. I can't land this patch at
> the moment as the old eo base efl_future lifecycle is a complete wreck with
> this change, so I will focus back on removing efl_future. If you had time in
> the mean time to check your patch and see how to fix it, it would be great.

wait wait. first. some objects. actually a fair few objects need to know their
parents to find their loop provider etc. an they need to do this on "shut down"
to delete stuff etc. from here. so my question will be:

where should an object do this "shut down" in the invalidate or in destructor.
you seem to say that it mys be in invalidate which means invalidate effectifely
is the "meat" of the destructor, just the object keeps itself around with some
minimal content until destructor is called. so what you are doing is ensuring
there is ALWAYS the sequence of invalidate then destroy, right?

so i'm not sure what's wrong with that patch - can you detail it? i simply
moved to having a parent instead of no parent in many cases, and used efl_del
again like it used to be which should clear out the only reference to the
object by unparenting it etc. etc. ...

if you reverted the patch you would have lost all the "correct" parenting so
i'm not sure that that is going to be better at all, unless you kept that and
it's a partial revert really. i haven't looked at your branch yet, but it's
kind of not making a lot of sense in this email right now. i am not sure what
needs fixing where and how from this email at all, so it'd be nice for you to
explain some more...

> Cedric
> 
> PS: there are still bug regarding the lifecycle of the focus manager in that
> branch that I haven't had time to figure out yet, will get to it later.
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> 


-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Cedric Bail]

On March 29, 2018 10:58 AM, Carsten Haitzler  wrote:
> On Thu, 29 Mar 2018 13:17:43 -0400 Cedric Bail ced...@ddlm.me said:
> > On March 28, 2018 8:06 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me said:
> > > > On March 27, 2018 10:32 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr 
> > > > > said:
> > > > Ideally yes, but at least for the _example, it requires to spend some 
> > > > more
> > > > time to move them to use EFL_MAIN, which should be done in a different
> > > > patch not related to fixing the usage of efl_add.
> > > 
> > > some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but moving 
> > > to
> > > efl_add_ref() isn't the "right direction" for most of these.
> > 
> > Yeah, I haven't been a big fan of efl_main_loop_get and did tend to avoid 
> > it.
> > I should use it more often in the future.
> 
> it'd be good to stop doing that... :) long term at least. the slow and steady
> steps are to fix our parenting in efl and examples. :) i fixed it for you - or
> well i fixed everything i saw you changed. it may not be perfect, but it's a
> step in the right direction imho :) point out if i got something wrong. i 
> didn't
> do "Extensive" work like you say and convert to EFL_MAIN or within efl modify
> code to pass parents around many levels down or something.

So I have been working on finishing correcting the lifecycle of eo object by 
making sure that efl_invalidate does happen before any efl_destructor code 
(Currently in master, it is possible to get efl_invalidate to be called after 
the object destructor as it is called inside efl.object.destructor). This means 
that efl_del and efl_parent_{get,set} are a typical wrong piece of code in the 
destructore. The patch 2fb5cc3ad09f6aaf82b5d1131ac5ed22ed848bd4 is wrong in a 
lot of place that I had to give up fixing it due to time constraint (basically 
it assume our use before invalidate was efl_add/efl_del, while the closest to 
our pattern was efl_add_ref with efl_unref in the desstructor). I have a branch 
devs/cedric/lifecycle with all the change in it including a revert of your 
patch. I can't land this patch at the moment as the old eo base efl_future 
lifecycle is a complete wreck with this change, so I will focus back on 
removing efl_future. If you had time in the mean time to check your patch and 
see how to fix it, it would be great.

Cedric

PS: there are still bug regarding the lifecycle of the focus manager in that 
branch that I haven't had time to figure out yet, will get to it later.

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Carsten Haitzler]

On Thu, 29 Mar 2018 13:17:43 -0400 Cedric Bail  said:

> On March 28, 2018 8:06 PM, Carsten Haitzler  wrote:
> > On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me said:
> > > On March 27, 2018 10:32 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr said:
> > > > at least some of these i think are wrong.
> > > > the benchmarks are "debatable" but ok.
> > > > 
> > > > but:
> > > > test_bg.c
> > > > test_box.c
> > > > test_calendar.c
> > > > test_efl_gfx_map.c
> > > > test_evas_map.c
> > > > test_evas_mask.c
> > > > test_evas_snapshot.c
> > > > test_gfx_filters.c
> > > > test_glview.c
> > > > test_nstate.c
> > > > test_part_bg.c
> > > > test_photocam.c
> > > > test_part_shadow.c
> > > > test_ui_button.c
> > > > test_ui_clock.c
> > > > test_ui_panes.c
> > > > test_ui_popup.c
> > > > test_ui_progressbar.c
> > > > test_ui_scroller.c
> > > > ... (there's a pattern here... basically every single change for adding
> > > > a win) -> shouldn't it use loop as parent?
> > > 
> > > This are test where we want lifetime control of this object I think. This
> > 
> > just because the parent is the main loop does not mean you don't have
> > lifetime control. you can always del it. parent should be there to follow
> > back to the loop driving it for async events (futures), animator events
> > etc. - sure. we glue that in behind the scenes currently when there is a
> > null parent, but these objects SHOULD be part of the main loop tree...
> 
> Yes, it was relying on the behind the scene magic trick to get to the main
> loop. 
> > > means to me not depending on the lifetime of the main loop to get them
> > > destroyed, but on controlling the reference only and explicitely. This
> > > could be moved to use parent relationship, I have no strong opinion on
> > > this.
> > 
> > as above... you still can control it explicitly. nothing ever stopped this.
> > it's more the problem of exposing objects that you want a parent to really
> > control and once exposed, the user getting the handle could violate that
> > contract by explicitly deleting and stealing from the parent that really
> > owns it - thus the part interface with the tmp objects...
>  
> > > > ecore_audio_custom.c
> > > > ecore_audio_playback.c
> > > > ecore_audio_to_ogg.c
> > > > ecore_poller_example.c
> > > > efl_io_copier_example.c
> > > > efl_io_queue_example.c
> > > > efl_net_server_example.c
> > > > efl_net_server_simple_example.c
> > > > ... (more patterns... i got 25% through the diff and got bored of copy &
> > > > pasting samples here)
> > > > -> shouldn't it use loop as parent?
> > > 
> > > Ideally yes, but at least for the _example, it requires to spend some more
> > > time to move them to use EFL_MAIN, which should be done in a different
> > > patch not related to fixing the usage of efl_add.
> > 
> > some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but moving to
> > efl_add_ref() isn't the "right direction" for most of these.
> 
> Yeah, I haven't been a big fan of efl_main_loop_get and did tend to avoid it.
> I should use it more often in the future.

it'd be good to stop doing that... :) long term at least. the slow and steady
steps are to fix our parenting in efl and examples. :) i fixed it for you - or
well i fixed everything i saw you changed. it may not be perfect, but it's a
step in the right direction imho :) point out if i got something wrong. i didn't
do "Extensive" work like you say and convert to EFL_MAIN or within efl modify
code to pass parents around many levels down or something.

> > > > test_part_shadow.c
> > > > ecore_audio_to_ogg.c
> > > > 
> > > > -> shouldn't is be efl_del because add should have a parent like loop?
> > > 
> > > If we move them to use efl add with the loop as parent, yes, it should be,
> > > but as the current code use efl_add_ref, using efl_unref is the correct
> > > counter part.
> > 
> > sure. they go along for the ride. i just saw a massive commit and lots of
> > changes that were not even necessary (changing to unrefs from dels and to
> > add_ref instead of just using a parent instead of NULL).
> > 
> > ok. i've fixed it locally... i'll push.
> 
> Thanks !

no problems. i was holding that commit until i heard back from you. done. :)

-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Cedric Bail]

On March 28, 2018 8:06 PM, Carsten Haitzler  wrote:
> On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me said:
> > On March 27, 2018 10:32 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr said:
> > > at least some of these i think are wrong.
> > > the benchmarks are "debatable" but ok.
> > > 
> > > but:
> > > test_bg.c
> > > test_box.c
> > > test_calendar.c
> > > test_efl_gfx_map.c
> > > test_evas_map.c
> > > test_evas_mask.c
> > > test_evas_snapshot.c
> > > test_gfx_filters.c
> > > test_glview.c
> > > test_nstate.c
> > > test_part_bg.c
> > > test_photocam.c
> > > test_part_shadow.c
> > > test_ui_button.c
> > > test_ui_clock.c
> > > test_ui_panes.c
> > > test_ui_popup.c
> > > test_ui_progressbar.c
> > > test_ui_scroller.c
> > > ... (there's a pattern here... basically every single change for adding a
> > > win) -> shouldn't it use loop as parent?
> > 
> > This are test where we want lifetime control of this object I think. This
> 
> just because the parent is the main loop does not mean you don't have lifetime
> control. you can always del it. parent should be there to follow back to the
> loop driving it for async events (futures), animator events etc. - sure. we
> glue that in behind the scenes currently when there is a null parent, but 
> these
> objects SHOULD be part of the main loop tree...

Yes, it was relying on the behind the scene magic trick to get to the main loop.
 
> > means to me not depending on the lifetime of the main loop to get them
> > destroyed, but on controlling the reference only and explicitely. This could
> > be moved to use parent relationship, I have no strong opinion on this.
> 
> as above... you still can control it explicitly. nothing ever stopped this.
> it's more the problem of exposing objects that you want a parent to really
> control and once exposed, the user getting the handle could violate that
> contract by explicitly deleting and stealing from the parent that really owns
> it - thus the part interface with the tmp objects...
 
> > > ecore_audio_custom.c
> > > ecore_audio_playback.c
> > > ecore_audio_to_ogg.c
> > > ecore_poller_example.c
> > > efl_io_copier_example.c
> > > efl_io_queue_example.c
> > > efl_net_server_example.c
> > > efl_net_server_simple_example.c
> > > ... (more patterns... i got 25% through the diff and got bored of copy &
> > > pasting samples here)
> > > -> shouldn't it use loop as parent?
> > 
> > Ideally yes, but at least for the _example, it requires to spend some more
> > time to move them to use EFL_MAIN, which should be done in a different patch
> > not related to fixing the usage of efl_add.
> 
> some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but moving to
> efl_add_ref() isn't the "right direction" for most of these.

Yeah, I haven't been a big fan of efl_main_loop_get and did tend to avoid it. I 
should use it more often in the future.

> > > test_part_shadow.c
> > > ecore_audio_to_ogg.c
> > > 
> > > -> shouldn't is be efl_del because add should have a parent like loop?
> > 
> > If we move them to use efl add with the loop as parent, yes, it should be,
> > but as the current code use efl_add_ref, using efl_unref is the correct
> > counter part.
> 
> sure. they go along for the ride. i just saw a massive commit and lots of
> changes that were not even necessary (changing to unrefs from dels and to
> add_ref instead of just using a parent instead of NULL).
> 
> ok. i've fixed it locally... i'll push.

Thanks !

Cedric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Cedric Bail]

On March 28, 2018 8:00 PM, Carsten Haitzler  wrote:
> On Wed, 28 Mar 2018 12:21:53 -0400 Cedric Bail ced...@ddlm.me said:
> > On March 27, 2018 10:18 PM, Carsten Haitzler ras...@rasterman.com wrote:
> > > On Fri, 23 Mar 2018 22:57:28 +0900 Stefan Schmidt ste...@osg.samsung.com
> > > said:
> > > 
> > > > On 03/21/2018 09:57 AM, Cedric BAIL wrote:
> > > > > cedric pushed a commit to branch master.
> > > > > 
> > > > > http://git.enlightenment.org/core/efl.git/commit/?id=4c4177ac207f982de8139c47c7afedd26ff9e15a
> > > > > 
> > > > > commit 4c4177ac207f982de8139c47c7afedd26ff9e15a
> > > > > Author: Cedric BAIL ced...@osg.samsung.com
> > > > > Date: Thu Mar 15 12:50:20 2018 -0400
> > > > > 
> > > > > efl: use efl_add_ref to create objects which have no parent
> > > > > 
> > > > > Signed-off-by: Mike Blumenkrantz 
> > > > > 
> > > > 
> > > > This is the third patch of patches I see that have Cedric as author and 
> > > > a
> > > > sign off by Mike.
> > > > 
> > > > We have no document backing the signed off by line. No document 
> > > > describing
> > > > developer certificate of origin  (DCO) at all. Thus I ask you to stop
> > > > using signed off by tags here.
> > > 
> > > indeed we've never used anything like the above.
> > 
> > I have been using for more than a year Signed-off-by as a mean for reviewed.
> > Check all the patch I have pushed last year from phab. This was me being 
> > lazy
> > as there is no way from the default git command to append automatically a
> > Reviewed-by tag and as no one else was doing much on this and I needed an
> > easy way to account for the reviewing, I went with that. I have now setup a
> > local script that add the Reviewed-by line.
> 
> but stefan is still right that we have never documented the signed-off line
> with a DCO etc. that was it's original intent from kernel land, not as a
> reviewed-by.

Agreed and fixed for future case. I should now have Reviewed-by line added when 
I push a patch that I review.
 
> > > > If this aims to be a reviewed-by tag, please switch to using that tag.
> > > > If Mike is the author he should be listed as such and git will reference
> > > > Cedric as committer.
> > > 
> > > at least if going via phab ... the log will contain this (Reviewed-by).
> > 
> > No. Phab doesn't add any line. It says reviewer and if you look at who 
> > pushed
> 
> ok - sorry. I was wrong. it's "Reviewers". it does add who is doing the review
> to the log. but it tells me who in the git commits mailing list:
> 
> sanghyeonlee pushed a commit to branch master.
> 
> http://git.enlightenment.org/core/efl.git/commit/?id=2bad5fffce2474a80a1173448b1be10c9c7b1949
> 
> commit 2bad5fffce2474a80a1173448b1be10c9c7b1949
> Author: subhransu mohanty sub.moha...@samsung.com
> Date: Wed Feb 28 15:15:20 2018 +0900
> 
> eina/bezier: use FLT_EQ marcro for float equal comparison.
> 
> Reviewers: SanghyeonLee, jpeg, cedric
> 
> Subscribers: cedric
> 
> Differential Revision: https://phab.enlightenment.org/D5828
> 
> i see potential reviewers and who actually pushed it.

Yup, exactly.
 
> > the patch in the tree, you might be able to infer who did the last review.
> > But that wasn't very nice to process, so I went with the above. It is fixed
> > now.

Cedric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[The Rasterman]

On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail  said:

> On March 27, 2018 10:32 PM, Carsten Haitzler  wrote:
> > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr said:
> > at least some of these i think are wrong.
> > the benchmarks are "debatable" but ok.
> > 
> > but:
> > 
> > test_bg.c
> > test_box.c
> > test_calendar.c
> > test_efl_gfx_map.c
> > test_evas_map.c
> > test_evas_mask.c
> > test_evas_snapshot.c
> > test_gfx_filters.c
> > test_glview.c
> > test_nstate.c
> > test_part_bg.c
> > test_photocam.c
> > test_part_shadow.c
> > test_ui_button.c
> > test_ui_clock.c
> > test_ui_panes.c
> > test_ui_popup.c
> > test_ui_progressbar.c
> > test_ui_scroller.c
> > ... (there's a pattern here... basically every single change for adding a
> > win) -> shouldn't it use loop as parent?
> 
> This are test where we want lifetime control of this object I think. This

just because the parent is the main loop does not mean you don't have lifetime
control. you can always del it. parent should be there to follow back to the
loop driving it for async events (futures), animator events etc. - sure. we
glue that in behind the scenes currently when there is a null parent, but these
objects SHOULD be part of the main loop tree...

> means to me not depending on the lifetime of the main loop to get them
> destroyed, but on controlling the reference only and explicitely. This could
> be moved to use parent relationship, I have no strong opinion on this. 

as above... you still can control it explicitly. nothing ever stopped this.
it's more the problem of exposing objects that you want a parent to really
control and once exposed, the user getting the handle could violate that
contract by explicitly deleting and stealing from the parent that really owns
it - thus the part interface with the tmp objects...

> > ecore_audio_custom.c
> > ecore_audio_playback.c
> > ecore_audio_to_ogg.c
> > ecore_poller_example.c
> > efl_io_copier_example.c
> > efl_io_queue_example.c
> > efl_net_server_example.c
> > efl_net_server_simple_example.c
> > ... (more patterns... i got 25% through the diff and got bored of copy &
> > pasting samples here)
> > -> shouldn't it use loop as parent?
> 
> Ideally yes, but at least for the _example, it requires to spend some more
> time to move them to use EFL_MAIN, which should be done in a different patch
> not related to fixing the usage of efl_add.

some use EFL_MAIN ... otherwise efl_main_loop_get() will work. but moving to
efl_add_ref() isn't the "right direction" for most of these.

> > test_part_shadow.c
> > ecore_audio_to_ogg.c
> > -> shouldn't is be efl_del because add should have a parent like loop?
> 
> If we move them to use efl add with the loop as parent, yes, it should be,
> but as the current code use efl_add_ref, using efl_unref is the correct
> counter part.

sure. they go along for the ride. i just saw a massive commit and lots of
changes that were not even necessary (changing to unrefs from dels and to
add_ref instead of just using a parent instead of NULL).

ok. i've fixed it locally... i'll push.

-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[The Rasterman]

On Wed, 28 Mar 2018 12:21:53 -0400 Cedric Bail  said:

> On March 27, 2018 10:18 PM, Carsten Haitzler  wrote:
> > On Fri, 23 Mar 2018 22:57:28 +0900 Stefan Schmidt ste...@osg.samsung.com
> > said:
> > > On 03/21/2018 09:57 AM, Cedric BAIL wrote:
> > > 
> > > > cedric pushed a commit to branch master.
> > > > 
> > > > http://git.enlightenment.org/core/efl.git/commit/?id=4c4177ac207f982de8139c47c7afedd26ff9e15a
> > > > 
> > > > commit 4c4177ac207f982de8139c47c7afedd26ff9e15a
> > > > Author: Cedric BAIL ced...@osg.samsung.com
> > > > Date: Thu Mar 15 12:50:20 2018 -0400
> > > > 
> > > > efl: use efl_add_ref to create objects which have no parent
> > > > 
> > > > Signed-off-by: Mike Blumenkrantz 
> > > > 
> > > This is the third patch of patches I see that have Cedric as author and a
> > > sign off by Mike.
> > > 
> > > We have no document backing the signed off by line. No document describing
> > > developer certificate of origin  (DCO) at all. Thus I ask you to stop
> > > using signed off by tags here.
> > 
> > indeed we've never used anything like the above.
> 
> I have been using for more than a year Signed-off-by as a mean for reviewed.
> Check all the patch I have pushed last year from phab. This was me being lazy
> as there is no way from the default git command to append automatically a
> Reviewed-by tag and as no one else was doing much on this and I needed an
> easy way to account for the reviewing, I went with that. I have now setup a
> local script that add the Reviewed-by line. 

but stefan is still right that we have never documented the signed-off line
with a DCO etc. that was it's original intent from kernel land, not as a
reviewed-by. 

> > > If this aims to be a reviewed-by tag, please switch to using that tag.
> > > If Mike is the author he should be listed as such and git will reference
> > > Cedric as committer.
> > 
> > at least if going via phab ... the log will contain this (Reviewed-by).
> 
> No. Phab doesn't add any line. It says reviewer and if you look at who pushed

ok - sorry. I was wrong. it's "Reviewers". it does add who is doing the review
to the log. but it tells me who in the git commits mailing list:

sanghyeonlee pushed a commit to branch master.

http://git.enlightenment.org/core/efl.git/commit/?id=2bad5fffce2474a80a1173448b1be10c9c7b1949

commit 2bad5fffce2474a80a1173448b1be10c9c7b1949
Author: subhransu mohanty 
Date:   Wed Feb 28 15:15:20 2018 +0900

eina/bezier: use FLT_EQ marcro for float equal comparison.

Reviewers: SanghyeonLee, jpeg, cedric

Subscribers: cedric

Differential Revision: https://phab.enlightenment.org/D5828

i see potential reviewers and who actually pushed it.

> the patch in the tree, you might be able to infer who did the last review.
> But that wasn't very nice to process, so I went with the above. It is fixed
> now.
> 
> Cedric
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel


-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Cedric Bail]

On March 27, 2018 10:32 PM, Carsten Haitzler  wrote:
> On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL cedric.b...@free.fr said:
> at least some of these i think are wrong.
> the benchmarks are "debatable" but ok.
> 
> but:
> 
> test_bg.c
> test_box.c
> test_calendar.c
> test_efl_gfx_map.c
> test_evas_map.c
> test_evas_mask.c
> test_evas_snapshot.c
> test_gfx_filters.c
> test_glview.c
> test_nstate.c
> test_part_bg.c
> test_photocam.c
> test_part_shadow.c
> test_ui_button.c
> test_ui_clock.c
> test_ui_panes.c
> test_ui_popup.c
> test_ui_progressbar.c
> test_ui_scroller.c
> ... (there's a pattern here... basically every single change for adding a win)
> -> shouldn't it use loop as parent?

This are test where we want lifetime control of this object I think. This means 
to me not depending on the lifetime of the main loop to get them destroyed, but 
on controlling the reference only and explicitely. This could be moved to use 
parent relationship, I have no strong opinion on this.
 
> ecore_audio_custom.c
> ecore_audio_playback.c
> ecore_audio_to_ogg.c
> ecore_poller_example.c
> efl_io_copier_example.c
> efl_io_queue_example.c
> efl_net_server_example.c
> efl_net_server_simple_example.c
> ... (more patterns... i got 25% through the diff and got bored of copy &
> pasting samples here)
> -> shouldn't it use loop as parent?

Ideally yes, but at least for the _example, it requires to spend some more time 
to move them to use EFL_MAIN, which should be done in a different patch not 
related to fixing the usage of efl_add.
 
> test_part_shadow.c
> ecore_audio_to_ogg.c
> -> shouldn't is be efl_del because add should have a parent like loop?

If we move them to use efl add with the loop as parent, yes, it should be, but 
as the current code use efl_add_ref, using efl_unref is the correct counter 
part.

Cedric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Cedric Bail]

On March 27, 2018 10:18 PM, Carsten Haitzler  wrote:
> On Fri, 23 Mar 2018 22:57:28 +0900 Stefan Schmidt ste...@osg.samsung.com said:
> > On 03/21/2018 09:57 AM, Cedric BAIL wrote:
> > 
> > > cedric pushed a commit to branch master.
> > > 
> > > http://git.enlightenment.org/core/efl.git/commit/?id=4c4177ac207f982de8139c47c7afedd26ff9e15a
> > > 
> > > commit 4c4177ac207f982de8139c47c7afedd26ff9e15a
> > > Author: Cedric BAIL ced...@osg.samsung.com
> > > Date: Thu Mar 15 12:50:20 2018 -0400
> > > 
> > > efl: use efl_add_ref to create objects which have no parent
> > > 
> > > Signed-off-by: Mike Blumenkrantz 
> > > 
> > This is the third patch of patches I see that have Cedric as author and a
> > sign off by Mike.
> > 
> > We have no document backing the signed off by line. No document describing
> > developer certificate of origin  (DCO) at all. Thus I ask you to stop using
> > signed off by tags here.
> 
> indeed we've never used anything like the above.

I have been using for more than a year Signed-off-by as a mean for reviewed. 
Check all the patch I have pushed last year from phab. This was me being lazy 
as there is no way from the default git command to append automatically a 
Reviewed-by tag and as no one else was doing much on this and I needed an easy 
way to account for the reviewing, I went with that. I have now setup a local 
script that add the Reviewed-by line.
 
> > If this aims to be a reviewed-by tag, please switch to using that tag.
> > If Mike is the author he should be listed as such and git will reference
> > Cedric as committer.
> 
> at least if going via phab ... the log will contain this (Reviewed-by).

No. Phab doesn't add any line. It says reviewer and if you look at who pushed 
the patch in the tree, you might be able to infer who did the last review. But 
that wasn't very nice to process, so I went with the above. It is fixed now.

Cedric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[The Rasterman]

On Fri, 23 Mar 2018 22:57:28 +0900 Stefan Schmidt  said:

> Hello.
> 
> 
> On 03/21/2018 09:57 AM, Cedric BAIL wrote:
> > cedric pushed a commit to branch master.
> >
> > http://git.enlightenment.org/core/efl.git/commit/?id=4c4177ac207f982de8139c47c7afedd26ff9e15a
> >
> > commit 4c4177ac207f982de8139c47c7afedd26ff9e15a
> > Author: Cedric BAIL 
> > Date:   Thu Mar 15 12:50:20 2018 -0400
> >
> > efl: use efl_add_ref to create objects which have no parent
> > 
> > Signed-off-by: Mike Blumenkrantz 
> 
> This is the third patch of patches I see that have Cedric as author and a
> sign off by Mike.
> 
> We have no document backing the signed off by line. No document describing
> developer certificate of origin  (DCO) at all. Thus I ask you to stop using
> signed off by tags here.

indeed we've never used anything like the above.

> If this aims to be a reviewed-by tag, please switch to using that tag.
> If Mike is the author he should be listed as such and git will reference
> Cedric as committer.

at least if going via phab ... the log will contain this (Reviewed-by).

> Please get this sorted out. Thanks.
> 
> regards
> Stefan Schmidt
> 
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel


-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 02/09: efl: use efl_add_ref to create objects which have no parent

[Stefan Schmidt]

Hello.


On 03/21/2018 09:57 AM, Cedric BAIL wrote:
> cedric pushed a commit to branch master.
>
> http://git.enlightenment.org/core/efl.git/commit/?id=4c4177ac207f982de8139c47c7afedd26ff9e15a
>
> commit 4c4177ac207f982de8139c47c7afedd26ff9e15a
> Author: Cedric BAIL 
> Date:   Thu Mar 15 12:50:20 2018 -0400
>
> efl: use efl_add_ref to create objects which have no parent
> 
> Signed-off-by: Mike Blumenkrantz 

This is the third patch of patches I see that have Cedric as author and a sign 
off by Mike.

We have no document backing the signed off by line. No document describing 
developer certificate of origin  (DCO) at all.
Thus I ask you to stop using signed off by tags here.

If this aims to be a reviewed-by tag, please switch to using that tag.
If Mike is the author he should be listed as such and git will reference Cedric 
as committer.

Please get this sorted out. Thanks.

regards
Stefan Schmidt


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel