Page MenuHomePhabricator

elm_widget: update eo parent when widget parent is changed
AbandonedPublic

Authored by tasn on Dec 15 2015, 10:04 PM.

Details

Summary

After obj is sub_object_del'ed, eo's parent-child relationship becomes
different from elm_widget's parent-child one. It causes accidental
deletion of obj when its original eo parent obj is deleted.
This patch updates obj's eo parent to widget parent to keep consistency
in managing child object.

Diff Detail

Repository
rELM core/elementary
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1006
Build 1071: arc lint + arc unit
conr2d updated this revision to Diff 7844.Dec 15 2015, 10:04 PM
conr2d retitled this revision from to elm_widget: update eo parent to top widget when sub_object_del.
conr2d updated this object.
conr2d added reviewers: JackDanielZ, tasn, cedric, raster.
conr2d added subscribers: id213sin, woohyun, CHAN.

This should not be merged now. It doesn't cover the case of adding a widget to the orphan parent already detached widget hierarchy.

conr2d updated this revision to Diff 7853.Dec 16 2015, 2:52 AM

make eo parent and widget parent same

conr2d retitled this revision from elm_widget: update eo parent to top widget when sub_object_del to elm_widget: make eo parent and widget parent same.Dec 16 2015, 2:54 AM
conr2d updated this object.
conr2d updated this revision to Diff 7854.Dec 16 2015, 3:01 AM

decrease ref count after setting parent when sub_object_add'ed

conr2d updated this revision to Diff 7855.Dec 16 2015, 3:08 AM

elaborate comments

conr2d added a reviewer: jpeg.Dec 16 2015, 4:02 AM
tasn edited edge metadata.Dec 16 2015, 6:50 AM

It's actually really annoying that you keep on posting updates. Could you please just send patches to phab when they are ready?

Also, if you are making them the same, why not *actually* make them the same? That is, remove the widget parent internally and just make it an alias for Eo parent?

I remember we didn't want to do it because it's not always the same thing (there are actually 3 different widget parents in elm iirc, and probably others lurking around).

@tasn
Sorry for annoying you. I'm not familiar with IRC, so I wanted to receive opinions by uploading patches. I'll be more careful when making a diff.
By the way, this patch is suggested as an alternative of D2942.
In this issue, problem is the conflict between eo's parent-child management and that of elm_widget. Your suggestion, removing elm_widget parent but makking it as an alias of eo parent, will break "content_unset" concept and backward compatibility.
It would rather be better to make a godfather object for orphan children objects so as not to delete unset objects because of deletion of unrelated ex-parent object.

tasn added a comment.Dec 17 2015, 8:00 AM

I know it can cause issues, but it sounded like that was exactly what you were trying to do, so I asked. Merging the concepts is exactly that no?

Anyhow, I don't remember when it was but we talked about it before (a few of us), and we said that maybe content unset should just ref + 1, and content set should just ref - 1, so content setting would be an implied transfer of ownership, this should fix everything and keep backwards compat, no?

Is this kind of what you are trying to do here?

JackDanielZ edited edge metadata.Dec 17 2015, 11:38 PM

Hi guys,

Merging the elm_widget parent and the Eo parent seems a good idea. It means that the Eo parent will change on every sub_object_add/del. I don't see any issues on this.

@tasn, concerning the refs stuff, I didn't understand why you would need it, as modifying the parent already modifies the ownership.

As I told Jee-Yong, I don't think setting the parent to NULL during sub_object_del is good, as it can create issues when trying to access the canvas... Instead, the top widget could/should be set as parent.

JackDanielZ

tasn added a comment.Dec 21 2015, 6:18 AM

Cool, can set it to the canvas then, or just the top widget as suggested. Doesn't matter to me. Please proceed in merging them then.

Hello, little question, if eo_parent and widget_parent are the same sdc->parent_obj looks redundant to me, why not removing it?

tasn added a comment.Dec 31 2015, 6:15 AM

@bu5hm4n, that's essentially what I said. If we are merging their behaviours, we should just merge the data structure too and just get rid of all of the redundancy.

cedric edited edge metadata.Jan 4 2016, 2:13 PM

Should we wait for the removal of the parent_obj or should we land this patch ?

conr2d added a comment.Jan 4 2016, 2:18 PM

@cedric

Wait for a while, I will update this patch soon. I've been too busy.

conr2d updated this revision to Diff 8049.Jan 4 2016, 9:55 PM
conr2d edited edge metadata.

merge eo parent and widget parent and remove parent_obj from Elm_Widget_Smart_Data

conr2d updated this revision to Diff 8058.Jan 5 2016, 2:48 AM

set parent as null when sub_object_del is called during destruction

tasn added a comment.Jan 5 2016, 9:46 AM

