Page MenuHomePhabricator

Cleanup elm_slider.eo
Closed, ResolvedPublic

Description

elm_slider.eo should join Efl.Ui namespace. There might be some small cleaning of the API to merge some function.

@cedric What is the purpose of this task ?
Could you give some details on this ?

I could not understand well about "small cleaning of the API to merge some functions".

cedric added a comment.May 1 2017, 2:54 PM

The easy one, it is not yet in Efl.Ui namespace. The harder part. I think that indicator should be an efl_part with format, visible and visible on focus property (Or something along that line). I am still wondering about the range API of slider and if it shouldn't be an interface that could also apply on edje efl_part drag.

I can understand your point.
I will have discussion on this task with my members, and will give some description how we expect the slider will be.

One thing I want to know about efl_ui_progress interface.
I'm wondering why value related properties(value, min/max value) are not in this interface.
I think these can be used for progressbar and slider together.
I feel little bit strange about part_value_set in elm_progressbar class, and there is no guideline which PART is available for setting value.

jpeg added a comment.May 11 2017, 10:07 PM

I can understand your point.
I will have discussion on this task with my members, and will give some description how we expect the slider will be.

One thing I want to know about efl_ui_progress interface.
I'm wondering why value related properties(value, min/max value) are not in this interface.

Yes this needs to be fixed, as you mention.

I think these can be used for progressbar and slider together.
I feel little bit strange about part_value_set in elm_progressbar class, and there is no guideline which PART is available for setting value.

Part APIs need to use Efl.Part instead of a part name in the API itself. This will make the progressbar API much simpler for the common case.

All in all I believe this ticket is quite easy to resolve.

jpeg assigned this task to sanjeev.May 25 2017, 2:40 AM
singh.amitesh added a subscriber: sanjeev.

@woohyun @cedric @raster @jpeg @DaveMDS

progressbar does not hv min_max and progress interface is used in both slider and progressbar.

min_max in slider does make sense and we don't have min_max in progressbar since the default range is from 0 to 1.

we should think about this.

In T5361#86244, @cedric wrote:

The easy one, it is not yet in Efl.Ui namespace. The harder part. I think that indicator should be an efl_part with format, visible and visible on focus property (Or something along that line). I am still wondering about the range API of slider and if it shouldn't be an interface that could also apply on edje efl_part drag.

@cedric we have a function for setting the format function in slider: indicator_format_function().

  1. get rid of this function and move other indicator function in slider internal part class.
  2. keep it in slider and allow developers to override this function?
jpeg added a comment.Jun 12 2017, 8:17 PM

The format function could be done either by override (efl_override or proper eo inheritance) or by using a delegate object, on which we would indeed need to override (but it's probably cleaner to do so than altering the widget itself). Events & promises are not suited to this.

As for the range I believe a min/max would be good, with 0 to 1 being the default values.

@singh.amitesh

As @jpeg said, I think min/max could be easily understood by developers as for range.
For example, min = 10, max = 110 ,
and if app wants to get current value with 50% progress, then return 60 !!
Easy to use and good for developers :)

singh.amitesh added a comment.EditedJun 13 2017, 2:26 AM

We (i and @jpeg) think that Efl.Ui.Progress should be renamed to Efl.Ui.Range
It makes more sense actually. efl_ui_progress_* APIs on slider widget looks Odd.

If we do this, then we need to rename the range APIs in slider to something else. what should be the name?

Reference:
https://refreshless.com/nouislider/

Also there is a span_size property in Efl.Ui.Progress Intf which actually set the min size on the internal part of slider/progressbar. It can be handled as calling min_size hints on efl_part()?

@cedric

singh.amitesh added a comment.EditedJun 13 2017, 9:58 PM

We (i and @jpeg) think that Efl.Ui.Progress should be renamed to Efl.Ui.Range
It makes more sense actually. efl_ui_progress_* APIs on slider widget looks Odd.

If we do this, then we need to rename the range APIs in slider to something else. what should be the name?

