Page MenuHomePhabricator

efl.ui.slider
Closed, ResolvedPublic

Description

class Efl.Ui.Slider @beta
├ (E) slider,drag,start
├ (E) slider,drag,stop
  • Move steady to range_display_interactive.
  • Separate efl_ui_slider and elm_slider (WooHyun)
  • Separate efl_ui_slider and efl_ui_slider_interval (WooHyun)
  • Refactor efl_ui_slider (including the work of centralizing data flow) (WooHyun)
There are a very large number of changes, so older changes are hidden. Show Older Changes
zmike added a comment.Jun 12 2019, 9:49 AM

The changed events should send the current value, otherwise I guess this is okay?

Also there's the delay,changed event; should this be changed,delay or something instead?

zmike moved this task from Backlog to Evaluating on the efl: api board.Jun 12 2019, 9:49 AM

I had to look at the code to understand what is delay,changed.
This is emitted 0.2s after the slider stops moving. Every time the slider moves, the timer is restarted.
There's no doc about this, but thanks to my background in GStreamer I know this is useful to delay any expensive action (like seeking on a media stream) until the user stops scrubbing the slider up and down.

So, armed with this knowledge, I propose we rename this event to changed,stable.
I don't think delayed is a good name, since I would expect to receive as many delayed events as changed events (only later).

zmike added a comment.Jun 13 2019, 9:13 AM

That's a good point, but stable has sort of a weird feel to it as well.

I think stable conveys the exact meaning, but that might be my signal processing background.
stabilized instead of changed?
changed,steady?

zmike added a comment.Jun 13 2019, 9:35 AM

I think the issue for me is that changed is still in the signal name, and changed implies that a change was completed. Having changed,stable or similar is weird because it implies that regular changed events are not "complete".

So just steady? How about that?

zmike added a comment.Jun 14 2019, 6:48 AM

I think that's also not quite what we want either?

woohyun added a comment.EditedJun 18 2019, 3:12 AM

If we can distinguish the "steady" event from the other previous "changed" events by using "slider,drag,stop" -
I don't know why "steady" needs to be supported.

That is, if events will come with following sequence - then, applications can just add one flag to check "whether the user is still dragging the slider".

  1. slider,drag,start
  2. (dragging) -> changed -> changed -> changed -> .....
  3. slider,drag,stop
  4. changed (= steady)

Or, if 3 and 4 are happened inversely, then just keep the latest value in last call of "changed", and do something when "slider,drag,stop" is called.

Am I missing something :| ?

It's not the same thing. You can receive steady events before receiving the drag,stop event.

Think about dragging the time slider in a media player: you want to show the frame corresponding to the time position in the slider, even when the user is still dragging.
The steady event is a filtered version of the changed event. It is a mechanism to avoid performing unnecessary operations (when the slider is changing quickly), unrelated to drag,start and drag,stop.

Drag,start - Changed - Changed - Changed ... (0.2s timeout) Steady ... Changed - Changed ... (0.2s timeout) Steady ... Drag,stop

escwyp added a subscriber: escwyp.Jun 19 2019, 3:42 PM

What about programs that still use the old name "delay,changed"? For me they stop working as expected since bc98c94dc9b1f41406870eca709a97ec63daf96c

woohyun added a comment.EditedJun 19 2019, 7:18 PM

It's not the same thing. You can receive steady events before receiving the drag,stop event.

Think about dragging the time slider in a media player: you want to show the frame corresponding to the time position in the slider, even when the user is still dragging.
The steady event is a filtered version of the changed event. It is a mechanism to avoid performing unnecessary operations (when the slider is changing quickly), unrelated to drag,start and drag,stop.

Drag,start - Changed - Changed - Changed ... (0.2s timeout) Steady ... Changed - Changed ... (0.2s timeout) Steady ... Drag,stop

I can understand the difference. Thanks for your explanation.
I think giving a way to change the timeout would be good supports for developers.