@cedric, not land yet, I wanna review it now that it's up to date.

@conr2d, I think that while we are at it, we should probably remove Elm.Widget.parent from the .eo file, keep the legacy function for legacy purposes, and change all of elm to just use eo_parent_get(). Thoughts?

Anyhow, before you go at it, let me review the code. Will do so tomorrow.

tasn requested changes to this revision.Jan 6 2016, 8:35 AM
tasn edited edge metadata.

I have a few inline comments, and the following comment from yesterday:
I think that while we are at it, we should probably remove Elm.Widget.parent from the .eo file, keep the legacy function for legacy purposes, and change all of elm to just use eo_parent_get(). Thoughts?

As we discussed previously, I'd be a bit surprised to see everything just working after this change. I remember evas parent and elm parent were different in some places. An easy example would be:

obj = elm_entry_add(some_edje_object);

This would set obj to an entry object with a non entry parent. We should probably error on this in the finalizer if this is indeed illegal, or at least infer the correct elm parent or do something correct about it. Though I think that is some evil work behind the scenes that is probably not a good idea. Another broken scenario would be to sent parent to non elm parent after the object has been created. Again, if you change that behind the scenes for the user it'll be broken, and if you don't it'll be broken.

In summary, I'm a bit wary of some of the assumptions this approach has.

src/lib/elm_widget.c
1307

Please also enclose this with {}.

2933

I think this may be problematic (as previously mentioned). Are you sure this is completely consistent with reality? I.e must the parent be a widget? If it must, then this check is pointless, and if it doesn't have to be, then this is probably wrong, as NULL is probably not the right value, and a better value could be inferred.

Could you please test locally by running a few elm entry tests (like genlist and others) before your changes with the following line added:

if (!eo_isa(sd->parent_obj, ELM_WIDGET_CLASS)) printf("DAMN!!! :((((\n");

and see if it's an existing scenario? (Obviously also look at the code and see if it's possible).

This revision now requires changes to proceed.Jan 6 2016, 8:35 AM

@tasn

You pointed out the potential risk of this patch exactly. There was little consideration about the cases of unusual parent-child relationship like nonwidget-widget or widget-nonwidget. I will refine the patch to cover general cases and apply your comment. Thank you.

conr2d updated this revision to Diff 8480.Feb 16 2016, 10:33 PM
conr2d edited edge metadata.
conr2d retitled this revision from elm_widget: make eo parent and widget parent same to elm_widget: update eo parent when widget parent is changed.
conr2d updated this object.

update eo parent when widget parent is changed

tasn added a comment.Feb 18 2016, 9:01 AM

Looks fine, I only have one query. Is parent_obj set by any other means (I think it is)? If so, we should update the eo parent there too.

Also, what the hell, I just looked at parent_set. This is the code:

EOLIAN static void
_elm_widget_parent_set(Eo *obj EINA_UNUSED, Elm_Widget_Smart_Data *_pd EINA_UNUSED, Evas_Object *parent EINA_UNUSED)
{
}

This looks awfully wrong. I wonder who did it and why, and also why it's not documented.
tasn added a comment.EditedFeb 18 2016, 9:02 AM

@JackDanielZ did it in rELMf7086fac95cb557f65a68c34639666397fd6d7d6
I think by accident, but you can never be sure with that menace of a guy. ;P

Fixing.

Maybe you should switch to using that function so you don't have to duplicate that functionality all around?

Edit: not even, it actually goes way back. Still investigating.

Edit2: Can't fix. Setting parent makes thing segfault. I don't have time to open this can of worms.

tasn added a comment.Feb 18 2016, 9:10 AM

Anddddd one last comment. Sorry for the storm.

src/lib/elm_widget.c
1289

This is actually different in upstream.

It changed recently, in rELMeae9499906e1f02a9eee558c9f5e31357606c578, so I think you should drop the if and in both cases set the widget_top (again, using parent_set, and not like this). I'm fixing parent_set now.

tasn added a comment.Feb 29 2016, 5:59 AM

Hey @conr2d,

I came up with a solution for the multiple parents while I was in HQ. I discussed it a bit and people seem happy. I'm going to work on Eo4 (more about that soon) in the next few days, but once that's done, I'll tackle the multiple parents thing to be clear and consistent.

Tom.

@tasn That's good news. I will wait for Eo4 patch that you make. Thanks for telling me. : )

raster edited edge metadata.May 17 2016, 7:11 AM

ok now? visual parent should solve this.

tasn commandeered this revision.Jun 20 2016, 2:43 AM
tasn edited reviewers, added: conr2d; removed: tasn.

No reply for a while, let's consider this close.

tasn abandoned this revision.Jun 20 2016, 2:44 AM

Closed.