Page MenuHomePhabricator

efl_ui_scrollable: apply scroll_hold_push/pop and scroll_freeze_push/pop
ClosedPublic

Authored by eagleeye on Feb 7 2018, 4:02 AM.

Details

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
eagleeye created this revision.Feb 7 2018, 4:02 AM
eagleeye requested review of this revision.Feb 7 2018, 4:02 AM

I am not a fan of this code as it is very error prone. Could it be possible to have an internal helper that does the test and dispatching instead ?

cedric requested changes to this revision.Feb 14 2018, 11:27 AM
This revision now requires changes to proceed.Feb 14 2018, 11:27 AM

@cedric
I need to handle this problem by myself because @eagleeye is in education for a month.
Could you decribe what are the error prone codes ?

I will fix them as soon as possible :)

cedric added a comment.Mar 6 2018, 5:45 PM

Well this patch duplicate :

if (elm_widget_is_legacy(obj))
  elm_interface_scrollable_hold_set(obj, EINA_TRUE);
else
  efl_ui_scrollable_scroll_hold_set(obj, EINA_TRUE);

Putting that into an internal function helper would make this patch simpler and less error prone. Additional question, shouldn't we make elm_interface_scrollable_hold_set deprecated internally to make sure to not use it directly ? We can avoid the warning by having a #ifdef test that trigger a different function prototype (with and without deprecation) depending where it is included.

I don't know if there is difference between my code and code with internal function.
Also 'efl_ui_scrollable' and 'elm_interface_scrollable' exist separately.(elm_interface_scrollable.c and efl_ui_scroll_manager.c) Therefore legacy scrollable widgets use 'elm_interface_scrollable' and new efl widgets use 'efl_ui_scrollable'.
So I think to deprecating 'elm_interface_scrollable' is too much effort. (A lot of codes will be modified)

@cedric Please check my comment. After that, I will proceed next step. Thanks :)

@cedric
I think internal things could be arranged later.
After closing this task, @eagle001 needs to apply new efl_ui_scrollable to efl_ui_xxx classes.
(not for elm_xxx class which would be covered by elm_interface_scrollable. I don't think replacing elm_interface_scrollable to efl_ui_scrollable is urgent)

So, I hope to continue this discussion after applying current version of this patch.
Because it is giving several serious bugs now.
(For example, efl_ui_slider in vertical efl_ui_scroller is moving up and down - while changing its value by left and right moving.)

I wish you also agree with my opinion.

@eagleeye

Please remove undefined properties and methods from Efl.Ui.Slider in test_ui_scroller.c (e.g. efl_ui_slider_part_indicator_visible_mode_set)

eagleeye updated this revision to Diff 14651.May 17 2018, 1:27 AM

rebase commit

Jaehyun_Cho accepted this revision.May 17 2018, 2:17 AM
This revision was not accepted when it landed; it landed in state Needs Review.May 17 2018, 2:18 AM
Closed by commit rEFLec670e9a22f0: efl_ui_scrollable: apply scroll_hold_push/pop and scroll_freeze_push/pop (authored by Hosang Kim <hosang12.kim@samsung.com>, committed by Jaehyun_Cho). · Explain Why
This revision was automatically updated to reflect the committed changes.