Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] (

#354)

* Support @observe("name:items") for ExtensionPoint

This is achieved by caching the value of ExtensionPoint and make it
impossible to mutate the list directly.

* Make sure an object with ExtensionPoint is garbage collectable after disconnecting listeners

* Clean up test and add docstring

* Remove reference back to the object

The reference only serves to support some edge cases where
the extension point values become out-of-sync because the listeners
are not connected yet. Once the listeners are connected the values
are refreshed. So all in all it is not worth keeping the reference,
as one will then need to use weakref to avoid cycle references,
and special handling in the pickling logic to handle that weakref.

* Move functions out of ExtensionPoint to avoid cycle reference between listeners and trait types

* Fix unpickling ExtensionPointValue in newer versions of Python

* Bump traits requirement to 6.1

Co-authored-by: Poruri Sai Rahul <[email protected]>

envisage/extension_point.py

@@ -11,11 +11,14 @@
 
 
 # Standard library imports.
+from functools import wraps
 import inspect
+import warnings
 import weakref
 
 # Enthought library imports.
 from traits.api import List, TraitType, Undefined, provides
+from traits.trait_list_object import TraitList
 
 # Local imports.
 from .i_extension_point import IExtensionPoint
@@ -156,14 +159,14 @@ def __repr__(self):
 
     def get(self, obj, trait_name):
         """ Trait type getter. """
+        cache_name = _get_cache_name(trait_name)
+        if cache_name not in obj.__dict__:
+            _update_cache(obj, trait_name)
 
-        extension_registry = self._get_extension_registry(obj)
-
-        # Get the extensions to this extension point.
-        extensions = extension_registry.get_extensions(self.id)
-
-        # Make sure the contributions are of the appropriate type.
-        return self.trait_type.validate(obj, trait_name, extensions)
+        value = obj.__dict__[cache_name]
+        # validate again
+        self.trait_type.validate(obj, trait_name, value[:])
+        return value
 
     def set(self, obj, name, value):
         """ Trait type setter. """
@@ -192,21 +195,41 @@ def connect(self, obj, trait_name):
         """
 
         def listener(extension_registry, event):
-            """ Listener called when an extension point is changed. """
-
-            # If an index was specified then we fire an '_items' changed event.
+            """ Listener called when an extension point is changed.
+
+            Parameters
+            ----------
+            extension_registry : IExtensionRegistry
+                Registry that maintains the extensions.
+            event : ExtensionPointChangedEvent
+                Event created for the change.
+                If the event.index is None, this means the entire extensions
+                list is set to a new value. If the event.index is not None,
+                some portion of the list has been modified.
+            """
             if event.index is not None:
-                name = trait_name + "_items"
-                old = Undefined
-                new = event
+                # We know where in the list is changed.
+
+                # Mutate the _ExtensionPointValue to fire ListChangeEvent
+                # expected from observing item change.
+                getattr(obj, trait_name)._sync_values(event)
+
+                # For on_trait_change('name_items')
+                obj.trait_property_changed(
+                    trait_name + "_items", Undefined, event
+                )
 
-            # Otherwise, we fire a normal trait changed event.
             else:
-                name = trait_name
-                old = event.removed
-                new = event.added
+                # The entire list has changed. We reset the cache and fire a
+                # normal trait changed event.
+                _update_cache(obj, trait_name)
 
-            obj.trait_property_changed(name, old, new)
+        # In case the cache was created first and the registry is then mutated
+        # before this ``connect`` is called, the internal cache would be in
+        # an inconsistent state. This also has the side-effect of firing
+        # another change event, hence allowing future changes to be observed
+        # without having to access the trait first.
+        _update_cache(obj, trait_name)
 
         extension_registry = self._get_extension_registry(obj)
 
@@ -250,3 +273,166 @@ def _get_extension_registry(self, obj):
             )
 
         return extension_registry
+
+
+def _warn_if_not_internal(func):
+    """ Decorator for instance methods of _ExtensionPointValue such that its
+    effect is nullified if the function is not called with the _internal_use
+    flag set to true.
+    """
+
+    @wraps(func)
+    def decorated(object, *args, **kwargs):
+        if not object._internal_use:
+            warnings.warn(
+                "Extension point cannot be mutated directly.",
+                RuntimeWarning,
+                stacklevel=2,
+            )
+            # This restores the existing behavior where the operation
+            # is acted on a list object that is not persisted.
+            return func(TraitList(iter(object)), *args, **kwargs)
+        return func(object, *args, **kwargs)
+
+    return decorated
+
+
+class _ExtensionPointValue(TraitList):
+    """ _ExtensionPointValue is the list being returned while retrieving the
+    attribute value for an ExtensionPoint trait.
+
+    This list returned for an ExtensionPoint acts as a proxy to query
+    extensions in an ExtensionRegistry for a given extension point id. Users of
+    ExtensionPoint expect to handle a list-like object, and expect to be able
+    to listen to "mutation" on the list. The ExtensionRegistry remains to be
+    the source of truth as to what extensions are available for a given
+    extension point ID.
+
+    Users are not expected to mutate the list directly. All mutations to
+    extensions are expected to go through the extension registry to maintain
+    consistency. With that, all methods for mutating the list are nullified,
+    unless it is used internally.
+
+    The requirement to support ``observe("name:items")`` means this list,
+    associated with `name`, cannot be a property that gets recomputed on every
+    access (enthought/traits#624), it needs to be cached. As with any
+    cached quantity, it needs to be synchronized with the ExtensionRegistry.
+
+    Note that the list can only be synchronized with the extension registry
+    when the listeners are connected (see ``ExtensionPoint.connect``).
+
+    Parameters
+    ----------
+    iterable : iterable
+        Iterable providing the items for the list
+    """
+
+    def __new__(cls, *args, **kwargs):
+        # Methods such as 'append' or 'extend' may be called during unpickling.
+        # Initialize internal flag to true which gets changed back to false
+        # in __init__.
+        self = super().__new__(cls)
+        self._internal_use = True
+        return self
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        # Flag to control access for mutating the list. Only internal
+        # code can mutate the list. See _sync_values
+        self._internal_use = False
+
+    def _sync_values(self, event):
+        """ Given an ExtensionPointChangedEvent, modify the values in this list
+        to match. This is an internal method only used by Envisage code.
+
+        Parameters
+        ----------
+        event : ExtenstionPointChangedEvent
+            Event being fired for extension point values changed (typically
+            via the extension registry)
+        """
+        self._internal_use = True
+        try:
+            if isinstance(event.index, slice):
+                if event.added:
+                    self[event.index] = event.added
+                else:
+                    del self[event.index]
+            else:
+                slice_ = slice(
+                    event.index, event.index + len(event.removed)
+                )
+                self[slice_] = event.added
+        finally:
+            self._internal_use = False
+
+    __delitem__ = _warn_if_not_internal(TraitList.__delitem__)
+    __iadd__ = _warn_if_not_internal(TraitList.__iadd__)
+    __imul__ = _warn_if_not_internal(TraitList.__imul__)
+    __setitem__ = _warn_if_not_internal(TraitList.__setitem__)
+    append = _warn_if_not_internal(TraitList.append)
+    clear = _warn_if_not_internal(TraitList.clear)
+    extend = _warn_if_not_internal(TraitList.extend)
+    insert = _warn_if_not_internal(TraitList.insert)
+    pop = _warn_if_not_internal(TraitList.pop)
+    remove = _warn_if_not_internal(TraitList.remove)
+    reverse = _warn_if_not_internal(TraitList.reverse)
+    sort = _warn_if_not_internal(TraitList.sort)
+
+
+def _get_extensions(object, name):
+    """ Return the extensions reported by the extension registry for the
+    given object and the name of a trait whose type is an ExtensionPoint.
+
+    Parameters
+    ----------
+    object : HasTraits
+        Object on which an ExtensionPoint is defined
+    name : str
+        Name of the trait whose trait type is an ExtensionPoint.
+
+    Returns
+    -------
+    extensions : list
+        All the extensions for the extension point.
+    """
+    extension_point = object.trait(name).trait_type
+    extension_registry = extension_point._get_extension_registry(object)
+
+    # Get the extensions to this extension point.
+    return extension_registry.get_extensions(extension_point.id)
+
+
+def _get_cache_name(trait_name):
+    """ Return the attribute name on the object for storing the cached
+    extension point value associated with a given trait.
+
+    Parameters
+    ----------
+    trait_name : str
+        The name of the trait for which ExtensionPoint is defined.
+    """
+    return "__envisage_{}".format(trait_name)
+
+
+def _update_cache(obj, trait_name):
+    """ Update the internal cached value for the extension point and
+    fire change event.
+
+    Parameters
+    ----------
+    obj : HasTraits
+        The object on which an ExtensionPoint is defined.
+    trait_name : str
+        The name of the trait for which ExtensionPoint is defined.
+    """
+    cache_name = _get_cache_name(trait_name)
+    old = obj.__dict__.get(cache_name, Undefined)
+    new = (
+        _ExtensionPointValue(
+            _get_extensions(obj, trait_name)
+        )
+    )
+    obj.__dict__[cache_name] = new
+    obj.trait_property_changed(trait_name, old, new)