This block allows us to pass in some data via user_data, and fetch it back out when calling the callback. This works great for writing language bindings, but comes with a notable drawback: the var is overwritten, causing the object-being-animated to no longer clean up the animation automatically during lv_obj_del.
Would it be possible to consolidate the custom and non-custom callbacks into one:
Do not overwrite a->var
Always pass lv_anim_t* via the animation callback to the callback function
Users can fetch the object being animated via a->var in the callback function
By not overwriting a->var we can avoid two tricky bugs: (1) the location of your lv_anim_t struct cannot move in memory before calling lv_anim_start, and (2) we retain the automatic deletion of animations during object deletion.
I agree that it’s a problem. I think the simplest would be to add an lv_anim_custom_exec_cb_t custom_exec_cb member to lv_anim_t and the animation engine would call the one which is was set.
I would like to implement this for the upcomming V9 release.
I have one question:
The function lv_anim_custom_delete is now a thing wrapper around the lv_anim_delete since before that change the callback will always be stored in the exec_cb struct field of the animation.
If we add a new field, the lv_anim_delete function cannot be used anymore.
In my optionen we have to options:
duplicate the lv_anim_delete and change one line io compare against the new field instead of the exec_cb.
refatore the lv_anim_delete function to create a function which does the looping over all animations gets a flag to indicate whether to compare agains the exec_cb field or the custom_exec_cb field
I personally would favour the 2 options since I’m not a fan of code duplication.
BTW: I just realized that the same applies for lv_anim_custom_get vs lv_anim_get and maybe some other functions as well.
As far as I understood the original problem the implementation will not solve the problem because when you set the custom callback with lv_anim_set_custom_exec_cb the internal variable will be set to the animation.
When you than try to delete all animations which belong to an object by calling
lv_anim_del(obj, NULL)
it will not find the animations with the custom callback because the variable no longer points to the object but to the animation instance.
The question is: Why do you need to set
a->var = a
in lv_anim_set_custom_exec_cb?
This is no longer neccessary because now you have two different fields for the callbacks in the animation.
And if you don’t overrwrite the a->var in the lv_anim_set_custom_exec_cb function that the original problem of Cameron_Pickett is solved.
The second issue is what I pointed out.
When you try to delete animation by specifying the variable AND the callback.
The current implementation looks like this:
bool lv_anim_delete(void * var, lv_anim_exec_xcb_t exec_cb)
{
lv_anim_t * a;
bool del_any = false;
a = _lv_ll_get_head(anim_ll_p);
while(a != NULL) {
bool del = false;
if((a->var == var || var == NULL) && (a->exec_cb == exec_cb || exec_cb == NULL)) {
_lv_ll_remove(anim_ll_p, a);
if(a->deleted_cb != NULL) a->deleted_cb(a);
lv_free(a);
anim_mark_list_change(); /*Read by `anim_timer`. It need to know if a delete occurred in
the linked list*/
del_any = true;
del = true;
}
/*Always start from the head on delete, because we don't know
*how `anim_ll_p` was changes in `a->deleted_cb` */
a = del ? _lv_ll_get_head(anim_ll_p) : _lv_ll_get_next(anim_ll_p, a);
}
return del_any;
}
With this function you cannot delete an animation by checking the custom_exec_cb
Therefore my question of how the delete function should be implemented.
I think too that the best way is to have both functions.
The only problem I have is that the code for both functions is highly redundant, basically only one line is differnt.
So may question is, should we
duplicate the code or
create a function where we pass the information what we want to compare as parameter and the make two inline wrappers for the two functions:
static bool _anim_delete(void * var, void * cb, bool custom_callback)
{
lv_anim_t * a;
bool del_any = false;
a = _lv_ll_get_head(anim_ll_p);
while(a != NULL) {
bool del = false;
if ( (a->var == var || var == NULL)
&& ( !cb
|| ( custom_callback && ((void *) a->exec_cb == cb))
|| (!custom_callback && ((void *) a->custom_exec_cb == cb))
)
) {
_lv_ll_remove(anim_ll_p, a);
if(a->deleted_cb != NULL) a->deleted_cb(a);
lv_free(a);
anim_mark_list_change(); /*Read by `anim_timer`. It need to know if a delete occurred in
the linked list*/
del_any = true;
del = true;
}
/*Always start from the head on delete, because we don't know
*how `anim_ll_p` was changes in `a->deleted_cb` */
a = del ? _lv_ll_get_head(anim_ll_p) : _lv_ll_get_next(anim_ll_p, a);
}
return del_any;
}
bool lv_anim_delete(void * var, lv_anim_exec_xcb_t exec_cb)
{
return _anim_delete(var, (void *)exec_cb, false);
}
bool lv_anim_custom_delete(void * var, lv_anim_custom_exec_cb_t exec_cb)
{
return _anim_delete(var, (void *)exec_cb, true);
}
Personally I prefer the second option to avoid code duplication as much as possible.
I was thinking about it, and I’m not sure it has enough added value to be worth the refactoring and breaking the API. So let’s focus on the custom_exec_cb related update.