Page MenuHomePhabricator

Efl.Ui.Progressbar
Closed, ResolvedPublic

Description

class @beta Efl.Ui.Progressbar extends Efl.Ui.Layout_Base implements Efl.Ui.Range_Display, Efl.Ui.Format,
                          Efl.Ui.Layout_Orientable, Efl.Access.Value,                                    
                          Efl.Text, Efl.Content, Efl.Text_Markup                                         
{                                                                                                        
   @property pulse_mode (boolean);
   @property pulse (boolean);
   @property show_progress_label (boolean);
}
  • Separate elm_progressbar from efl_ui_progressbar (@bu5hm4n : thx !!)
segfaultxavi triaged this task as Normal priority.
segfaultxavi moved this task from Backlog to Evaluating on the efl: api board.

This has proper documentation and a simple enough API.

My only concern is that the "pulse" API looks a bit convoluted:

  • There's the pulse_mode flag which controls whether the bar is in "normal" mode (bar fills up as progress advances) or in "infinite" mode (where the bar never fills up, because the current progress is unknown).
  • There's the pulse flag which is used only in "infinite mode" (pulse_mode == true). When pulse == true the bar has a pulsating animation. When false the animation is stopped, but it is still in "infinite" mode.

I suggest:

  • Rename pulse_mode to progress_mode: Whether the bar "pulses" or does another animation depends on the theme.
  • Turn pulse_mode into an Enum instead of a boolean: This will allow having more than one animation in the future, and the meaning of the property will be more clear. The enum values will be Normal and Infinite.
  • Rename pulse into animated_progress: Again, the particular animation depends on the theme, it might not "pulse" at all.

@segfaultxavi
Thanks for checking first :)

Legacy elm_progressbar has been a confusing widget to developers because the properties (pulse, pulse_mode) were ambiguous.
So I also hope to refine the class definition of efl_ui_progressbar.

I think we can think with another aspect.
In Android, progressbar is used with the ways -

  1. Indeterminate mode
    • Use indeterminate mode for the progress bar when you do not know how long an operation will take. Indeterminate mode is the default for progress bar and shows a cyclic animation without a specific amount of progress indicated.
    • Setting value is meaningless with this mode
    • The animation is enabled as default, and by unset this mode (i.e. change to determinate mode), the animation is stopped.
  2. Determinate mode
    • Use determinate mode for the progress bar when you want to show that a specific quantity of progress has occurred. For example, the percent remaining of a file being retrieved, the amount records in a batch written to database, or the percent remaining of an audio file that is playing.
    • Setting value (including min/max value) is working with this mode.
    • No animation is supported because value would be set when application wants.

Based on this research, I think current efl_ui_progressbar's pulse with range value (ex: 0 ~ 100) seems meaningless.
(Indeed, Tizen developers have not been used this at all)

What application wants is either -

  1. Infinite animation (without any value) for showing "progressing state"

or

  1. No animation as default. Application will make their own progress movement by value setting

