Page MenuHomePhabricator

efl_ui_scroll : apply finalize and invalidate object.
AbandonedPublic

Authored by SanghyeonLee on Jul 12 2018, 12:57 AM.

Details

Reviewers
eagleeye
Hermet
devilhorns
bu5hm4n
Group Reviewers
committers
Summary

To construct/destruct eo object with proper sequence with evas & efl_ui_widget,

apply finalize and invalidate features.
This patch can prevent wrong access of evas_object_callback_del_full after
object was invalidated.
Test Plan

elementary_test and examples.

run efl.ui.scroller and efl_ui_list_example_1 and efl_ui_view_list_example_1
to check callback_del_full make an error messages.
(the items in the widget also emitted same error log. it will be fixed by
 another patch soon)

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6856
Build 6837: arc lint + arc unit
SanghyeonLee created this revision.Jul 12 2018, 12:57 AM
SanghyeonLee requested review of this revision.Jul 12 2018, 12:57 AM

rebasing patch on latest master

fix sub-object invalid deletions

pd->smanager, pd->box and pd->pan are bound to the object's lifecycle, so I would suggest to not explicitly efl_del those in the invalidate() call.
From what I understand, the invalidate() is meant for cases where you have to have access to the child before it gets deleted (which now occurs before the parent's destructor).

IMHO

bu5hm4n requested changes to this revision.Jul 12 2018, 4:40 AM
bu5hm4n added a subscriber: bu5hm4n.

Hello,

thx for the patch, the idea looks good to me :)

Btw. is there some wiki article describing the concept of those scroll objects ?

src/lib/elementary/efl_ui_list.c
503

Empty destructor ... :)

src/lib/elementary/efl_ui_scroll_manager.c
2429

Not too sure if this is a good idea, you moved the complete initialization to finalize, even sd->obj = obj and others.

Note that basically every api call can happen between contstructor and finalizer.

To me it looks like most of the calls should stay in the constructor, except the things that are depending on the parent.

2498

Why having a empty destructor ? :)

src/lib/elementary/efl_ui_scroll_manager.eo
28

Nit: whitespace error

src/lib/elementary/efl_ui_scroller.c
398

Its okay this time, but thats a additional patch next time, ok ? :)

438

What is this needed for ?

467

... empty constructor :)
Why?

src/lib/elementary/efl_ui_view_list.c
730

the pan object is now freed in two implementations, the scrollmanager and here, there should be some kind of convension what is allowed to free this and what is not. (Probebly annotated with @owner in a .eo file, but that can be handled in a additional patch)

This revision now requires changes to proceed.Jul 12 2018, 4:40 AM

I'll rebase and update patch by the comment of @bu5hm4n soon. thanks to review it.

SanghyeonLee abandoned this revision.Thu, Jan 31, 1:02 AM