This is the first patch introducing efl_ui_pager into elementary.
See T5317
Lint Skipped |
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 |
@jpeg please briefly look through this and let me know if this is "similar" to what you expected.
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! |
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 \
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?
If I remember correctly, Ubuntu doesn't use systemd by default, so it's ok if you disable it.
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.
Do you have any idea why scrolling feels so incredibly slow? I get a lot of tearing here, it's quite odd.
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] ...?
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? |
maybe because i didn't consider optimization yet...? ^^;;
I'll visit you and see what you're seeing : ]
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, |
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.. !
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.. |
@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 :)
@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.