@raster suggested it to be named as "interval".

Reference:
https://refreshless.com/nouislider/

jpeg added a comment.Jun 13 2017, 10:34 PM

Efl.Ui.Range with an interval sounds good to me.

@singh.amitesh
I also think span_size property is something ambiguous for developers.
(Honestly, elementary developers are using that property without knowing how it works to progressbar or slider)

I think, if progressbar and slider are working fine with "resizing" or "min value setting", the, developers would be happier.

After discussing with @jpeg and @woohyun, we have decided to get rid of span_size API from interface and keep it as legacy API for slider and progressbar.
We should have a slider theme without any text parts and contains only bar and ef.ui.slider should be compatible with new (minimal theme) and old theme.

In this way, we get rid of span_size and developer can use min_size api to set the min size of slider.

jpeg added a comment.Jun 20 2017, 11:08 PM

@singh.amitesh You later mentionned that the span_size might actually be required, as a "nude" progress bar wouldn't be good enough. Care to comment more?
Honestly if we can have "nude" widgets I believe it would be just as simple. We only need to make it real easy to app developers to combine those widgets with their own labels, and sync them (i.e. in an event callback).

cedric raised the priority of this task from TODO to High.Jul 10 2017, 2:24 PM

new interval slider widget is added. It is a slider with two indicators. Legacy range would work on base slider as usual.

can we close this? i think its completed.

jpeg added a comment.Sep 18 2017, 7:25 PM

No it's not completed.

type slider_func_type: __undefined_type; [[Elementary slider function type]]
type slider_freefunc_type: __undefined_type; [[Elementary slider free function type]]

And it's used in:

@property indicator_format_function {
   set {
      [[Set the format function pointer for the indicator label

        Set the callback function to format the indicator string.
      ]]
   }
   values {
      func: slider_func_type @nullable; [[The indicator format function.]]
      free_func: slider_freefunc_type @nullable; [[The freeing function for the format string.]]
   }
}

Are you sure the indicator APIs are specific to the slider and only the slider? They don't apply to progressbar or anything else???

In T5361#98753, @jpeg wrote:

No it's not completed.

type slider_func_type: __undefined_type; [[Elementary slider function type]]
type slider_freefunc_type: __undefined_type; [[Elementary slider free function type]]

And it's used in:

@property indicator_format_function {
   set {
      [[Set the format function pointer for the indicator label
 
        Set the callback function to format the indicator string.
      ]]
   }
   values {
      func: slider_func_type @nullable; [[The indicator format function.]]
      free_func: slider_freefunc_type @nullable; [[The freeing function for the format string.]]
   }
}

Are you sure the indicator APIs are specific to the slider and only the slider? They don't apply to progressbar or anything else???

progressbar does not have indicator so this API is not required there.

We can close this now.

T6204

jpeg added a comment.Nov 13 2017, 1:24 AM

Questions, based on efl_ui_slider.eo:

methods {
   @property indicator_show { (bool) }
   @property indicator_show_on_focus { (bool) }
   @property step { ... }
   @property indicator_visible_mode { (Efl.Ui.Slider.Indicator_Visible_Mode) }
}
parts {
   indicator: Efl.Ui.Slider.Part; [[A floating indicator above the slider.]]
}

So why do we have a part and then 3 API's that contain the word indicator?
Also, why is it "show" and then "visible_mode"? show is not a noun, is it?

jpeg added a comment.Nov 14 2017, 11:25 PM

Looks like the answer is to have a single enum for EO API, which is slightly different from the legacy:

  • default
  • always
  • none
  • on_focus
  • on_drag (new in eo as it's "default" in legacy)

Bbut default here should mean "whatever is in elm_config".

In T5361#104728, @jpeg wrote:

Looks like the answer is to have a single enum for EO API, which is slightly different from the legacy:

  • default
  • always
  • none
  • on_focus
  • on_drag (new in eo as it's "default" in legacy) Bbut default here should mean "whatever is in elm_config".

Thanks @jpeg

Created a subtask for this. T6376