@escwyp is right, legacy sliders are broken now (take elementary_config's Scale slider, for example). I'm looking into it.

@woohyun I don't like having a hardcoded 0.2s timeout, but I also don't think it is very useful to be able to control that...

@segfaultxavi

https://phab.enlightenment.org/D9130 <- Could you check whether this is something you are looking into ?

@woohyun I was trying to push the fix and I could not because you beat me by 1h :D

zmike added a comment.Jun 20 2019, 7:13 AM

This seems like it should have a unit test?

Diffusion closed subtask T7863: efl.ui.direction as Resolved.
Diffusion closed subtask T7562: efl.input.interface as Resolved.

delay,changed and changed can be just in the range_display, so this here is not needed anymore...

segfaultxavi updated the task description. (Show Details)Jul 12 2019, 6:39 AM

I agree, but I would say that these events belong to Efl.Ui.Range_Interactive, rather than Display.

As discussed, changed goes to Efl.Ui.Range_Display and steady goes to Efl.Ui.Range_Interactive.

bu5hm4n added a comment.EditedJul 16 2019, 5:16 AM

Just as a quick heads up of that widget:

  • Using internal hidden supper weird api, that gets randomly wrong values passed.
  • moving the key events for changing the slider does not change the value
  • events are only emitted after user did interaction, not via api
  • _efl_ui_slider_down_knob and _efl_ui_slider_move_knob are plain copies of each other.
  • internal vs. external API for setting / fetching the value. Which seems to be out of sync in pretty much most cases ?
  • Theme functions that try to parse theire own name just to create it again from what they have parsed ???

Just as a quick heads up of that widget:

@eagleeye and I also want to check the things together ~ :)

  • Using internal hidden supper weird api, that gets randomly wrong values passed.

Can I know the name of that weird API ? I also want to check together.

  • moving the key events for changing the slider does not change the value

In elementary_test (Efl.Ui.Slider), I can change the value by key event (left or right).
Am I missing something on your comment ?

  • events are only emitted after user did interaction, not via api

Hmm. Do you mean that we need to add some APIs which can do the same thing with user interactions ?

  • _efl_ui_slider_down_knob and _efl_ui_slider_move_knob are plain copies of each other.

Right. thery are just the same. We may create an internal function for efl_ui_slider. (not for efl_ui_slider_interval)
Anyway, this thing may relate to refactoring not to interface :)

  • internal vs. external API for setting / fetching the value. Which seems to be out of sync in pretty much most cases ?

I'm a bit confused that what are internal and external APIs for setting / fetching the value.

  • Using internal hidden supper weird api, that gets randomly wrong values passed.

Can I know the name of that weird API ? I also want to check together.

efl_ui_slider_move_knob
efl_ui_slider_down_knob
efl_ui_slider_val_set
efl_ui_slider_val_fetch

Those APIs are declared in elm_priv.h which is definitly not the way we want to go, it makes the whole design of the object quite hard to understand, and inheriting/extending it impossible.

  • moving the key events for changing the slider does not change the value

In elementary_test (Efl.Ui.Slider), I can change the value by key event (left or right).
Am I missing something on your comment ?

This was fixed by 4b1a1e85b2d77bccdc03ca6ece389aea4881d3ec

  • events are only emitted after user did interaction, not via api

Hmm. Do you mean that we need to add some APIs which can do the same thing with user interactions ?

No - i am talking about line 79 - 82 in efl_ui_slider.c the CHANGED event (which is emitted twice) is only emitted if it was a user interaction, changing the value via API does not emit this event. Which is wrong, the user needs to be informed every time the value changes, not just when the user interacts.

  • _efl_ui_slider_down_knob and _efl_ui_slider_move_knob are plain copies of each other.

Right. thery are just the same. We may create an internal function for efl_ui_slider. (not for efl_ui_slider_interval)
Anyway, this thing may relate to refactoring not to interface :)

  • internal vs. external API for setting / fetching the value. Which seems to be out of sync in pretty much most cases ?

I'm a bit confused that what are internal and external APIs for setting / fetching the value.

There is efl_ui_slider_val_fetch and efl_ui_slider_val_set which are totally internal, i do not see any value in having API in the vtable which needs to be called in order to have valid values in efl_ui_range_displays API.

