bindableProperty of an object's copy is not bindable anymore

[balex89]

Description

If we make a copy.copy() (or a deepcopy()) of an object with a bindableProperty, the resulting copy's bindable attribute is not really bindable anymore (until its value is reassigned manually).

Example

import copy

from nicegui import binding


class MyClass:
    x = binding.BindableProperty()

    def __init__(self):
        self.x = 1


original = MyClass()
duplicate = copy.copy(original)

assert (id(original), 'x') in binding.bindable_properties  # ok
assert (id(duplicate), 'x') in binding.bindable_properties  # AssertionError

Why is that?

The copy module techniques do not invoke __setattr__() at any point, so the BindableProperty.__set__() method never gets to update binding.bindable_properties with the new object's id().

And therefore, when binding, unexpectedly an active link will be created.

A workaround

A possible obvious approach might involve defining __copy__ and __deepcopy__ methods for a target class and updating bindable_properties in the process.
This works just fine for a trivial case. But I personally don't like it much, as it lacks universality and might get trickier if complicated by descriptors or unnecessary __init__() logic.

What copy uses under the hood is the pickle approach. From the docs:

In fact, the copy module uses the registered pickle functions from the copyreg module.

That leads us to the following solution:

import copy
import copyreg
from typing import TypeVar, Type, Any

from nicegui import binding

T = TypeVar('T')


def register_bindables(original_obj: T, copy_obj: T) -> None:
    """ Here we replicate the "`bindable_properties` representation" of an object
    WITHOUT any unnecessary attribute accessing in process.
    """
    for attr_name in dir(original_obj):
        if (id(original_obj), attr_name) in binding.bindable_properties:
            binding.bindable_properties[(id(copy_obj), attr_name)] = copy_obj


def pickle_hooked(obj: Any):
    """ This is a `obj.__reduce__()` wrapper that invoke bindables registration """
    reduced = obj.__reduce__()
    creator = reduced[0]

    def creator_with_hook(*args, **kwargs):
        cls_copy = creator(*args, **kwargs)
        register_bindables(obj, cls_copy)
        return cls_copy

    return (creator_with_hook,) + reduced[1:]


def copyable(cls: Type[T]) -> Type[T]:
    """ Decorate your class with this to register the modified `pickle` function
    Note that if you derive from `cls`, you have to decorate the child class with `@copyable` again.
    """
    copyreg.pickle(cls, pickle_hooked)
    return cls


@copyable
class MyClass:
    x = binding.BindableProperty()

    def __init__(self):
        self.x = 1


original = MyClass()
duplicate = copy.copy(original)

assert (id(original), 'x') in binding.bindable_properties  # ok
assert (id(duplicate), 'x') in binding.bindable_properties  # ok

This also is sufficient for the __deepcopy__() to work right.

Possible solutions

If we would agree on bindable_properties being useful only when binding functions invoked, than we could fix it "on fly" when binding is happening, like so:

if (id(self_obj), self_name) not in bindable_properties:
    if isinstance(object.__getattribute__(type(self_obj), self_name), BindableProperty):
        bindable_properties[(id(self_obj), self_name)] = self_obj
    else:
        active_links.append((self_obj, self_name, other_obj, other_name, forward))

I'm not sure that is the case though. bindable_properties is a public variable, it can be used for, say, gathering binding statistics (like here) and therefore deserves more consistency.
It seems like If we are to stick with bindable_properties dictionary as it is, then we have to update it when copying happens.

Maybe we should keep track of classes, where bindableProperty was ever used, and change the way that class is copied (for instance like was proposed in a workaround above). This might look something like this then:

class BindableProperty:
    ...

    def __set__(self, owner: Any, value: Any) -> None:
        if type(owner) not in bindable_property_owner_types:
            bindable_property_owner_types.add(type(owner))
            make_copyable(type(owner))  # register copy() hook
        ...

[falkoschindler]

Thanks for reporting this problem, @balex89! And thanks for including a workaround.

I've been thinking about it again and again over the past two months and I'm still undecided what to do about it. So far I see three options:

1. Provide a decorator like @binding.copyable to allow the user to fix this problem explicitly if needed.
2. Add bindable properties "on the fly" when invoking a binding function.
3. Automatically make the class copyable when setting a bindable property.

Option 1 is nice because it doesn't require any change to the existing binding implementation, but is hard to discover. Users will probably run into the problem first before noticing the need for this decorator.
Option 2 is nice because it works automatically and requires very little change to the implementation. Sure, the dictionary is filled a little inconsistently, but it still allows precise statements about how many bindable properties are registered - even if copied classes will register their bindable properties later.
Option 3 might be the most correct(?) but seems to be the most complex one and adds an additional dictionary lookup for every write access to a bindable property which could add up.

For simplicity's sake I tend towards option 2. What do you think? Would you like to create a pull request and see how it turns out?