Page MenuHomePhabricator

interface_scrollable: refactoring interface_scrollable
Open, NormalPublic

Description

I found some redundant code blocks, unnessacery recursive logics and optimize way for interface_scrollable
so that I want to know nice opinions from community.

I made a.k.a summary diagram as below. and I am going to introduce it.

  1. pan_changed_cb

    because bar is closely related with pan and also content, calculation related with bar should take place iniside pan_changed_cb. however, bar_size_adjust on pan_changed_cb will be invoked only when content is changed. it should be called when position/size of pan is changed, not only size of content is changed.
  1. redundancy of bar_size_adjust

    bar_size_adjust is called from reconfigure and also pan_changed_cb now. but both edje_resize and edje_move have nothing to do with bar, and also pan_smart_resize(which invoke pan_changed_cb) will be invoked soon so that I think we can cut the way from reconfigure.
  1. Invalid internal state while calculating

    In case of scroller resizing, edje resize/pan_smart_resize/content_resize will be called in order. however calculations of both bar size or page size on the edje resize could not be valid because size of pan or content could be changed. I think we have to postpone calculating of xxx_adjust once calculations are done so that we can calculate just once with valid states.
  1. Invalid logic

    elm_obj_pan_pos_get(sid->pan_obj, &px, &py); if (vx != mx) x = px; if (vy != my) y = py; elm_obj_pan_pos_set(sid->pan_obj, x, y);

    There is the logic like above in bar_size_adjust, but I don't really think the logic is reasonable. vx/y are double values of drag and mx/y are int values of pan_pos_max. The statement which compares ratio and pixel looks invalid for me. It will be false when content smaller than viewport (v==0.0 m==0px) or set position of pan which is a just 1px larger than content to the bottom.(v==1.0, m==1px) In almost case, this logic work like setting pan pos by current pan pos except really really rere case like above. That's the reason why I think we can remove it.

I can make result as below.
In this scenario, calculations are reduced to 1/20 in maximum case.
I am going to write a patch for this task.

akanad created this task.Feb 17 2017, 12:49 AM
akanad updated the task description. (Show Details)Feb 17 2017, 2:19 AM
jpeg added a subscriber: jpeg.Feb 19 2017, 6:19 PM
jpeg triaged this task as Normal priority.Feb 19 2017, 10:26 PM
jpeg added subscribers: stefan_schmidt, cedric.

See D4671. Adding @cedric and @stefan_schmidt. I would like to merge the patch to get it tested. Not sure the timing to do so is right, though (with the upcoming release)

I briefly looked over the code and descriptions now. To me this seems to be a pure optimization and now bug fixed or did I miss something here?
Getting this in would be great, but after 1.19 is out. That also allows more time for testing results from Tizen coming back.

jpeg assigned this task to akanad.Feb 21 2017, 1:22 AM
jpeg added a project: efl.

this patch has been merged into master branch of efl on tizen project.
Testers are intensively working on it because it is time to prepare for mass production.
And as far as I know, there is no critical issue : )

However, I found some bug cases with this patch on mirrored mode.
I will post summary of test result here next week.

akanad added a comment.Mar 8 2017, 1:23 AM

It has been a couple of weeks since the patch was merged into efl repo for mass production.
However, any problems are not ocurring on the intensive testing.

As I said, there is a bug on mirrored mode so that I wrote a ugly patch for it.
And I want to discuss about the cause of the bug and also how to improve it on the community too.

if you want to see the patch, please refer to the attachment.

raster added a subscriber: raster.Mar 9 2017, 1:24 AM

i need to look into this (it's marked for review) but.,.. just because "it's been merged into tizen and is not having problems" does not mean it doesn't have problems... there are enough patches in tizen that are full of problems, just that tizen doesn't see them by luck... :) so that's never a good REASON to accept. it's a nice datapoint for QA... but a review should happen. :)

akanad added a comment.Mar 9 2017, 2:24 AM

I do not think that the patch(D4671) is valid or fully aceptable just because it has no critical error on tizen platform at all, as you can see previous comments on this thread.
I am just telling the result of testing that someone might wait for. that's it.
The reason why I attached the patch for tizen is for letting community know that there could be another things we should discuss. I want you to understand my intent.

oh sure.. no problem. just wanting to say that dont take "i found no bug" as a "its ready" :) :)

zmike edited projects, added Restricted Project; removed efl.Jun 11 2018, 6:58 AM
bu5hm4n edited projects, added efl: widgets; removed Restricted Project.Jun 11 2018, 9:16 AM
zmike added a subscriber: zmike.Jan 16 2019, 9:31 AM

Is something happening here or ?