lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
250 stars 161 forks source link

Delete related animations in lv_obj_del #195

Closed kisvegabor closed 2 years ago

kisvegabor commented 2 years ago

In lv_obj_del() the destructor of lv_obj is called, in which lv_anim_del(obj, NULL); is called to delete the animations related to the object being deleted.

It's working because when the user calls lv_anim_set_var(&a, obj) the animation will be associated with the object, so we can find and delete all the animation related to a given obejct.

However, in MP the user uses lv_anim_set_custom_exec_cb which sets a->var = a therefore there is no way to figure out which variable is related to the animation.

Reproduced here.

Do you have any idea how to handle this?

amirgon commented 2 years ago

At least we have LvReferenceError now, so the application doesn't simply crash.

So one approach is to move the responsibility to the user - to catch the exception and delete the animation.
Handling non-error scenarios by raising/catching exceptions is a common technique in Python, (See EAFP).

Such exception handling can be automated by a simple Python Decorator like this:

def anim_cb(cb):
    def wrapper(a, obj, v):
        try:
            cb(obj, v)
        except lv.LvReferenceError:
            print("Animation deleted!")
            a.custom_del(None)
    return wrapper

@anim_cb
def anim_x_cb(obj, v):
    obj.set_x(v)

@anim_cb
def anim_size_cb(obj, v):    
    obj.set_size(v, v)

...

a1.set_custom_exec_cb(lambda a,val: anim_size_cb(a, obj, val))
lv.anim_t.start(a1)

Here is an example.

Another option is to add another field to lv_anim_t to hold a pointer to the object, but the user would have to remember to set it.

It's working because when the user calls lv_anim_set_var(&a, obj) the animation will be associated with the object, so we can find and delete all the animation related to a given obejct.

But even in C, this is not very robust. If the user uses some proxy/wrapper object as "var" you will not find the LVGL object and have the same problem - the animation would work correctly but deleting the object would not delete the animation.
I think that this is the only place that you assume that var is really an LVGL object and not anything else, right?

kisvegabor commented 2 years ago

Thanks for the example.

But even in C, this is not very robust. If the user uses some proxy/wrapper object as "var" you will not find the LVGL object and have the same problem

It's true, but at least the most common case (animate a single object) is working well in C. To support at least these cases in MP we should keep var. What if we modified lv_anim_set_custom_exec_cb:

static inline void lv_anim_set_custom_exec_cb(lv_anim_t * a, lv_anim_custom_exec_cb_t exec_cb)
{
    //a->var     = a;  let the user set var
    a->exec_cb = (lv_anim_exec_xcb_t)exec_cb;
    a->custom_exec_cb_set = 1;  
}

When exec_cb is called LVGL can decide whether to pass a->var or a based on custom_exec_cb_set.

What do you think?

amirgon commented 2 years ago

To support at least these cases in MP we should keep var

Do you expect the user to update var just for this corner case? var has no purpose in animation (when using custom_exec_cb), apart from the case of automatically deleting the animation. Even it's name "var" doesn't imply it should be the LVGL object being animated. (a better name would be obj...) So my guess is that Micropython users might not bother to assign var when creating an animation.

What if we modified lv_anim_set_custom_exec_cb

This could work, but only on cases where the user remembers explicitly setting var to the LVGL object. So I'm not sure it's worth the trouble, especially when this can be handled in Python as in the example above.

kisvegabor commented 2 years ago

Using lambda expressions in lv_anim_set_custom_exec_cb is only one way to set a custom exec_cb. In C if you set such callback (for whatever reasons) probably you want to set a var too t know which object to animate.

So maybe var is not required in MP (if you use lambda) and there is a technique to catch such problems, it's not general.

Anyway, I don't insists on adding this feature, just wanted to know your opinion about this problem. So I think we can close this issue. Thank you again.