Page MenuHomePhabricator

elementary: introduce Efl.Ui.CollectionView a generic listing View.
ClosedPublic

Authored by cedric on Sep 15 2019, 11:39 PM.

Details

Summary

The idea of this widget is to provide to MVVM what Efl.Ui.Collection provide and
leverage the same shared logic for layout.

Co-authored-by: Mike Blumenkrantz <zmike@samsung.com>
Co-authored-by: Marcel Hollerbach <mail@marcel-hollerbach.de>

Depends on D10034

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cedric created this revision.Sep 15 2019, 11:39 PM
bu5hm4n requested changes to this revision.Sep 16 2019, 1:24 AM

Other than that this looks quite okay.

My only problem with this is that there are no unit tests at all, i am like 100% sure that someone will change a tiny little bit, and everything will fall apart :(

src/examples/elementary/efl_ui_collection_view_example_1.c
21

This is not needed anymore.

42

Can we remove that ?

121

This is not needed anymore.

131

Can you remove this ?

This revision now requires changes to proceed.Sep 16 2019, 1:24 AM
segfaultxavi requested changes to this revision.Sep 16 2019, 4:36 AM

TOTALLY UNACCEPTABLE

src/lib/elementary/efl_ui_collection_view.eo
9

Indentation...

Macro flammenwerfer:

13

NO DOCS?!?!?!

one bug that I found in current version is.. if I move scrollbar by force, no item shows up. if I scroll by thumb or wheel, item will be show up after that event.
seems position move is not triggered view update. won't be a problem in tizen as we don't have dragable scrollbar though.

SanghyeonLee added inline comments.Sep 16 2019, 10:35 PM
src/examples/elementary/efl_ui_collection_view_example_1.c
2

it need to be gcc -o efl_ui_collection_view_example_1 efl_ui_collection_view_example_1.c `pkg-config --cflags --libs elementary

23

not used.

112

here we could add

efl_model_property_set(model, "item.width", eina_value_uint_new(50));
efl_model_property_set(model, "item.height", eina_value_uint_new(50));

for the grid size set.
I normally prefer to be public on size model and call the API like,
efl_ui_size_model_item_size_set(model, EINA_SIZE2D(50, 50));

if you think this is okay, let me raise the patch for that.

123

what about adding img factory for grid and list icon?
this is extra work, so I'll update patch after this patch is landed.

cedric planned changes to this revision.Sep 17 2019, 8:42 PM
cedric added inline comments.
src/examples/elementary/efl_ui_collection_view_example_1.c
112

On the long run I would like to properly support other computation method than homogeneous. Currently the code kind of allow it, but there is no API exposing it. I would really like to keep it this way for now. I will update my patch to set the size on the first item in a way that is not to problematic.

123

That would be nice. Thank you!

cedric added inline comments.Sep 17 2019, 8:45 PM
src/examples/elementary/efl_ui_collection_view_example_1.c
112

On the long run I would like to properly support other computation method than homogeneous. Currently the code kind of allow it, but there is no API exposing it. I would really like to keep it this way for now. I will update my patch to set the size on the first item in a way that is not to problematic.

123

That would be nice. Thank you!

SanghyeonLee added inline comments.Sep 17 2019, 11:24 PM
src/examples/elementary/efl_ui_collection_view_example_1.c
112

that is what i really want to know :) thanks.

zmike requested changes to this revision.Sep 19 2019, 1:49 PM

Needs elm_test case.

This revision now requires changes to proceed.Sep 19 2019, 1:49 PM
cedric updated this revision to Diff 25292.Fri, Sep 20, 3:25 PM

phab!!!

bu5hm4n requested changes to this revision.Sat, Sep 21, 3:21 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_ui_widget_factory.c
114

Here is no assertion that ui_view is a item. It can be whatever widget you like, which results in a shitload of errors, if you chain factories to factories.

This revision now requires changes to proceed.Sat, Sep 21, 3:21 AM
cedric updated this revision to Diff 25419.Mon, Sep 23, 4:20 PM

rebase and take comment into account

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 24, 11:13 AM
This revision was automatically updated to reflect the committed changes.