In my opinion this widget needs two very important things:

  • Absolut independend legacy and eo-api implementations, elm_slider should have nothing to do / to deal with efl_ui_slider.c everything else makes refactoring of the new api impossible, since we will definitly break legacy API, which is not possible
  • There needs to be one central "flow of data" if the user interacts with the widget, and a new value is set, the value needs to go through the API of efl_ui_range_display, everything else makes extending this interface impossible, the inheritoor would never get told via API about a change, which is a pain.

@bu5hm4n

Thanks for your explanation.
So, can we summarize the work items like -

  1. separating efl_ui_slider and elm_slider
  2. centralizing data flow based on efl_ui_range_display

??

Plus, about CHANGED event.
How do you think about supporting CHANGED and USER_CHANGED ? (for efl_ui_slider after separating with elm_slider)

I'm curious, why do you want to distinguish between changed and user_changed? What is the use case?

I'm curious, why do you want to distinguish between changed and user_changed? What is the use case?

There were some cases something like -

  1. slider widget is used for volume controller.
  2. Default value is set by API => no sound should be played
  3. A user changes the volume by dragging the slider widget => sound should be played as an example

I had thought that this might be a one of the reasons that legacy slider did not emit CHANGED event.

If USER_CHANGED is supported, then developers may feel easy to implement above feature.

Thanks @woohyun for giving a use case! That could indeed be solved easily by emitting user_changed events.

However, this looks like a general problem. We could have the same issue with any Widget that accepts user input but can also be changed through API (Spinners, for example).
I see some solutions:

  1. We add user_changed events to ALL widgets currently emitting changed events.
  2. We make ALL changed events emit a struct that includes a flag (lots of work!).
  3. We add a flag to the Efl_Event structure itself.
  4. We do nothing and let the user handle this case externally (for example, freezing events before setting slider values through API).

What do you think @bu5hm4n @zmike @cedric ? Is this use case worth exploring a bit more?

What do you think of adding a flag to efl_ui_widget, called user_interaction, it is set on widgets where a user is currently interacting with a widget (pressing, unpressing, typing). The API-user can then check in the changed event if the user is currently interacting with the widget or not. This has the advantage of flexibility, we can also find out if a element is currently moved because the user interacts with a slider (for example). Additionally, implementing something like user_changed events is super hard, as an example, radio_group: selecting a radio button will emit a event to the radio_group, the radio group will unset the previously selected radio button, this call-flow does only involve public API, which does not have a user_changed flag, so no way to differenciate). However, finding out that the UI-user does interact with a widget while this event is emitted makes this whole thing easier ... :)

@bu5hm4n With your proposal and the radio example you give, only the clicked radio button will have the user_interaction flag set, no? the radio button being unset will not have this flag. A user listening to "unset" events won't know it's because of user interaction, or am I missing something?

