traits icon indicating copy to clipboard operation
traits copied to clipboard

Extended trait listeners not called on intermediate list items changes

Open corranwebster opened this issue 4 years ago • 5 comments

Given the following code:

from traits.api import HasTraits, List, Instance, Int

class Child(HasTraits):
    value = Int

class Parent(HasTraits):
    children = List(Instance(Child))

p = Parent()

def printer(object, name, old, new):
    print('Printing', object, name, old, new)

def notifier():
    print('Changed')

p.on_trait_change(printer, 'children.value')
p.on_trait_change(notifier, 'children.value')
p.children = [Child()]
p.children.append(Child(value=10))

Notice that both listeners fire on change to the entire list, but only the one with no arguments fires on modification of the list. The documentation says that it should fire:

In the case of List or Dict traits, ... The handler routine is also invoked when items are added or removed from a list or dictionary, because this is treated as an implied change to the item’s trait being monitored.

This is presumably a long-standing bug, as this test comments and has a FIXME for this issue https://github.com/enthought/traits/blob/master/traits/tests/test_extended_trait_change.py#L467

Given the structure of the test here https://github.com/enthought/traits/blob/master/traits/tests/test_extended_trait_change.py#L462-L464 the expectation is that it should get a standard TraitListEvent for the change.

Feel free to delete if this is a duplicate.

corranwebster avatar Nov 03 '19 10:11 corranwebster

Note that it does work for more deeply nested listeners:

from traits.api import HasTraits, List, Instance, Int

class GrandChild(HasTraits):
    value = Int

class Child(HasTraits):
    children = List(Instance(GrandChild))
    value = Int

class Parent(HasTraits):
    children = List(Instance(Child))

p = Parent(children=[Child()])

def printer(object, name, old, new):
    print('Printing', object, name, old, new)

def notifier():
    print('Changed')

p.on_trait_change(printer, 'children.children.value')
p.on_trait_change(notifier, 'children.children.value')
p.children[0].children = [GrandChild()]
p.children[0].children.append(GrandChild(value=10))

However, some digging indicates that this is related to #537

I am pretty sure that the problem is that this line: https://github.com/enthought/traits/blob/master/traits/traits_listener.py#L810 should also include SRC_LISTENER (or perhaps better, be != DST_LISTENER, which can't happen, so a bare else is good enough).

corranwebster avatar Nov 03 '19 10:11 corranwebster

A similar issue likely occurs for Dict traits and probably needs a similar fix on this line: https://github.com/enthought/traits/blob/master/traits/traits_listener.py#L909

corranwebster avatar Nov 03 '19 10:11 corranwebster

I think this is probably related this failure of assertTraitChanged to catch a trait change using the same extended trait name that an @on_trait_change manages to catch? (I can open another issue for this if it's sufficiently different)

This test would fail if the commented out assertion was added:

import unittest

from traits.api import HasTraits, Instance, Int, List, on_trait_change
from traits.testing.unittest_tools import UnittestTools

# Traits 5.1.2


class ChildClass(HasTraits):
    count = Int


class ParentClass(HasTraits):
    children = List(Instance(ChildClass))

    total = Int

    @on_trait_change("children.count")
    def update_total(self):
        self.total = sum(child.count for child in self.children)


class TestParentClass(unittest.TestCase, UnittestTools):
    def test_trait_changes(self):

        ex_inst = ParentClass(
            children=[ChildClass(count=3)]
        )

        self.assertEqual(ex_inst.total, 3)

        with self.assertTraitChanges(ex_inst, "children.count"), \
                self.assertTraitChanges(ex_inst, "total"):
            ex_inst.children[0].count = 5
        self.assertEqual(ex_inst.total, 5)

        #with self.assertTraitChanges(ex_inst, "children.count") FAILS WITH NOT FIRED
        with self.assertTraitChanges(ex_inst, "total"):
            ex_inst.children[0] = ChildClass(count=7)
        self.assertEqual(ex_inst.total, 7)  # Trait change was fired

        with self.assertTraitChanges(ex_inst, "children.count"), \
                self.assertTraitChanges(ex_inst, "total"):
            ex_inst.children = [ChildClass(count=8)]
        self.assertEqual(ex_inst.total, 8)

Another view:

from traits.util.event_tracer import record_events

ex_inst = ParentClass(
    children=[ChildClass(count=3)]
)

with record_events() as rec:
    ex_inst.children[0].count = 9
    ex_inst.children[0] = ChildClass(count=7)
    ex_inst.children = [ChildClass(count=8)]

rec.save_to_directory("./")
2019-11-05 01:04:19.391310 -> 'count' changed from 3 to 9 in 'ChildClass'
2019-11-05 01:04:19.391310     CALLING: 'update_total' in assert_trait_change.py
2019-11-05 01:04:19.391513 <- EXIT: 'update_total'

2019-11-05 01:04:19.391545 -> 'children_items' changed from <undefined> to <traits.trait_handlers.TraitListEvent object at 0x117ce6470> in 'ParentClass'
2019-11-05 01:04:19.391545     CALLING: 'update_total' in assert_trait_change.py
2019-11-05 01:04:19.391571 <- EXIT: 'update_total'

2019-11-05 01:04:19.391631 -> 'children' changed from [<__main__.ChildClass object at 0x117cd1a40>] to [<__main__.ChildClass object at 0x117cd1af0>] in 'ParentClass'
2019-11-05 01:04:19.391631     CALLING: 'update_total' in assert_trait_change.py
2019-11-05 01:04:19.391654 <- EXIT: 'update_total'

However:

ex_inst = ParentClass(
    children=[ChildClass(count=3)]
)

def printer(object, name, old, new):
    print('Printing', object, name, old, new)

def notifier():
    print('Changed')

ex_inst.on_trait_change(printer, 'children.count')
ex_inst.on_trait_change(notifier, 'children.count')

ex_inst.children[0].count = 9
print()
ex_inst.children[0] = ChildClass(count=7)
print()
ex_inst.children = [ChildClass(count=8)]
Printing <__main__.ChildClass object at 0x10c293af0> count 3 9
Changed

Changed

Printing <__main__.ParentClass object at 0x10c293b48> children [<__main__.ChildClass object at 0x10c2a1938>] [<__main__.ChildClass object at 0x10c293af0>]
Changed

k2bd avatar Nov 05 '19 01:11 k2bd

#621 fixes this issue (and seems to be the correct fix), but it turned out to be too disruptive to existing Traits-using projects. We need to find a sane upgrade path for projects that allows those projects time to make appropriate fixes.

See also #537.

mdickinson avatar Jan 03 '20 08:01 mdickinson

For reference:

This issue does not apply to observe. The example in the main comment would be written like this with observe:

from traits.api import HasTraits, List, Instance, Int

class Child(HasTraits):
    value = Int

class Parent(HasTraits):
    children = List(Instance(Child))

p = Parent()

def printer(event):
    print('Printing', event)


p.observe(printer, 'children.items.value')
p.children = [Child()]
p.children.append(Child(value=10))

will result in this output:

Printing TraitChangeEvent(object=<__main__.Parent object at 0x7fa32ff75f90>, name='children', old=[], new=[<__main__.Child object at 0x7fa32ff7e450>])
Printing ListChangeEvent(object=[<__main__.Child object at 0x7fa32ff7e450>, <__main__.Child object at 0x7fa32ff7e950>], index=1, removed=[], added=[<__main__.Child object at 0x7fa32ff7e950>])

The issue remains valid for on_trait_change, however.

kitchoi avatar Nov 06 '20 12:11 kitchoi