This patch adds a new class efl_ui_pager and related classes.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
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 :-) | |
Comment Actions
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. | |
| 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. | |
Comment Actions
@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);
Comment Actions
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 ?