enthought / traits

Observable typed attributes for Python classes
Other
432 stars 85 forks source link

Remove "changed" from TraitDict change event #1031

Open kitchoi opened 4 years ago

kitchoi commented 4 years ago

Currently when a TraitDictObject is mutated, the change event contains the following: https://github.com/enthought/traits/blob/99d83bd8134bcb97b991724fde7bb5d6a24962de/traits/trait_dict_object.py#L38-L45

Instead of having these three mutually exclusive sets of values, we could just have two sets: added and removed. Keys that are found in both added and removed represent changed items. I.e. something like:

    added : dict
        Keys for added or updated items. Values are current.  
    removed : dict
        Keys for removed or updated items. Values are no longer in the dict.

This API will likely result in cleaner change handler code. For example, the block for event.changed in the following example from mayavi can be removed:

        try:
            for obj, actors in event.removed.items():
                self._remove_actors_widgets(actors)
            for obj, actors in event.added.items():
                self._add_actors_widgets(actors)
            for obj, actors in event.changed.items():
                # The actors in the event are the old ones. Grab the new ones
                # from the actor map itself.
                self._remove_actors_widgets(actors)
                self._add_actors_widgets(self.value[obj])
        finally:
            scene.disable_render = old_disable_render
            scene.render()

There are also instances where the meaning of changed was misinterpreted, from traitsui:

        # Handle the added and changed items
        groups_to_set = event.added
        groups_to_set.update(event.changed)
        styles = getattr(self.object, self.factory.styles_trait, None)
        self._apply_styles(styles, groups_to_set)

        # Handle the removed items by resetting them
        [
            list(map(self._reset_formatting, dates))
            for dates in event.removed.values()
        ]

For backward compatibility, handlers using on_trait_change can continue receiving added, removed and changed unchanged.

For the observe framework (see #977), it is a new framework that users can opt-in when they are ready. This presents an opportunity to improve the API.

kitchoi commented 4 years ago

Sorry for those receiving an email notification containing virtually no content. I hit some magic keybinding on GH which caused the issue to be submitted early. The issue is now updated.

mdickinson commented 4 years ago

The other part of this is that in the current TraitDictEvent, we have incomplete information: the added and removed parts are complete, but for the changed part, the event receiver gets the old values in the dict, but has to explicitly look up the new values. It's a surprising API that's easy to accidentally misuse (as the examples @kitchoi found show). The proposed change would mean that the event would contain complete information about the changes.

mdickinson commented 4 years ago

the event receiver gets the old values in the dict, but has to explicitly look up the new values

This has also always felt rather fragile to me: instead of information about the changes being generated when the changes happen, we look up those new values later. It's quite possible for the dictionary to have changed in the meantime (perhaps as the result of some other listener, or perhaps as the result of something horrible concurrency-related), so that the lookups fail.