Backport PRs #421, #422 and #423 (#427)

* Revert PR #354 (#422)

* Revert "Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] (#354)"

This reverts commit d62af08.

* TST/FIX : Revert test that used observe to listen to changes to

extension point items. The test now uses a static trait change handler
like before

	modified:   envisage/tests/test_plugin.py

* Regression test for #417 (#421)

* Add failing test

* STY: Fix line spacing issues

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

* Fix test suite dependence on ipykernel (#423)

* Fix hard test suite dependence on ipykernel

* Update envisage/tests/test_ids.py

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

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

* DOC : Update changelog

	modified:   CHANGES.rst

Co-authored-by: Mark Dickinson <[email protected]>

Author: Poruri Sai Rahul

CHANGES.rst

@@ -2,6 +2,28 @@
  Envisage CHANGELOG
 ====================
 
+Version 6.0.1
+=============
+
+Released: 2021-06-18
+
+This bugfix release fixes the issue where Extension Point resolution was
+happening too eagerly, which caused issues during application startup time in
+certain cases. We recommend all users of Envisage to upgrade to this bugfix
+version.
+
+Fixes
+-----
+
+- Revert PR #354, which caused the issue #417. (#422)
+
+Tests
+-----
+
+- Ensure that the testsuite passes with minimal dependencies. (#423)
+- Add a regression test for issue #417. (#421)
+
+
 Version 6.0.0
 =============
 

envisage/extension_point.py

@@ -11,14 +11,11 @@
 
 
 # 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
@@ -118,14 +115,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)
 
-        value = obj.__dict__[cache_name]
-        # validate again
-        self.trait_type.validate(obj, trait_name, value[:])
-        return value
+        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)
 
     def set(self, obj, name, value):
         """ Trait type setter. """
@@ -154,41 +151,21 @@ def connect(self, obj, trait_name):
         """
 
         def listener(extension_registry, 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:
-                # We know where in the list is changed.
+            """ Listener called when an extension point 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
-                )
+            # If an index was specified then we fire an '_items' changed event.
+            if event.index is not None:
+                name = trait_name + "_items"
+                old = Undefined
+                new = event
 
+            # Otherwise, we fire a normal trait changed event.
             else:
-                # The entire list has changed. We reset the cache and fire a
-                # normal trait changed event.
-                _update_cache(obj, trait_name)
+                name = trait_name
+                old = event.removed
+                new = event.added
 
-        # 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)
+            obj.trait_property_changed(name, old, new)
 
         extension_registry = self._get_extension_registry(obj)
 
@@ -232,166 +209,3 @@ 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)

envisage/tests/test_application.py

@@ -111,6 +111,53 @@ class PluginC(Plugin):
     x = List(Int, [98, 99, 100], contributes_to="a.x")
 
 
+# PluginD and PluginE each contribute to the other's extension points, but both
+# expect to be started before contributions are made.
+# xref: enthought/envisage#417
+
+
+class PluginD(Plugin):
+    """ Plugin that expects to be started before contributing to
+    extension points. """
+
+    id = "D"
+    x = ExtensionPoint(List, id="d.x")
+
+    y = List(Int, contributes_to="e.x")
+
+    started = Bool(False)
+
+    def start(self):
+        self.started = True
+
+    def _y_default(self):
+        if self.started:
+            return [4, 5, 6]
+        else:
+            return []
+
+
+class PluginE(Plugin):
+    """ Another plugin that expects to be started before contributing to
+    extension points. """
+
+    id = "E"
+    x = ExtensionPoint(List, id="e.x")
+
+    y = List(Int, contributes_to="d.x")
+
+    started = Bool(False)
+
+    def start(self):
+        self.started = True
+
+    def _y_default(self):
+        if self.started:
+            return [1, 2, 3]
+        else:
+            return []
+
+
 class ApplicationTestCase(unittest.TestCase):
     """ Tests for applications and plugins. """
 
@@ -265,6 +312,27 @@ def test_extension_point(self):
         self.assertEqual(6, len(extensions))
         self.assertEqual([1, 2, 3, 98, 99, 100], extensions)
 
+    def test_extension_point_resolution_occurs_after_plugin_start(self):
+        # Regression test for enthought/envisage#417
+
+        # Given
+        d = PluginD()
+        e = PluginE()
+        application = TestApplication(plugins=[d, e])
+
+        # When
+        application.start()
+
+        # Then
+        self.assertEqual(
+            application.get_extensions("d.x"),
+            [1, 2, 3],
+        )
+        self.assertEqual(
+            application.get_extensions("e.x"),
+            [4, 5, 6],
+        )
+
     def test_add_extension_point_listener(self):
         """ add extension point listener """