Page MenuHomePhabricator

scroll_manager: new scoller scratch
ClosedPublic

Authored by eagleeye on Sep 25 2017, 4:59 AM.

Details

Summary

scrollable widgets had a interface_scrollable as a mixin so that the widgets had a 'is-a' relation with interface_scrollabe.
however, new scroller concept don't have 'is-a' relationship, but 'has-a' relationship.
scrollable widgets should have a scroll manager inside them, then scroll manager handles event from user and api implementations.
and also we cut the features such as paging because there will be aka 'elm_pager'.

we are expecting that the new concept make us to maintain the scroller easier.
please excuse for many unorganized code and logics. : (

[contained commit]
scrollable: add efl_ui_scroller example
scrollable: refactoring for behavior in case of multiple scroller
scrollable: remove repetitive scrollbar code.
scrollable: combine calculating bounce distance code.
scroll_manager: mouse up function refactoring
scroll_manager: mouse move function refactoring
scroll_manager: warp animator wip
scroll_manager: fix denominator value when calculating flicking behavior.
Fix to disconnect bounce animator once animation is done
gather duplicated animator drop logics
gather duplicated conditions
Rearrange prototypes and append comment
Add manipulate functions for animators
scroll_manager: change member_add function.
scroll_manger: apply mirroring logic
scroll_manager: apply scrollbar
apply API to scroller widget
scroll_manager: apply scroll event callback
Change logics for all about scroll animating
efl_ui_pan: add efl_ui_pan
scrollable: change content_min_limit to match_content
scroll theme: apply overlapped scrollbar

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
eagleeye marked an inline comment as done.Nov 5 2017, 11:19 PM

update doc

eagleeye updated this revision to Diff 12926.Nov 7 2017, 11:41 PM

apply new scroller to Efl.Ui.Image.Zoomable

jpeg requested changes to this revision.Nov 9 2017, 6:26 PM

This patch is now very old. Can't rebase on master.

This revision now requires changes to proceed.Nov 9 2017, 6:26 PM
jpeg added a comment.Nov 9 2017, 7:29 PM

Almost none of my earlier comments have been addressed yet. This patch needs to be rebased on master, as well.
Here's the checklist:

  • make
  • make check
  • make examples
  • make distcheck
  • make terminology
  • elementary test must have proper test cases
  • everything needs to run fine
src/lib/efl/interfaces/efl_ui_scroll_bar.eo
9 ↗(On Diff #12926)

It's more like a direction.

15 ↗(On Diff #12926)

This is still inconsistent between scrollbar and scroll_bar. Pick one. Scroll.bar / Scroll_Bar is fine. The enums above could be Scroll.Bar_Mode and .Bar_Type.
Or chose Scrollbar everywhere but then it's not consistent with Scroll.Interactive :)

17 ↗(On Diff #12926)

Why this? no i don't think you want this

18 ↗(On Diff #12926)

prefix should not be required

src/lib/efl/interfaces/efl_ui_scroll_interactive.eo
5 ↗(On Diff #12926)

probably efl_ui_scroll for events

6 ↗(On Diff #12926)

Why do we have Scrollable and Scroll?

src/lib/elementary/efl_ui_image_zoomable.c
147

I think the event info should be in a struct here. Better for bindings.

206

Just

efl_ui_scrollable_show(sd->manager, sd->show);
735–737

code style. view should be declared at the beginning of a block

936

code style

1086–1087

code style

1148

style

src/lib/elementary/efl_ui_pan.eo
15

no prefix here

17

pan_position

35

pan_position_min

60

This is lacking the event info type declaration. As in other comments I think this is a struct, defined in this file. But it must be well defined.

src/lib/elementary/efl_ui_scroll_manager.eo
24

why not protected?
what happens if it's set to null?

32

why are bar properties here? there is a whole scrollbar class as well?
also what are the units?

49

Questions unanswered. Also, why do we need these apis here when there's a dedicated scrolbar eo file/

72

why aren't all these in scroll.bar??

eagleeye updated this revision to Diff 12949.Nov 9 2017, 9:05 PM

Rebase and fix build warning.

jpeg added a comment.EditedNov 10 2017, 12:14 AM

First impressions: this has not been designed for desktop use, because the scrollbars disappear after scrolling, and there is no way to make them appear (by going to the edges of the scroll region, for instance).

We absolutely need test cases, many. Basically the same test cases as Scroller has (at least Scroller 1 and 2 should be there). I already gave many review comments, so I'll wait for the next update.

eagleeye updated this revision to Diff 12992.Nov 14 2017, 2:36 AM
eagleeye marked 14 inline comments as done.

Update code

eagleeye updated this revision to Diff 12993.Nov 14 2017, 2:43 AM

Rebase commit

Changes:
revert scroller base theme - It is designed for desktop use now :)
add hidden_bar style theme and EFL_UI_SCROLLBAR_MODE_HIDDEN - It is designed for mobile or tablet use.
change name efl_ui_scroll_bar to efl_ui_scrollbar and efl_ui_scroll_interactive to efl_ui_scrollable_interactive.
add test efl ui scroller 1 & 2 - first one is default scroller and second one is applied hidden_bar style.
move property related bar to Efl.Ui.Scrollbar interface.
separate EFL_UI_PAN_EVENT_CHANGED event.

src/lib/efl/interfaces/efl_ui_scrollable.eo
2

I deleted Efl.Ui.Scroll.Single.Direction

src/lib/elementary/efl_ui_scroller.c
128–129

I wanna break relationship between "scroll manager" and "edje".
Because we will be able to create scrollable widget without edje

This comment was removed by eagleeye.
jpeg requested changes to this revision.Nov 15 2017, 12:30 AM

make distcheck fails:

../../../src/lib/elementary/efl_ui_scroll_manager.c:7:10: fatal error: efl_ui_widget_scroll_manager.h: No such file or directory
 #include "efl_ui_widget_scroll_manager.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[5]: *** [Makefile:36072: lib/elementary/lib_elementary_libelementary_la-efl_ui_scroll_manager.lo] Error 1
make[5]: *** Waiting for unfinished jobs....
../../../src/lib/elementary/efl_ui_scroller.c:9:10: fatal error: efl_ui_widget_scroller.h: No such file or directory
 #include "efl_ui_widget_scroller.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[5]: *** [Makefile:36065: lib/elementary/lib_elementary_libelementary_la-efl_ui_scroller.lo] Error 1
../../../src/lib/elementary/efl_ui_pan.c:7:10: fatal error: efl_ui_widget_pan.h: No such file or directory
 #include "efl_ui_widget_pan.h"
          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
This revision now requires changes to proceed.Nov 15 2017, 12:30 AM
jpeg added a comment.Nov 15 2017, 12:35 AM

I wonder what to do about the hidden_bar thing. It seems like this is an option, maybe platform-dependent (just theme work? or elm_config? or elm_config+API to choose?).
make distcheck fails because some files have not been added to Makefile.am
We need a better test case. Look at elementary_test -to "scroller 2" for an example. Scroller inside a scroller is a must. Also have some real content, with focus (for on focus show, etc...).

Fix distcheck and add some real test case. I want to merge this patch soon.

src/bin/elementary/test_scroller.c
1054

Not a fan of this... style_set should not be used by apps like this.

eagleeye updated this revision to Diff 13204.Nov 27 2017, 11:17 PM

add better example for Efl.Ui.Scroller

eagleeye updated this revision to Diff 13216.Nov 28 2017, 3:51 AM

fix build warning.

I need more time to consider "hidden_bar":)

And I will update focus and key_action_move features.

jpeg added a comment.Dec 10 2017, 6:38 PM

fix build warning.

I need more time to consider "hidden_bar":)

And I will update focus and key_action_move features.

Okay so... does this mean we should review this or is it still work in progress? I've been waiting for another update but nothing in two weeks.

eagleeye updated this revision to Diff 13445.Dec 11 2017, 1:54 AM

Removing hidden_bar, because it is platform-dependant feature.

Please review this patch.

jpeg added a comment.Dec 11 2017, 2:59 AM

This is mostly good!

Just a few comments.
Also, the test case does not work perfectly:

  1. Wheel scroll is not smooth when going slowly, but smooth when going fast.
  2. Scroll down in the scroller-inside-the-scroller does not move the outer scroller. Up somehow works.

Try to fix some of the warnings. I use these CFLAGS with clang:

-g -ggdb3 -O0 -W -Wall -Wextra -Wshadow -Wimplicit-fallthrough -Wno-parentheses-equality -Wno-missing-field-initializers -Wuninitialized

I like the test cases! Photocam behaves very very nicely! :)

src/lib/efl/interfaces/efl_ui_scrollable_interactive.eo
50

maybe scroll_freeze?

65

maybe scroll_hold?

68

not clear? if i remember correctly, hold is for scrolling only when selecting text?

78

this name can not be used. it conflicts with Efl.Loop.User.loop

89

why both freeze and block?

148

Unlike @.show

158

Do we need 2 functions? Or just an optional bool flag to enable/disable animation?

src/lib/efl/interfaces/efl_ui_scrollbar.eo
30

name conflicts with efl.gfx.size
scrollbar_size would be ok
also... please document :)

38

same comment as above

48

I'm not a fan. Make this at least @protected maybe even @beta

58

indentation :P and doc

jpeg added a comment.Dec 11 2017, 3:00 AM

Finally, do you have a branch for these patches? Or this just 1 huge patch?

jpeg added inline comments.Dec 11 2017, 3:47 AM
src/lib/elementary/efl_ui_scroll_manager.eo
16

This should be Efl.Ui.Base.mirrored

eagleeye updated this revision to Diff 13466.EditedDec 12 2017, 4:59 AM

fix build warning with clang.
add scroll,start and scroll,stop signal.
fix wheel event bug.

Oh I missed new comment:) I will update it tommorow.

eagleeye updated this revision to Diff 13500.EditedDec 13 2017, 9:55 PM

Change eo property name.

Block api has some options, blocking X, blocking Y and blocking both.
Add these options to freeze api, is this a good way?:)

