This is the first patch introducing efl_ui_pager into elementary.
See T5317
Details
Details
- Reviewers
jpeg woohyun Jaehyun zmike - Maniphest Tasks
- T5317: Add a tab/pageview widget
elementary_test -to pager
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| src/lib/elementary/efl_page_transition.c | ||
|---|---|---|
| 48 | This looks very much like an interface. Or @pure_virtual functions at least. And the constructor should not be there if it's empty. | |
| src/lib/elementary/efl_page_transition.eo | ||
| 9 | Not Efl.Object but Efl.Ui.Pager or whatever you really want. | |
| 14 | This is not necessary. After bind the transition object should be able to listen to geometry (resize/move) events on its target. | |
| src/lib/elementary/efl_page_transition.h | ||
| 14 | This entire file is not required... | |
| src/lib/elementary/efl_page_transition_scroll.c | ||
| 16 | Please use the existing functions in eina. Use Eina_Rect or Eina_Rectangle internally. | |
| 65 | eina_rectangles_intersect(&pd->rect, &pi->rect) | |
| 125 | t = abs(pd->move) | |
| 129 | EINA_DBL_EQ | |
| 155 | with Eina_Rect you can then do position_set(obj, pi->rect.pos) | |
| 183 | no need for !! here :) | |
| src/lib/elementary/efl_page_transition_scroll.h | ||
| 10 | I think all the viewport information should be here. Including clippers and geometry. | |
| src/lib/elementary/efl_ui_pager.c | ||
| 220 | not useful | |
| 231 | pi = calloc(1, sizeof(*pi)) | |
| 233 | ouch. please make the code dynamic... was is this arbitrary value of 4? | |
| 269 | please don't use group_add in new widgets... constructor and finalize should do just fine | |
| 309 | same remark for group_del | |
| 334 | calloc, no cast, sizeof(*pi) | |
| 451 | fixme: should be stack above | |
| 609 | maybe this interface should return false? (just wondering your opinion) | |
| 621 | this should use the same type as other sizes: Eina.Size2d | |
| 641 | i wonder if padding should be scalable or not? and maybe pass a bool flag to say scalable or not | |
| 681 | loop might need to be an enum:
| |
| src/lib/elementary/efl_ui_pager.eo | ||
| 34 | size2d | |
| 59 | called for every change when dragging? or when the page changed? | |
| src/lib/elementary/efl_ui_widget_pager.h | ||
| 47 | specific to the transition | |
Comment Actions
@jpeg please briefly look through this and let me know if this is "similar" to what you expected.
Comment Actions
Okay this is hard to review somehow Phab doesn't give me context :(
"packed" event belongs to the efl.pack interface. It should also have the packed object as event info, definitely not some private data.
| src/lib/elementary/efl_page_transition_scroll.c | ||
|---|---|---|
| 252 | EINA_DBL_EQ(move, 0.) | |
| 413 | Well that's not possible. Efl_Ui_Pager_Data is a private struct. | |
| src/lib/elementary/efl_ui_pager.c | ||
| 410 | No no no you can't use a private data struct as event info, how would apps use it? | |
| src/lib/elementary/efl_page_transition.c | ||
|---|---|---|
| 48 | It remains as a class. I added some common code in _efl_page_transition_bind(). | |
| src/lib/elementary/efl_page_transition.eo | ||
| 9 | what about circular reference? build fails with Efl.Ui.Pager... | |
| src/lib/elementary/efl_page_transition.h | ||
| 14 | I moved the common data for efl_page_transition's subclasses to here. | |
| src/lib/elementary/efl_page_transition_scroll.c | ||
| 16 | I chose Eina_Rectangle to use eina_rectangles_intersect().. but Eina_Rectangle can't be passed as a parameter of efl_gfx_geometry_set() ... hmm. | |
| 125 | pd->move is a double.. | |
| src/lib/elementary/efl_ui_pager.c | ||
| 609 | i'll think about it | |
| 641 | we'll discuss this off-line | |
| src/lib/elementary/efl_ui_pager.eo | ||
| 59 | "updated" should remain internal.. I'll add page change event! | |
Comment Actions
This breaks distcheck. Please add test_pager to EXTRA_DIST. Apply this patch:
diff --git a/data/elementary/objects/Makefile.am b/data/elementary/objects/Makefile.am index 1c98813325..a7c5ee2abf 100644 --- a/data/elementary/objects/Makefile.am +++ b/data/elementary/objects/Makefile.am @@ -6,6 +6,7 @@ elementary/objects/test_external.edc \ elementary/objects/test_masking.edc \ elementary/objects/test_prefs.edc \ elementary/objects/test_prefs.epc \ +elementary/objects/test_pager.edc \ elementary/objects/multip.edc \ elementary/objects/cursors.edc \ elementary/objects/font_preview.edc \
Comment Actions
ewww... fixed it.
on my side, distcheck failed as follows:
| configure: error: Systemd dependency requested but not found |
I configured with --disable-systemd option,
maybe I can't see the right error message with --disable-systemd option?
Comment Actions
If I remember correctly, Ubuntu doesn't use systemd by default, so it's ok if you disable it.
Comment Actions
If loop is OFF, I shouldn't see any page on the left of the first page. Or on the right of the last page.
Comment Actions
Do you have any idea why scrolling feels so incredibly slow? I get a lot of tearing here, it's quite odd.
Comment Actions
yes, right. I only pushed codes blocking the event and didn't push the displaying part.. it's coming~!
actually I'm thinking about how pages should look like while loop is disabled, cause i'm not sure the "proxy" thing will work everytime...
what if a pager displays 5 pages in the viewport and user sets only 2 pages in it? then should it look like
[Page1] [Page2] [Page1] [Page2] [Page1] ...?
Comment Actions
This feature very much needs a good test case, with real content (that has a min size), not just simple edje objects.
Same question with a single page :)
Yes, as we discussed, the proxy can be a future option. I think you need an enum in loop instead of bool.
| src/lib/elementary/efl_page_transition.eo | ||
|---|---|---|
| 9 | How does it fail? It shouldn't fail. Circular refs are OK. | |
| src/lib/elementary/efl_ui_pager.eo | ||
| 11 | mark as @nonull | |
| 25 | This needs documentation and a test case. Otherwise don't add the API. Does it actually make sense in the general case or only with the scroll page transition? | |
| 40 | there's already padding in pack api. but it's 2d. what about scaling here? | |
Comment Actions
maybe because i didn't consider optimization yet...? ^^;;
I'll visit you and see what you're seeing : ]
Comment Actions
You might want to compile with -Wfloat-equal.
| src/lib/elementary/efl_ui_pager.c | ||
|---|---|---|
| 44 | EINA_DBL_EQ | |
| 68 | Why this choice here? | |
| 89 | EINA_DBL_EQ | |
| 231 | Any reason for using legacy callbacks? Besides just habits... | |
| 260 | i think this should call the evas intercept function. look at other implementations of size set | |
| 274 | same here | |
| 363 | :)) | |
| 433 | why don't you use Eina_Size2d internally? | |
| 443 | you might want to verify size >= 0 (or > 0?) | |
| src/lib/elementary/efl_page_transition.eo | ||
|---|---|---|
| 9 | I tried everything (?) and still get this.. In file included from ../../src/lib/elementary/Elementary.h:287:0, | |
Comment Actions
I'm fixing your inline comments
no, not the comments, the code.
and besides simple code fixing, I want to talk about the concept and logic soon.. because I think it can be much better than now.. !
Comment Actions
For circular dependency, you have to define the type in C, otherwise the C code can not compile. But eolian doesn't complain. Example in Ector_GL.h:
#ifndef _ECTOR_GL_SURFACE_EO_CLASS_TYPE #define _ECTOR_GL_SURFACE_EO_CLASS_TYPE typedef Eo Ector_Cairo_Surface; #endif
| src/lib/elementary/efl_ui_pager.c | ||
|---|---|---|
| 68 | just randomly chose one option before opening this as an API. how do you think? | |
| src/lib/elementary/efl_ui_pager.eo | ||
| 25 | This "scroll" means scrolling interaction, so it is a general property. | |
| 40 | I'll fix this later... I'm not sure whether this should remain as a common property or not.. | |
Comment Actions
@cedric
I think @eunue is handling this work in https://phab.enlightenment.org/D5832.
So we can abandon this contribution.
@eunue
After checking my opinion, please remove this contribution for better commit managing :)
Comment Actions
@cedric
Please see https://phab.enlightenment.org/D5832 for the latest version.
I'll abandon this revision after I check if there's anything to consider from the comments.