Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bindableProperty of an object's copy is not bindable anymore #3995

Open
balex89 opened this issue Nov 15, 2024 · 0 comments
Open

bindableProperty of an object's copy is not bindable anymore #3995

balex89 opened this issue Nov 15, 2024 · 0 comments

Comments

@balex89
Copy link

balex89 commented Nov 15, 2024

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
        ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant