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

Listeners on extended trait change handlers firing unexpectedly on List trait items #537

Open
corranwebster opened this issue Nov 1, 2019 · 3 comments
Labels
topic: traits listener rework Issues related to reworking listener infrastructure; see also EEP2, EEP3 type: bug
Milestone

Comments

@corranwebster
Copy link
Member

corranwebster commented Nov 1, 2019

Given the following code:

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

class Child(HasTraits):
    values = List(Int)

class Parent(HasTraits):
    child = Instance(Child)
    values = List(Int)


p = Parent(
    child=Child(values=[1, 2, 3]),
    values=[4, 5, 6],
)


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


p.on_trait_change(printer, 'values')
print("Change list items, listener doesn't fire values_items, as expected")
p.values[1] = 100

p.on_trait_change(printer, 'child.values')
print("Change list items on an instance attr, listener fires child.values_items!")
p.child.values[1] = 100

and based on the documentation, we don't expect the trait change handler for child.values to fire when the _items fire. Based on the documentation, this should only happen in the case where the trait change handler takes no arguments (see https://docs.enthought.com/traits/traits_user_manual/notification.html#dynamic-handler-special-cases).

This behaviour is the root cause for the TraitsUI bugs described in enthought/traitsui#403 and enthought/traitsui#680 where the unexpected firing of a trait change handler is causing an unwanted refresh of the UI.

Looking into the way that the listeners are set up, it looks like it is the following line of code that is causing the _items change handler to be connected:

elif self.type == ANY_LISTENER:
object._on_trait_change(
handler,
name + "_items",
remove=remove,
dispatch=self.dispatch,
priority=self.priority,
target=self._get_target(),
)

Further, it appears that the issue may be that the value of self.type may not be being assigned correctly;

  • The type trait is derived from the signature of the handler on the ListenerNotifyWrapper class here:
    self.type = ListenerType.get(
  • This is then applied to the listener object here: https://github.com/enthought/traits/blob/master/traits/has_traits.py#L2814-L2821 (by type = lnw.type) along with a number of other properties (handler, dispatch, etc.)
  • Most of the other properties have a _<trait>_changed handleron theListenerItem` which push the changed values through the listener structures recursively: see the methods starting here
    # -- Event Handlers ---------------------------------------------------------
  • The type trait doesn't have a change handler, and so doesn't get propagated recursively.

Adding the following code to ListenerItem resolves the issues described above:

def _type_changed(self, type):
    """ Handles the ``type`` trait being changed.
    """
    if self.next is not None:
        self.next.type = type

However, this appears to be a very long-standing bug (12+ years) so some caution may need to be taken when fixing it that there may be code that relies on the current behaviour.

Edit: simplified the example to remove some unused code.

@corranwebster
Copy link
Member Author

There is a secondary issue, which is that some of this confusion derives from trying to maintain backwards compatibility with Traits 2 when Traits 3 came out. This may be an opportunity to rationalize listener dispatch.

mdickinson pushed a commit that referenced this issue Nov 29, 2019
* Add tests for extended trait change issues #537 and #538.

* Add expected failures for tests that fail expectedly due to #537 and #538

* Remove unused import of Set trait.

* Fix typo in tests

Co-Authored-By: Mark Dickinson <mdickinson@enthought.com>

* Use more meaningful instance class names.

* Better variable names and test order.
@mdickinson
Copy link
Member

mdickinson commented Jan 3, 2020

#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 #538.

@kitchoi
Copy link
Contributor

kitchoi commented Jun 12, 2020

With observe, the event we don't expect to fire is not fired:

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

class Child(HasTraits):
    values = List(Int)

class Parent(HasTraits):
    child = Instance(Child)
    values = List(Int)


p = Parent(
    child=Child(values=[1, 2, 3]),
    values=[4, 5, 6],
)

p.observe(print, 'values')
p.values[1] = 100  # nothing is printed

p.observe(print, 'child.values')
p.child.values[1] = 100  # nothing is printed

If mutation to a list/dict/set is observed, it needs to be explicitly requested, like this:

p.observe(print, 'child.values.items')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: traits listener rework Issues related to reworking listener infrastructure; see also EEP2, EEP3 type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants