Page MenuHomePhabricator

Introduce indicator API
Needs RevisionPublic

Authored by bu5hm4n on Dec 8 2019, 2:12 PM.

Details

Summary

this commit brings a new API called indicator-api.

The idea is to have a API where one entity (data_provider) can offer a
range and a value. The other entity (indicator.container) can then take
up these values and ranges and make them something to display on the
screen.

For now this is only implemented in spotlight. The range goes from 0 to
*num of elements*. the value is the currently active element. Floating
point numbers are used to indicate a animation between element A and B.
The same logic can be implemented for scrollers, where range goes from
0 to how often the viewport fits into the size of the content.

The overall idea of this implementation is to provide a easy to use
interface that can be used to pass double values from one widget to
another.

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work_indicator
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14863
bu5hm4n created this revision.Dec 8 2019, 2:12 PM
bu5hm4n requested review of this revision.Dec 8 2019, 2:12 PM
segfaultxavi requested changes to this revision.Dec 16 2019, 8:01 AM

Testsuite fails:

../src/tests/elementary/suite_helpers.c:60:S:efl_ui_indicator_data_provider-efl_ui_pager:event_callback_check_limits:0: (after this point) Received signal 11 (Segmentation fault)
src/lib/elementary/efl_ui_indicator_container.eo
14

If the only information this method provides are the limits, I would call it update_limits.
Just like update_displayed_value below only updates the displayed value :)

src/lib/elementary/efl_ui_indicator_data_provider.eo
1

This is extremely similar to the Efl.Ui.Range_Display interface. Why is a new interface needed?

src/lib/elementary/efl_ui_indicator_icon.c
13

Why Spotlight Container? I thought this new indicator could be used anywhere!

It looks like this var is only used to call efl_content_count() so maybe it should be an Efl_Container *?

src/lib/elementary/efl_ui_indicator_icon.eo
4

Macro busted:

6

Shouldn't this have some more positioning controls to specify WHERE exactly the indicator is placed over that other object?
Something like alignment hints.

src/tests/elementary/spec/efl_test_indicator_data_provider.c
97

Macro busted:

This revision now requires changes to proceed.Dec 16 2019, 8:01 AM

Yeah - lets ignore the error for now - its more about the concept.

src/lib/elementary/efl_ui_indicator_data_provider.eo
1

We cannot just implement half of a interface range_display has both, a getter and a setter for range_value as well as range limits. However, as a dataprovider you dont want to accept a new value, The events and properties are basically the same, the meaning however is absolutly different.

src/lib/elementary/efl_ui_indicator_icon.c
13

Because the spotlight container is the only widget where i have implemented this so far. (Since the infrastructure has been there)
Its the type of the widget that is used, maybe its used in future for something else, i would rather keep it like this.

src/lib/elementary/efl_ui_indicator_icon.eo
6

I would have left that to the theme to decide where to put it exactly, since this heavily depends on the appearance of the container. its added on (We might want to think about some container depending theme name).

src/tests/elementary/spec/efl_test_indicator_data_provider.c
97

Fuck this. Why are you still using ancient libcheck versions ?!

Thank you for the patch :)

I have a question related to Efl.Ui.Spotlight.Indicator.
Do you have any plan to replace or modify Efl.Ui.Spotlight.Indicator and Efl.Ui.Spotlight.Indicator_Icon?
e.g. replaces them to Efl.Ui.Indicator and Efl.Ui.Indicator_Icon
e.g. modifies their class hierarchies like Efl.Ui.Spotlight.Indicator extends Efl.Ui.Indicator and modifies their properties

src/lib/elementary/efl_ui_indicator_data_provider.eo
9

What do you think about changing this property to separated 2 methods as follows?
indicator_limits_get { }
indicator_limits_set @protected { }

Then, we can forbid that indicator_limits_set is called outside of this widget.

16

same comment with indicator_limits

src/lib/elementary/efl_ui_indicator_icon.eo
3

What do you think about that a container widget (e.g. Spotlight.Container) takes care of the location of the Indicator.Container?

Then, container widget (e.g. Spotlight.Container) can set the position of Indicator.Container where it wants and this property may not be needed.

src/lib/elementary/efl_ui_spotlight_container.eo
149

Related to the comment on Data_Provider.indicator_limits,

If @property indicator_limits is separated to indicator_limits_set (@protected) and indicator_limits_get, then how about implements both of them and emitting indicator events in the indicator_limits_set? (i.e. calling indicator_limits_set instead of emitting event)

If indicator events are emitted in the indicator_limits_set, I think it would be helpful for developers who want to refer this implementation.

150

same comment with .indicator_limits

src/tests/elementary/spec/efl_test_indicator_data_provider.c
97

It seems that Ubuntu supports version 0.10.0 by apt install.
To support ck_assert_double_eq on Ubuntu, then need to build libcheck source code..

bu5hm4n added inline comments.Dec 17 2019, 12:50 AM
src/lib/elementary/efl_ui_indicator_data_provider.eo
9

Damn, this is an oversight, this should be get only. No setter at all, the implementation can do that internally.

16

Yeah sorry, this is an oversight, get only is what it should be.

src/lib/elementary/efl_ui_indicator_icon.eo
3

I was thinking about that, but that means that *every* widget needs to have a place for the icon. I think its easier to have a general "holder" thing that the icon can position itself on a widget in some generic way. Additionally - we can think about some magic for placing a icon depending on the widget. However, this "widget" depending positioning should be a special case and not the default. Otherwise we are having the overhead of providing a icon slot on every single widget we are having.

src/lib/elementary/efl_ui_spotlight_container.eo
149

Same as above, this is an oversight, there should only be a getter.

150

Same as above, this is an oversight, there should only be a getter.

src/tests/elementary/spec/efl_test_indicator_data_provider.c
97

@Jaeyhun_Cho yeah i know. But it annoys me that i have to use some macros instead of just "ck_assert_double_eq". The first libcheck release that introduced this macro is 3 (?) years old, i guess ubuntu is not really up to date with this one :/