Page MenuHomePhabricator

efl_ui_pager: add a new class
AbandonedPublic

Authored by eunue on Nov 6 2017, 11:52 PM.

Details

Summary

This is the first patch introducing efl_ui_pager into elementary.

See T5317

Test Plan

elementary_test -to pager

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
eunue created this revision.Nov 6 2017, 11:52 PM
jpeg edited the summary of this revision. (Show Details)Nov 7 2017, 12:43 AM
jpeg added projects: efl, Restricted Project.
jpeg added inline comments.Nov 7 2017, 3:01 AM
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)
also you should combine these two lines anyway with geometry_set

183

no need for !! here :)
but the next line is called on NULL maybe

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))
safer and cleaner

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
probably not needed after all

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:

  • no_loop
  • loop (if possible)
  • loop_mirror (use proxies if not enough items to loop)
src/lib/elementary/efl_ui_pager.eo
34

size2d

59

called for every change when dragging? or when the page changed?
i think there is a need for a page change event (once the transition is complete)

src/lib/elementary/efl_ui_widget_pager.h
47

specific to the transition

eunue updated this revision to Diff 12933.Nov 8 2017, 10:23 PM

@jpeg please briefly look through this and let me know if this is "similar" to what you expected.

jpeg added a comment.Nov 9 2017, 6:21 PM

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?

eunue marked 17 inline comments as done.Nov 15 2017, 12:40 AM
eunue added inline comments.
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!

eunue updated this revision to Diff 13039.Nov 16 2017, 1:50 AM
eunue marked an inline comment as done.
jpeg added a comment.Nov 16 2017, 5:58 PM

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 \
eunue added a comment.Nov 16 2017, 7:13 PM

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?

jpeg added a comment.Nov 16 2017, 8:11 PM
In D5430#93883, @eunue wrote:

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.

jpeg added a comment.Nov 16 2017, 10:11 PM

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.

jpeg added a comment.Nov 16 2017, 10:18 PM

Do you have any idea why scrolling feels so incredibly slow? I get a lot of tearing here, it's quite odd.

In D5430#93894, @jpeg wrote:

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.

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] ...?

jpeg added a comment.Nov 16 2017, 10:47 PM

This feature very much needs a good test case, with real content (that has a min size), not just simple edje objects.

In D5430#93898, @eunue wrote:
In D5430#93894, @jpeg wrote:

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.

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] ...?

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?

In D5430#93895, @jpeg wrote:

Do you have any idea why scrolling feels so incredibly slow? I get a lot of tearing here, it's quite odd.

maybe because i didn't consider optimization yet...? ^^;;
I'll visit you and see what you're seeing : ]

jpeg added a comment.Nov 17 2017, 12:21 AM

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?)

eunue added inline comments.Nov 17 2017, 12:36 AM
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,
from ../../src/lib/elementary/elc_fileselector.c:15:
../src/lib/elementary/efl_page_transition.eo.h:32:46: error: unknown type name 'Efl_Ui_Pager'; did you mean 'Efl_Ui_Panes'?
EOAPI void efl_page_transition_bind(Eo *obj, Efl_Ui_Pager *pager);

In D5430#93907, @jpeg wrote:

You might want to compile with -Wfloat-equal.

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.. !

jpeg added a comment.Nov 19 2017, 6:51 PM

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
eunue updated this revision to Diff 13118.Nov 21 2017, 6:45 AM
eunue marked 9 inline comments as done.
eunue added inline comments.
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..

jpeg added a comment.Jan 21 2018, 6:59 PM

@eunue where are we on this?

@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 :)

eunue added a comment.Mar 26 2018, 1:26 AM

@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.

zmike requested changes to this revision.May 2 2018, 4:52 PM
zmike added a subscriber: zmike.

This patch landed in an alternate form.

This revision now requires changes to proceed.May 2 2018, 4:52 PM

@eunue
Could you abandon this patch ?

eunue abandoned this revision.May 3 2018, 7:13 PM