So, my conclusion is - (based on @segfaultxavi 's support)

  1. First, separate elm_progressbar from efl_ui_progress
  2. Only support "progress_mode" with Enum (as @segfaultxavi suggested)
  3. remove "pulse"
  4. also remove "show_progress_label" (including apply_formatted_value, text, markup, and other methods for text display)

@segfaultxavi @bu5hm4n @zmike @Jaehyun_Cho
How do you think about this ?

bu5hm4n added a comment.EditedDec 8 2019, 1:45 AM

Sorry for my late reply, i was busy.

So, my conclusion is - (based on @segfaultxavi 's support)

  1. First, separate elm_progressbar from efl_ui_progress

Sounds like a good idea.

  1. Only support "progress_mode" with Enum (as @segfaultxavi suggested)

I am not sure why @wooyhun @segfaultxavi do want this. The infinite animation (like they are right now in the theme) cannot handle any other "animation" than how it is right now. Was there ever the demand for something like this ? It feels to me that this would be a little bit over engineered, a simple boolean flag is enough IMO (and also easier to work with API wise)

  1. remove "pulse"

Yes, this is somehow implied when you set progress mode to none determinate. "freezing" etc. should be handled internally by the widget.

  1. also remove "show_progress_label" (including apply_formatted_value, text, markup, and other methods for text display)

Why should we remove this ? Every single usage of elm_progressbar in community apps uses the text format features, (sometimes setting it to NULL, which is equal to show_progress_label == false), so there is IMO a very high demand for keeping this.

woohyun added a comment.EditedDec 8 2019, 5:54 PM

@bu5hm4n
Thanks for your feedback.

First, separate elm_progressbar from efl_ui_progress

I'll begin this work soon :)

I am not sure why @wooyhun @segfaultxavi do want this. The infinite animation (like they are right now in the theme) cannot handle any other "animation" than how it is right now. Was there ever the demand for something like this ? It feels to me that this would be a little bit over engineered, a simple boolean flag is enough IMO (and also easier to work with API wise)

Honestly, I also cannot expect what new option can be added for this.
So, I have tried to research other platforms' cases - and I still cannot get any other modes, yet :)
I think you maybe right that enum would lead over engineering for this property.
@segfaultxavi How do you think about just keeping this with bool ?

Why should we remove this ? Every single usage of elm_progressbar in community apps uses the text format features, (sometimes setting it to NULL, which is equal to show_progress_label == false), so there is IMO a very high demand for keeping this.

Many platforms (including Tizen) are using this progressbar with various of appearances.
For example,
https://www.tutlane.com/images/android/android_progressbar_example_diagram.gif

And most of them cannot define their default "progress_label" (and its proper location) -
except the "determinate mode" with specific value range.
(With this reason, Tizen has never included text area of elm_progressbar for a long time)

Plus, I don't think developers will feel much inconvenient from "adding their own progressbar label with proper text format and location".

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

Many platforms (including Tizen) are using this progressbar with various of appearances.
For example,
https://www.tutlane.com/images/android/android_progressbar_example_diagram.gif

And most of them cannot define their default "progress_label" (and its proper location) -
except the "determinate mode" with specific value range.
(With this reason, Tizen has never included text area of elm_progressbar for a long time)

I am not sure i understand your reasoning. But the progress_label is just a boolean property to show the progress label or not. The position is defined in the theme, if the theme does not offer this field, then there is obviously nothing that can be displayed. In case that the progressbar is in pulse mode, the progress_label will not be displayed either.

Plus, I don't think developers will feel much inconvenient from "adding their own progressbar label with proper text format and location".

I 100% disagree with that, Its impossible to add something like the current progress label to the progressbar without support from the progressbar. And as I said before, *every* single community application out there with a progressbar uses the progress label (e, evisum, edi, terminology), so its not really a possibility to remove that feature...

@bu5hm4n

I am not sure i understand your reasoning. But the progress_label is just a boolean property to show the progress label or not. The position is defined in the theme, if the theme does not offer this field, then there is obviously nothing that can be displayed. In case that the progressbar is in pulse mode, the progress_label will not be displayed either.

I think you are understanding well my reasoning :)
And what I don't prefer is the property with "This property will work with some styles and will not work with the others."

This is because we always need to update the documentation for every style update in theme.
(or .. do we need to support a method something like "check_the_availability_of_progress_label".. hmm. I don't know)

If there can be a way that we can support clear explanation (in doc) about this property, I also agree with supporting this property.

Well, its the developers responsibility to know that the theme he is using is supporting the text label or not.
Right now he can check that with checking the return type of efl_part(obj, "efl.text") If the type of the part is != ERROR, then you know this part exists.
Maybe @segfaultxavi has some ideas on it ?

I am OK with using a boolean indefinite_progress instead of an enum progress_mode if you all think the enum is overkill. I thought that it would be more future-proof to use an enum, but you are right the type of animation should be controlled through the theme.

I think the show_progress_label property is useful. It contains a lot of formatting options (through the Efl.Ui.Format interface). Without it, apps will need to add their own label, and format and update it themselves. I agree this label is commonly more used in Desktop environments than on wearables. I would keep it and add a note on the docs saying that "the availability of the label depends on the theme."

What does the "Separate elm_progressbar from efl_ui_progressbar" task mean?

What does the "Separate elm_progressbar from efl_ui_progressbar" task mean?

The idea is that the legacy widget does not depend on the efl.ui version anymore, that is esp. Usefull for Bugfixes that might change legacy behaviour.

Ah OK, "separate the implementation", got it :)

@bu5hm4n @segfaultxavi

Ok ~ then, I'll do below works soon.

  1. separate the implementation
  2. define efl_ui_progressbar only with "indefinite_progress" and "show_progress_label"

All good ? :)

I just started to work on the refactoring, and it seems the legacy stuff keeps on working pretty well. Considering how much work it takes to seperate efl.ui.progress and elm_progress vs. how much it helps,i would rather defer that task, the code does not look that twisted.