no you are right ... :( damn.

Which brings us to a more complex topic... What does "user interaction" mean, exactly?
When a radio button is selected, another one is unselected through API. The unselected event is a user interaction?
When a user clicks a button which re-centers a scroller, a lot of events are emitted (position changed, visibility...). All these events are user interactions?

We need to define very carefully what is a user interaction...

bu5hm4n updated the task description. (Show Details)Jul 31 2019, 5:13 AM
zmike added a comment.Jul 31 2019, 6:56 AM

I think the concept of user interaction here only applies to the widget which is being activated. For the case of a radio being unset, an app would be listening for the active radio since there can only be a single radio activated at a given time.

It seems to me like having something centralized in efl_ui_widget would work for this. A widget can toggle this flag for the duration of emitting all the changed events, and then even the events which may be unsetting values (e.g., the radio case) will have this flag active.

hmmm... in the radio case, which widget would perform this centralization? and which ones are emitting the selected and unselected events?

zmike added a comment.Aug 1 2019, 6:30 AM

It's the radio group which has the events, so the radio group can just toggle the flag for the duration of emitting its callbacks (propagated directly from the flags of its child objects, which can similarly be set from clickable).

@segfaultxavi @bu5hm4n @zmike

How about discussing "user,changed' later after the next release ?
When I asked about this, I just thought that it was a simple thing by adding it to efl_ui_slider only.
But, while discussion, I have felt like this one needed to be handled with more general consideration for whole widgets.
So, I think it would be better to discuss this after doing the basic interface works.

If everyone agrees, then I'll create a task for "user,changed".

How would we do that? We make this release without user,changed events and add them afterwards? I'm OK with this but are these events required by any app right now?

How would we do that? We make this release without user,changed events and add them afterwards? I'm OK with this but are these events required by any app right now?

@segfaultxavi

I think there can be some alternatives for the case that there is no "user,changed" (e.g make a flag and set it just before calling API, and check whether the flag is on or off in the callback function).

But !! if there are some cases that widget itself changes the value internally, then this needs to be discussed again.
(If my understanding is right, efl_ui_slider does not call range_value_set function internally)

So, could I add below two work items ?

  1. separating efl_ui_slider and elm_slider
  2. centralizing data flow based on efl_ui_range_display

If these works don't give any change on "interface's definitions", then we can set lower priority.
I cannot estimate well for now.

@bu5hm4n
How do you think about this ?
While reviewing codes, you did think these refactoring will affect to the change on interfaces ?

woohyun updated the task description. (Show Details)Aug 16 2019, 11:17 PM
bu5hm4n updated the task description. (Show Details)Aug 16 2019, 11:23 PM

The problem is, those works give a really big change on how the interface interacts, as right now event emission is pretty messed up, and a lot more events are emitted than we actually need ... I am not sure if we get arround resolving that before declaring it stable.

I will try the work item #2, and try to estimate the things you are worrying about.

I hope the whole work items are doable :)
I will make a patch for #2 soon.

Cool, thank you!

Lets see how far we get :)

woohyun updated the task description. (Show Details)Aug 20 2019, 5:57 PM

Ok. I'm moving to Action Item #3.
If anyone has another idea on the slider class (and it needs big change), please let me know asap :)

woohyun added a comment.EditedAug 21 2019, 3:52 AM

@bu5hm4n @segfaultxavi @zmike

As @bu5hm4n talked before - there are lots of bad things in both efl_ui_slider and efl_ui_slider_interval.

Such as,
efl_ui_slider_move_knob
efl_ui_slider_down_knob
efl_ui_slider_val_set
efl_ui_slider_val_fetch

Now, I can know why they have had these internal APIs.
That's because "efl_ui_slider_interval is perfectly different widget from efl_ui_slider !!!!".

For example, if you try value_set to efl_ui_slider_interval -
which thumb (of two) should be moved ?

In efl_ui_slider_interval, only range_set API is available.

We may make a mixin class for these, but - first - I want to get agreement to SEPARATE these two classes.

How do you think about this ?

(If everyone says YES, then I will make a patch to separate them. Plus, I want to keep @beta for efl_ui_slider_interval because it needs to change the class definition + has lots of bugs)

Separating them looks like a very good idea, we can still later in refactor common out out of them, and stuff this code into a common place like a shared header file, no need to make it in a class.
The Interval thing also might not fit into the range_interactive API, so this sounds very good to me.
@zmike your opinion ?

zmike added a comment.Aug 21 2019, 7:46 AM

Separating seems like a good idea for now so that we can focus completely on slider.

woohyun updated the task description. (Show Details)Aug 21 2019, 5:37 PM
woohyun updated the task description. (Show Details)Aug 23 2019, 2:53 AM

I can continue the work of refactoring later - but, I don't think the work is relating to "class definition work".

I also want to get more opinions in the aspect of interface :)

If there is no more, we may consider to move this task to stabilized step.

woohyun updated the task description. (Show Details)Aug 27 2019, 11:29 PM
bu5hm4n updated the task description. (Show Details)Aug 28 2019, 6:52 AM

Thank you @woohyun, i would say that this now looks ready to be stabelized? @zmike @segfaultxavi

I love what we did to this class... it is almost empty! I think we cannot stabilize much more :D

bu5hm4n moved this task from Evaluating to Stabilized on the efl: api board.Sep 6 2019, 2:35 AM
zmike closed subtask T7578: efl.ui.view as Resolved.Thu, Sep 26, 8:12 AM