Page MenuHomePhabricator

efl.ui.format
Closed, ResolvedPublic

Description

|mixin Efl.Ui.Format @beta
|├ (P) format_func
|├ (P) format_string
|├ (P) format_values
|├ (M) formatted_value_get @protected
|├ (M) decimal_places_get @protected
|├ (M) apply_formatted_value @pure_virtual @protected
  • Add the above properties and methods and adapt all existing classes (e776f5f0d7b6).

Related Objects

StatusAssignedTask
ResolvedNone
ResolvedNone
ResolvedCHAN
bu5hm4n created this task.May 3 2019, 12:00 PM
bu5hm4n triaged this task as TODO priority.

This class needs serious doc love. I do not understand how format_cb and format_string are supposed to interact. Also, Format_Func_Cb only has input parameters, no output, where is the formatted string supposed to be written?

zmike added a comment.Jun 14 2019, 9:18 AM

We try to leave some functionality to the imagination of the user; no docs can possibly hope to capture the gestalt of efl's splendor.

woohyun added a comment.EditedJun 17 2019, 8:58 PM

I agree with @segfaultxavi that I perfectly cannot understand how format_cb and format_string work together So, confusing :(

In efl_ui_calendar, the results are different between below [1] and [2].
That's because format_string_set is doing format_cb_set internally (in efl_ui_calendar).

[1]

efl_ui_format_string_set(efl_added, "%b");
efl_ui_format_cb_set(efl_added, NULL, _cal_format_cb, NULL);

[2]

efl_ui_format_cb_set(efl_added, NULL, _cal_format_cb, NULL);
efl_ui_format_string_set(efl_added, "%b");

So, my opinion is .... it's not something easy to understand !!! :(
I'm still considering what would be the better way to support this.

@CHAN Could you share your idea on this ?

woohyun added a subscriber: CHAN.Jun 17 2019, 10:43 PM

Agreed. This is confusing. This is how it works (after examining the code):

The user can only set ONE property, either format_cb or format_string.

  • format_string is the easiest to use because it works just like printf.
  • format_cb is more flexible because the user provides a callback and can produce ANY string it wants based on the Eina_Value it receives.

So that's why @woohyun's examples render differently, the last call wins.

I think the above behavior is fine, we just need to document it properly.

SUGGESTION: Efl.Ui.Spin adds another property, called special_values which is an array of strings to use instead of certain values. I think it is useful and it is a different approach to the above two options. So I suggest we add format_special_values to Efl.Ui.Format and move that functionality here (and out of the spin).

What do you people think?

zmike added a comment.Jun 18 2019, 9:04 AM

I think something more like format_values would be a better name, in keeping with format_ namespacing?

I'm not opposed to this idea; the concept is that you would have a C-array of strings which correspond to numeric values?

Yes, that is the concept.

I suggested format_special_values, but I am happy too with format_values or format_value_array.

zmike added a comment.Jun 18 2019, 9:28 AM

I'm not sure we want to use the word special here since it's just strings? I really would prefer something like format_string_array but then that's misleading because format string is an actual programming term...

How about this: Efl.Ui.Format has one property called "formatter" which takes a object (and its ownership).
Then there is a Efl.Ui.Formatter interface, which has one function "apply" (?) that takes a Eina_Value and returns a string.

Then there can be several implementations that do that transformation, based on the most prefered way:

  • A string that formats simple data types like int etc.
  • A Array like the simple thing (?)

You can pretty much implement here any helper you want, and if someone wants to have something freaked out, he can just create his own interface implementation...

My first reaction is that this sounds awful to use and is massively overengineered, but I'll sleep on it.

@bu5hm4n @zmike

I'm not sure about the best way for this.
But, if we can make one event_hook function which would be called just before changing value (something like filter callback in elm_entry or event intercept function),
then developers can replace specific value to their own string easily.

How do you think about this idea ?

We already have the format_cb mechanism in place which allows the user to do anything he wants.

The rest of the properties we are discussing (format_string and format_values) are methods to simplify usage.
Defining callbacks, listening to events, or creating and instantiating new objects does not look simpler to me.

Whereas:

efl_ui_format_string_set(obj, "%2.1f Km");

and

Efl_Ui_Spin_Special_Value values[12] = {
  {1, "January"}, {2, "February"}, {3, "March"}, {4, "April"},
  {5, "May"}, {6, "June"}, {7, "July"}, {8, "August"},
  {9, "September"}, {10, "October"}, {11, "November"}, {12, "December"}
};
efl_ui_format_values_set(obj, values);

looks very convenient to to me.

If you agree to move format_values into Efl.Ui.Format then we can discuss its API, because right now it needs to convert from C arrays to Eina arrays and, again, that does not look simple (I'm talking about this).

CHAN added a comment.Jun 20 2019, 4:18 AM

I didn't know that there was a confusion in usability.
When we designing format interface between two or three people who knew the usability of both properties.

@segfaultxavi
The intended usability is correct as you mentioned above.
It might be a good idea to augment the documentation to reduce usability hassles.

+
The special value is another value that matches the value.
I tried to add in the Range interface this feature first because, there is a range of values and a step is required to be used.
However, since the usability is not universal in other classes that implement the range interface, it is limited to a spin specific function.

@CHAN The only usability issue is that it is not clear what happens when you use both format_cb and format_string. Better docs will fix that.

Regarding moving special_values from Efl.Ui.Spin to Efl.Ui.Format: This property turns a value into a string, just like format_cb and format_string, so I think it belongs to Efl.Ui.Format (Not Efl.Ui.Range_*). It will provide useful functionality to Progressbar, Tags, Calendar... for free. Don't you agree?

CHAN added a comment.Jun 21 2019, 4:26 AM

@segfaultxavi

Yes, i do. I think there is nothing wrong with it.

Needs to more discuss about naming tho... formatted_string_set?!

anyway please ping me if you need to help or discuss about it.

Thanks.

@CHAN Why would you call it formatted_string_set? It's a property, so the _set and _get will be generated automatically for C.

I think format_string is a better name, because "formatted string" would be the resulting string. For example, "%f Km" is a "format string" whereas "7 Km" is the "formatted string".

We still need a better name for the "special values" property. Current suggestions: format_values, format_values_array

The one side is the API, the other side is how we are *using* this API from the widget.

  • Calendar does use efl_ui_format_cb internally, which means, setting the format_string to the object *does* changes the format_cb, which looks super weird to me. Additionally, the format_cb does not handle all the format symbols that others do.
  • efl_ui_spin only works with format_cb, setting the string does not work.
  • efl_ui_tags only works with format_cb, setting the string does not work.

Additionally, the implementation of the logic that *applies* the formatting, is different in every single widget. So what I think should happen here is some unification of the logic that *applies* the formatting. If this is then happening as a event / format_cb / or-whatever is just a detail. As long as we have the implementation in one single place.

Additional work that needs to happen for this:

  • testcases in efl_ui_spec suite, so we can ensure the result is everywhere the same, for the whole set of features.
  • Define a priority / semantic for what is happening if mutliple properties are used.
  • Define a set of formatting symbols that we support in the string.
zmike added a comment.Jun 24 2019, 6:00 AM

I think if we are going with a priority system then the most bespoke one should be the highest priority. This would mean cb > values > string.

This is how I would like this mixin to work:

Users of widgets that include this mixin should be able to set format_cb, format_string or format_values, whichever one they prefer (We can then decide what do we do when more than one is provided).
Widgets including this mixin should call an internal method passing a value and retrieving a text string. This internal method (invisible to users) will generate the string using whatever mechanism the user provided (callback, value array or format string).

mixin @beta Efl.Ui.Format {
  methods {
    @property format_cb;
    @property format_values;
    @property format_string;
    @protected do_format (param Eina.Value; returns string;)
  }
}

How does that sound (including the list of remaining things from @bu5hm4n) ?

I am fine with this.

segfaultxavi moved this task from Backlog to Evaluating on the efl: api board.Jun 26 2019, 12:25 AM

I'll work on this then.

CHAN claimed this task.Jul 7 2019, 10:06 PM

We usually do not take ownership of the stabilization tasks. We all contribute by commenting.

Besides, this one is already done (the previous comment says that I was working on it), the changes are landed in the emergency github mirror and should be merged to our own repo shortly.

The changes are meanwhile mirrored to to official efl.git repo. Details on the workflow can be found on https://phab.enlightenment.org/w/efl_api_stabilization_workflow/.

segfaultxavi updated the task description. (Show Details)Jul 8 2019, 12:42 AM
segfaultxavi updated the task description. (Show Details)Jul 8 2019, 12:52 AM

I think this looks mostly fine ?

bu5hm4n moved this task from Evaluating to Stabilized on the efl: api board.Jul 12 2019, 6:26 AM
CHAN added a comment.Jul 15 2019, 10:06 PM

@segfaultxavi Sorry i'm late.

I just see whole things u have worked for this.
Its really good job!

Just one thing to ask u about this work.

The 'Efl.Ui.Format_String_Type' Do we need this type?
When user set a format string, we can classify the time format separately, and in case of ambiguity like %x, %d, we can see the usability as the type of widget.

With our current implementation, The codes is easy to read and clean It's good, but I'm asking if it's a new type that users do not need to know about actually.

I think having a way to specify the type of the format string is more future-proof, because we can add more types later.
The default value is simple (same as printf) so C# bindings do not need to provide this type most of the time. Only when using time strings the user will need to provide the type (currently only the Calendar widget uses it).
Moreover, now that all formatting is done by the mixin, having the type of the string depend on the widget will be confusing, because now all docs are on the mixin, not on the widget. How would we split the docs?

If you still want to remove this type, we can think of other ways to specify the string type: For example, a prefix on the string ("<time>Today is %d"), or we completely separate the format specifiers (so that %d means integer and %#d means day of the month).

CHAN added a comment.Jul 31 2019, 10:53 PM

Okay then i agree with ur opinion.