jpeg added a comment.Dec 14 2017, 6:50 PM

Looks mostly good thought I can spot some bugs.
Scroll with the mouse wheel, down, in the main scroller inside Efl.Ui.Scroller test case.

if (!_elm_config->thumbscroll_bounce_enable || !sd->bounce_vert)
  {
     if (ny <= min.y) no_bounce_y_end = EINA_TRUE;
     if (!sd->loop_v && (ny - min.y) >= max.y) no_bounce_y_end = EINA_TRUE;
  }

no_bounce_y_end will be wrongly set to true when scrolling down as ny == min.y for the first frame (after double to int rounding).

jpeg added a comment.Dec 14 2017, 6:50 PM

Where is the branch with all the patches? Or is it a single patch?

jpeg added a comment.Dec 14 2017, 6:56 PM

efl_ui_scrollable_show does not exist. In fact Scrollable has no methods, they're only in Scrollable.Interactive. Shouldn't scrollable have the properties for content_pos and such?

eagleeye updated this revision to Diff 13521.Dec 14 2017, 11:39 PM

fix bug and doc.

eagleeye updated this revision to Diff 13541.Dec 17 2017, 11:01 PM

add group calculate for scrolling.

jpeg added a comment.Dec 18 2017, 4:30 AM

I think this is good now!
Just one question: do the legacy APIs for elm_object_scroll_freeze/hold work with this?

jpeg accepted this revision.Dec 18 2017, 4:33 AM

I think I will merge this soon. Just not tonight as I'm going home :)

This revision is now accepted and ready to land.Dec 18 2017, 4:33 AM
Closed by commit rEFL47bf356435d7: scroller: Introducing Efl.Ui.Scroller (authored by WhiskyKiloSq, committed by Jean-Philippe Andre <jp.andre@samsung.com>). · Explain WhyDec 18 2017, 9:11 PM
This revision was automatically updated to reflect the committed changes.