So i think we could stabilize this with elm_progressbar and efl.ui.progressbar beeing in one file ?

@bu5hm4n

Thanks for handling this work :)

When I thought about the plan for separating, I wanted to remove "efl_ui_progressbar_part.eo" file and the feature in it.
I could not find the goal of the part APIs in the file.
There are "range_display_range_value_set" and "range_display_range_limits_set" which are doing the same thing with the part APIs.

How do you think about removing part APIs for efl_ui_progressbar ?

About the idea of keeing the both efl_ui_progressbar and elm_progressbar in an one file -
I don't have any objection if above change will not give big confusion for maintaining codes.

woohyun updated the task description. (Show Details)Dec 29 2019, 4:51 PM
bu5hm4n added a comment.EditedDec 31 2019, 12:21 AM

I think the part here are a important part for this class, as these are used to set values to the dual sliders :)

The range parts then are representing the two "knots" . The default impl. on the widget will reflect the first one.

@bu5hm4n
Hmm. I think that kind of feature can be supported by "dual progressbar" later which extends basic progressbar.
Honestly, I have not seen any UX which defined the "dual sliders".
Plus, that would not work in some progressbar styles, such as "wheel" in elm_progressbar.

But, this is just my opinion ~ and I want to hear the opinions of @segfaultxavi and @zmike.
Do you have another idea on this ?

I honestly still do not understand how these parts work inside widgets.
I see no mention to parts in efl_ui_progressbar.eo or efl_ui_slider.eo.

Sooo... how are parts used? How do I build a double slider (or double progressbar) in Unified?

@woohyun youtube is using it, one is doing the loading progress, one is doing the video progress ... But yeah, i see your point about that this could be done by another class. But that also means we have to redo this theme, do you have someone who could handle that ? I am in general utterly bad at everything that involves a theme :D.

@segfaultxavi Lets meet next week on IRC and find a place where we could document how parts do work... I agree, we have a not so good doc on that.

woohyun added a comment.EditedJan 6 2020, 5:29 PM

@bu5hm4n
Don't worry about the theme :)
In elm_progressbar, only "double" style supported that dual-progressbar feature.

elm_object_style_set(pb, "double");
...
elm_progressbar_part_value_set(pb, "elm.cur.progressbar1", progress_value)

That is, the theme for "efl_ui_progressbar" does not have any part for supporting the dual-progressbar. (because there is no "double" style)

@segfaultxavi
I really appreciate to comment about "parts doc" things. I also think "parts" are so difficult to be used with current documentation.

@bu5hm4n
I'm wondering the schedule for this work.
As I talked above we don't need to do anything to efl_ui_prgressbar's theme to remove part things.

Oh sorry - i somehow missed your reply.

True - i would then just guard the part get implementation that we can only use multiple parts when on the legacy widget, does that sounds fine ?

Oh sorry - i somehow missed your reply.

True - i would then just guard the part get implementation that we can only use multiple parts when on the legacy widget, does that sounds fine ?

@bu5hm4n
Honestly, I don't know whether efl_ui_progressbar_part(and it's functionality) would be used later.
But if you think keeping it would be better for the future, I am ok with that :)

Yeah i think keeping it is better. The API does not look wrong, i would implement it exactly the way it is now when ding the double progressbar. Additionally, removing it, means we have to refactor quite a lot of code, which will be simply duplicated by the time we introduce the double progressbar :)

Btw. the revisions attached above do cleanup the implementation of the progressbar, i cannot find anything right now in the code that does look weird to me, when they are reviewed, i am happy to take away the @beta tag :)

@segfaultxavi @zmike anything else you two are seeing on this ?

segfaultxavi added a comment.EditedJan 16 2020, 3:00 AM

API-wise, there's not much more to discuss. Looks right.

Ok :) Let's move this to "stabilized" if there would be no more opinion ~

woohyun moved this task from Evaluating to Stabilized on the efl: api board.Jan 21 2020, 8:10 PM

Different Platforms using this kind of progress bar and with different styles Like Horizontal progress bar, Vertical Progressbar in determinate and indeterminate mode.
Image Example: https://itinsidenews.com/uploads/gallery/202104/img_temp_606b7e8650b6e0-38134862-49373283.gif

and in most platforms, we don't see the default progress label and the reason is that in the progress bar interminate style there is no any fix position of progress bar progress.
Due to this reason, Tizen has never introduced the label text of elm_progressbar for a long time