Page MenuHomePhabricator

efl_ui_scrollbar: change scrollbar state when theme is reloaded.
Needs RevisionPublic

Authored by eagleeye on Tue, Nov 12, 3:41 AM.

Details

Summary

Scrollbar visible state is not synchronized when theme is reloaded.

Test Plan

launch elementary_test -> collection view or list widget

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14839
Build 10217: arc lint + arc unit
eagleeye created this revision.Tue, Nov 12, 3:41 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

eagleeye requested review of this revision.Tue, Nov 12, 3:41 AM
eagleeye edited the test plan for this revision. (Show Details)Tue, Nov 12, 3:42 AM
eagleeye added a reviewer: SanghyeonLee.
eagleeye added reviewers: akanad, zmike.
segfaultxavi requested changes to this revision.Tue, Nov 12, 4:40 AM
segfaultxavi added a subscriber: segfaultxavi.

efl_ui_scrollbar_visible_get() looks like a useful feature. Why don't we move it to efl_ui_scrollbar.eo so it is also available to other languages?

Something like:

@property visibility {
   [[Current visibility state of the scrollbars.
     This is useful in @Efl.Ui.Scrollbar_Mode.auto mode where EFL decides if the scrollbars
     are shown or hidden. See also the @[.bar,show] and @[.bar,hide] events.
   ]]
   get {}
   values {
      hbar: bool; [[Whether the horizontal scrollbar is currently visible.]]
      vbar: bool; [[Whether the vertical scrollbar is currently visible.]]
   }
}
This revision now requires changes to proceed.Tue, Nov 12, 4:40 AM

@segfaultxavi
isn't it too overlapped with bar_mode set?
we already have EFL_UI_SCROLLBAR_MODE in both axis,
with a AUTO(means if content is bigger than viewport, scrollbar shows, if smaller then viewport, hidden), ON, and OFF.

As the documentation I proposed says, when MODE is AUTO, the user cannot know if the scrollbar is currently shown or not, right? This is what efl_ui_scrollbar_visible_get() does.

As the documentation I proposed says, when MODE is AUTO, the user cannot know if the scrollbar is currently shown or not, right? This is what efl_ui_scrollbar_visible_get() does.

they can calculate it but, in that purpose,
I think it may helpful.
I agree on that :)

eagleeye updated this revision to Diff 27032.Thu, Nov 21, 3:06 AM

add bar_visibility property.

@segfaultxavi thank you for your review.

segfaultxavi requested changes to this revision.Fri, Nov 29, 1:39 AM

Very nice! Builds and passes tests.

Now, if I can ask for just another thing... Unit Tests!
Just put a small object inside a scroller with Scrollbar_mode.auto and check that the bars are invisible, then put a bigger object and check that the bars are visible, and combine with theme reloading.

This revision now requires changes to proceed.Fri, Nov 29, 1:39 AM
eagleeye updated this revision to Diff 27331.Thu, Dec 5, 2:14 AM

add unit tests

segfaultxavi added a subscriber: bu5hm4n.

The test looks correct but it makes the test suite fail. Did you check it?
I would make the timer much much smaller (otherwise the whole test take a long time to run).
I think there is a way to make the loop quit after one iteration, so you don't need a timer... @bu5hm4n can you review the unit test code?

src/tests/elementary/efl_ui_test_scroller.c
155

warning: variable ‘rect’ set but not used.

eagleeye updated this revision to Diff 27357.Thu, Dec 5, 11:09 PM

delete timer and warning message:)

The cause of test suite fail seems to be another widget.(list_collection, popup)

eagleeye updated this revision to Diff 27358.Thu, Dec 5, 11:11 PM

move comments

bu5hm4n requested changes to this revision.Fri, Dec 6, 6:15 AM

The test failure you are seeing is caused by the fact that this introduces scrollbar changes when the theme is loaded, this is initially not done. Is this staying like this ? Is this correct ? If yes, then please change the offending value from 22 to 21.

This revision now requires changes to proceed.Fri, Dec 6, 6:15 AM