Page MenuHomePhabricator

Add a new class efl_ui_pager and more
Needs ReviewPublic

Authored by eunue on Mar 6 2018, 2:05 AM.

Details

Reviewers
woohyun
cedric
Summary

This patch adds a new class efl_ui_pager and related classes.

Test Plan

elementary_test -to pager

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
eunue requested review of this revision.Mar 6 2018, 2:05 AM
eunue created this revision.
cedric requested changes to this revision.Mar 6 2018, 5:26 PM

The interface seems to make sense to me, I will have to do another pass I think once this is ready to land. Main question now is what is the purpose of this widget ? Looking at the documentation isn't very clear. I know the purpose as we have discussed about this IRL, but it would be better for everyone to get a good idea of what it is for and in what context use this widget.

src/lib/elementary/efl_page_indicator.eo
4

It would be nice to have slightly more documentation on what is the purpose of this class.

src/lib/elementary/efl_page_indicator_icon.eo
4

Same comment here. Would be nice to have a little bit more doc on the purpose of this class.

src/lib/elementary/efl_page_transition.eo
5

The comment apply also to this entire class. Could you put some more documentation ?

src/lib/elementary/efl_page_transition_scroll.c
37

If you are always carrying this rectangle, you might as well put the structure directly instead of using pointer. Then you just EINA_RECTANGLE_SET.

45

It seems to me that this loop could be done in the above loop if by just remembering the previous item and then fixing the first and last item of the list, no ?

69

Could you add a small comment for this loop and the next one describing what they are doing ?

139

Could you also put a small comment here describing the purpose of this loop ?

282

Can you put a comment describing the purpose of this if and another comment for the entire loop ?

358

There is a lot of common code is the two case of this if. I feel it would be beneficial to have the if inside the loop for a easier reading and maintenance of this code.

360

Also small comment for the purpose of this loop.

475

Small comment for this loop ? Also this loop and the next one look terribly alike. Could you refactor them and reduce the amount of duplicated code ?

590

This if is unecessary, EINA_LIST_FREE will not iterate on an empty list. It is safe and simpler to remove it.

src/lib/elementary/efl_page_transition_scroll.eo
4

A little bit more comment on this class and the below methods.

src/lib/elementary/efl_ui_pager.eo
11

A little bit more documentation describing the purpose of this class and what it should be used for.

13

Is that line necessary ?

37

In general every property needs more documentation, but this need some documentation for sure :-)

This revision now requires changes to proceed.Mar 6 2018, 5:26 PM

I added some inline comments. Please check them.

data/elementary/themes/default.edc
78

I don't think this is needed because there is not pager for elm legacy.

data/elementary/themes/edc/elm/pager.edc
1 ↗(On Diff #13909)

Let's move this pager.edc to "data/elementary/themes/edc/efl/"

src/bin/elementary/test_efl_ui_pager.c
126

Please modify your test codes to use new EFL_UI_XXX classes instead of elm_xxx.
Currently, we don't have a plan to support mixed use between them.

eunue marked 16 inline comments as done.Mar 15 2018, 6:13 AM
eunue added inline comments.
src/lib/elementary/efl_page_transition_scroll.c
45

you're so right

475

one loop adds items and the other removes items.. they can not be merged.

eunue updated this revision to Diff 13935.Mar 15 2018, 6:18 AM
eunue marked 2 inline comments as done.

Fixed codes according to @cedric's feedback.
@woohyun's comments will be done soon.

cedric requested changes to this revision.Mar 15 2018, 10:34 PM
cedric added inline comments.
src/lib/elementary/efl_page_indicator_icon.c
26

Can you factorize the code in each side of the if to be one function with different parameter ?

59

In all the function below you have a lot of code in common, could you refactorize it in one function with a different set of parameter ?

178

It is better practice to actually call the destructor after destroying the items just in case they do use the parent object in some form. Actually, I think you could use efl_object_invalidate for this step instead of the destructor.

src/lib/elementary/efl_page_transition_scroll.c
475

It wasn't obvious because they are so long and duplicate code themself. Could you move some of the inner code of the function into its own function and reuse them ? for both loop.

570

This entire function could actually be moved to efl_object_invalidate.

src/lib/elementary/efl_ui_pager.c
230

You could use EFL_EVENT_ANIMATOR_TICK on the object itself.

This revision now requires changes to proceed.Mar 15 2018, 10:34 PM
eunue updated this revision to Diff 13946.Mar 21 2018, 1:44 AM

This patch removes legacy theme and refactors sample code.

eunue marked 3 inline comments as done.Mar 21 2018, 1:46 AM
eunue added a comment.Apr 6 2018, 1:59 AM

@cedric @woohyun I will finish fixing the code according to the feedback by next week.
Besides the miscellaneous issues, I need your opinion on major concepts.
Please look through the structure and let me know if you have any concerns.
I have one!

The following code is the current version of pager sample.

Efl_Page_Transition *tran;

tran = efl_add(EFL_PAGE_TRANSITION_SCROLL_CLASS, pager);
efl_ui_pager_transition_set(pager, tran);
efl_page_transition_scroll_side_page_num_set(trans, ...)
efl_ui_pager_indicator_set(pager, EFL_PAGE_INDICATOR_ICON_CLASS);

We can "attach" transition or indicator object, but they are done differently.
I think they need to be in the same form.
The latter is what jp suggested.
The former is to handle transition object's own function call.
For now, indicator doesn't have any property to set, but we can always add some in the future.

So, which one seems better?

A

tran = efl_add(TRANSITION, pager);
efl_ui_pager_transition_set(pager, tran);

ind = efl_add(INDICATOR, pager);
efl_ui_pager_indicator_set(pager, ind);

B

efl_ui_pager_transition_set(TRANSITION, pager);
tran = efl_ui_pager_transition_get(pager);

efl_ui_pager_indicator_set(INDICATOR, pager);
ind = efl_ui_pager_indicator_get(pager);
woohyun added a comment.EditedApr 8 2018, 10:10 PM

@eunue

I don't know whether B is doable.
To support with B concept, we need to give description what types(or classes) are available for transition_set(or indicator_set).

Plus, we need to support transition_get(or indicator_get) which would return internally created class instance. (I don't know whether there is similar class supports in efl)

So, I think A is proper.

@cedric
How do you think about this ?

@cedric
How do you think about this ?

I agree with you, feel like A is the way to go.

zmike removed a child revision: D5847: efl_ui_tab_pager.
zmike added a subscriber: zmike.May 1 2018, 2:22 PM

I also vote A. Has there been any progress on this?