[Elisa-commits] [MERGE] replace observables with bindable models

[Alessandro Decina]

On Tue, Apr 22, 2008 at 3:11 PM, Benjamin Kampmann <benjamin at fluendo.com> wrote:
> This patch adds the bindable under core/utils and uses them in the model.
>  It also removes all the observable part (the media providers are probably
>  broken because of missing imports now).
>
>  As we are about the change the Controllers and Views anyway, I didn't
>  reimplement the automatic binding on model__set and controller__set.
>

bb:tweak

=== added file 'elisa-core/elisa/core/tests/test_bindable.py'
--- elisa-core/elisa/core/tests/test_bindable.py	1970-01-01 00:00:00 +0000
> +++ elisa-core/elisa/core/tests/test_bindable.py	2008-04-22 12:48:01 +0000

The tests look ok bug are really long. Can't they be split in
smaller tests?


> +class Bindable(Loggable):
[snip]
> +    def bind(self, attribute, destination_object, destination_attribute):
[snip]
> +        if not self._bindings.has_key(attribute):
> +            # no binding yet for attribute
> +            self._bindings[attribute] = []
> +
> +        self._bindings[attribute].append(binding)

This should be self._bindings.setdefault(attribute, []).append(binding)


> +    def unbind(self, attribute, destination_object, destination_attribute):
[snip]
> +        try:
> +            bindings = self._bindings[attribute]
> +        except KeyError:
> +            raise NotBoundError
> +
> +        self.debug("unbinding %s to %s of %s" %
> +                    (attribute, destination_attribute, destination_object))
> +
> +        strong_bindings = [(b[0](), b[1]) for b in bindings]
> +        try:
> +            i = strong_bindings.index((destination_object,
> +                                       destination_attribute))
> +            bindings.pop(i)
> +        except ValueError:
> +            raise NotBoundError

The raise should be raise NotBoundError(), but it's only a
style thing.

The list comprehension is not necessary here, I would do:

for index, (weak_dest, dest_attribute) in enumerate(bindings):
    if (weak_dest(), dest_attribute) == \
        (destination_object, destination_attribute):
        break
else:
    raise NotBoundError()

del bindings[index]


> +    def unbind_object(self, destination_object):
> +        """
> +        Remove all the bindings you have for a certain L{destination_object}.
> +        """
> +        self.debug("unbinding object %s" % destination_object)
> +
> +        for attribute, bindings in self._bindings.iteritems():
> +            refs = [b[0]() for b in bindings]
> +            while True:
> +                try:
> +                    i = refs.index(destination_object)
> +                    bindings.pop(i)
> +                    refs.pop(i)
> +                except ValueError:
> +                    # no more bindings to destination_object for attribute
> +                    break

It's faster if you change the inner while loop in:

deleted = 0
for index, (weak_destination, dest_attribute) in enumerate(list(bindings)):
    if weak_destination() == destination_object:
        del bindings[index - deleted]
        deleted += 1

> +    def __setattr__(self, attribute, new_value):
> +        super(Bindable, self).__setattr__(attribute, new_value)
> +
> +        if attribute.startswith('_'):
> +            return
> +
> +        try:
> +            bindings = self._bindings[attribute]
> +        except KeyError:
> +            # attribute not bound
> +            return
> +
> +        for binding in bindings:
> +            weak_destination, destination_attribute = binding
> +            destination_object = weak_destination()
> +            try:
> +                obj, attr = get_sub_obj(destination_object, destination_attribute)
> +                setattr(obj, attr, new_value)
> +            except AttributeError, e:
> +                self.warning("Problem in processing the setattr chain: %s" % e)
> +                pass

I don't like to swallow exceptions. When this happens
there's obviously a programming error and it's easier to
spot them if we just let the exception propagate, maybe
raising a new exception with a clearer message.

> +    def __delattr__(self, attribute):
> +        super(Bindable, self).__delattr__(attribute)
> +
> +        try:
> +            bindings = self._bindings[attribute]
> +        except KeyError:
> +            # attribute not bound
> +            return
> +
> +        for binding in bindings:
> +            weak_destination, destination_attribute = binding
> +            destination_object = weak_destination()
> +
> +            try:
> +                obj, attr = get_sub_obj(destination_object, destination_attribute)
> +                delattr(obj, attr)
> +            except AttributeError, e:
> +                self.warning("Problem in processing the delattr chain: %s" % e)
> +                pass

Same here.

Overall looks neat.

Alessandro