Page MenuHomePhabricator

format function cb overhaul
Closed, ResolvedPublic

Description

format functions are present in following widgets

  • slider

    For indicator
char *format_func(double val);
void format_free_func(char *data);
  • multibutton entry
char *mbe_format_cb(int count, void *data);
  • calender
char *calender_format_cb(struct tm *stime);

Proposal:

function format_func_cb {
  params {
    @in buf: Eina_Strbuf;
    @in val: Eina_Value;
  }
}
interface/mixin format_func
{
  void format_func_cb_set(format_func_cb format_func);
}
  • slider

usage:

void _slider_format_func(void *data, Eina_Strbuf *buf, Eina_Value v)
{
  double val;
  eina_value_get(&v, &val);

  eina_strbuf_append_printf(buf, " %d units", val); 
}

efl_ui_format_func_cb_set(sliderObj, _slider_format_func);

  • mbe
void _mbe_format_func(void *data, Eina_Strbuf *buf, Eina_Value v)
{
  int count;
  eina_value_get(&v, &count);

  eina_strbuf_append_printf(buf, " %d rabbits", count); 
}

efl_ui_format_func_cb_set(mbeObj, _mbe_format_func);
  • calender
void _calender_format_func(void *data, Eina_Strbuf *buf, Eina_Value v)
{
  struct tm stm;
  eina_value_get(&v, &stm);

  eina_strbuf_append_strftime(buf, " %b %y", stm); // we need strftime support in Eina_Strbuf 
}
efl_ui_format_func_cb_set(calenderObj, _calender_format_func);
singh.amitesh updated the task description. (Show Details)
singh.amitesh updated the task description. (Show Details)
singh.amitesh updated the task description. (Show Details)
jpeg added a comment.Oct 12 2017, 9:43 PM
void _mbe_format_func(void *data, Eina_Strbuf *buf, Eina_Value v)
{
  int count;
  eina_value_get(&v, &count);

  eina_strbuf_append_printf(buf, " %d rabbits", count); 
}
efl_ui_format_func_cb_set(mbeObj, _mbe_format_func);

You need to check the type of a value before reading it with _get.
Note: We probably need some more helpers like to_string that convert to int and double.

q66 added a comment.Oct 13 2017, 5:13 AM

just FYI, keep in mind that if you "pass" the defined function as an argument in Eolian, it will actually generate *three* arguments in C; data, the actual funcptr, and a "free" funcptr (Eina_Free_Cb) - this is necessary to track lifetimes.

jpeg added a comment.Oct 15 2017, 10:59 PM
In T6204#101594, @q66 wrote:

just FYI, keep in mind that if you "pass" the defined function as an argument in Eolian, it will actually generate *three* arguments in C; data, the actual funcptr, and a "free" funcptr (Eina_Free_Cb) - this is necessary to track lifetimes.

Sure, that's expected and perfectly fine.

just did a review on the ML about this patch, my suggestions:

  • naming: use format_cb (or format_func) and format_template for properties names;
  • pass format_template to format_cb function.
  • for format_cb implementation, always check eina_value_type_get() for the expected type! Don't assume it's what you want and blindly eina_value_get()... in case of mistakes/errors, it will likely lead to stack corruption which is a PAIN to find and solve... so it's a simple if to help debugging.

ah, last but not least, the implementation of the default format_func should be something like:

const Eina_Value_Type *type = eina_value_type_get(&value);
if (type == EINA_VALUE_TYPE_INT) {
   int v;
   eina_value_get(&value, &v);
   eina_strbuf_append_printf(buf, template, v);
} else if (type == EINA_VALUE_TYPE_FLOAT) {
   float v;
   eina_value_get(&value, &v);
   eina_strbuf_append_printf(buf, template, v);
} ... // for *ALL* basic types!
} else { // fallback: convert type to string
   char *v = eina_value_to_string(&value);
   eina_strbuf_append_printf(buf, "%s", v);
   free(v);
}
jpeg added a comment.Oct 18 2017, 1:41 AM

@barbieri I agree 100% with all your comments, and this is exactly what I had in mind for the format function / format template. :)
Now I'm just waiting for @singh.amitesh to come back from vacation and complete this :D

hi... the last commit rEFL8661fe234c7e doesn't seem to address my comments... still not passing the format_string to the callback and not handling other types of eina value, just assumes it's a double. Add the runtime type check to avoid bugs to pass unnoticed.

jpeg added a comment.Nov 6 2017, 7:33 PM

Please also check the ongoing work with Spin widget.

jpeg added a comment.EditedNov 14 2017, 11:55 PM

I think the format mixin should check what the format string is, like spinner does, in order to find if this matches the type of value passed. Otherwise it's not very clear to the user which format string should be passed? Documentation is never good enough :)
Also I think it would be best to be safe on the format string. Allow a single % of the right type, basically.

I agree with jpeg. The mixin is a nice place to scan the format string and validate it against the given value type. While printf format are not that simple nowadays, it's something doable in short time.

Handling "%s" for all types, automatically handling eina_value_to_string() would be nice.

but do not forget about modifiers, such as %.20s, %2.1f and so on...

jpeg added a comment.Dec 11 2017, 6:46 PM

I closed this but I'm not insanely happy as Efl.Ui.Spin tries to figure out the type and fails if we choose "%s" (which works fine with the mixin, just using to_string). Also long long or unsigned types are not supported.

singh.amitesh added a commit: Restricted Diffusion Commit.Aug 20 2018, 1:00 PM
singh.amitesh added a commit: Restricted Diffusion Commit.