envisage icon indicating copy to clipboard operation
envisage copied to clipboard

Support for observe in ExtensionPoint

Open kitchoi opened this issue 4 years ago • 5 comments

Traits 6.1 introduces a new trait notification/observation framework: observe. The mini-language for this framework has changed for observing mutations in a container such as a list.

Currently ExtensionPoint emits property change event using a trait named name and name_items. This allows users to use on_trait_change as if an ExtensionPoint was a List and listen to "mutations" to it even though the list is never cached. In other words, it behaves as if it was two Property combined. Changing on_trait_change("name_items") to observe("name:items") won't work as expected. This issue is about how to make ExtensionPoint be compatible with observe.

I might be able to put together a small example and post here later.

kitchoi avatar Jun 15 '20 09:06 kitchoi

I think we need to explore this for the 5.0.0 release (doesn't have to be merged into master) because it might bring up potential issues when we move to using traits observe. Adding this issue to the 5.0.0 milestone.

rahulporuri avatar Oct 19 '20 13:10 rahulporuri

For reference, I think observe("name_items") should still work, but the event object may not be what you think it is. name_items is just another trait, it is named as such to imitate on_trait_change mini-language.

kitchoi avatar Oct 19 '20 13:10 kitchoi

I have so far attempted two approaches, neither were successful:

(1) Reimplement ExtensionRegistry and friends with observe This is futile because in order to support @observe("name:items") where name is defined as an ExtensionPoint, we need the name to refer to a persisted TraitList or an instance of HasTraits that has a trait named "items". But that can't happen, see below.

(2) Caching the TraitListObject returned by ExtensionPoint.get. While this supports @observe("name:items"), this would be a wrong fix because the list returned by the ExtensionPoint is NOT supposed to be mutated. If I try to cache the list to support @observe("name:items"), this test I added in #346 will fail: https://github.com/enthought/envisage/blob/71a913c03ba8f3ec05cce968a76ca56db1c5c151/envisage/tests/test_extension_point.py#L114-L137

In particular, it would fail on the very last line with [1, 2, 3, 42] != [1, 2, 3]. Much of envisage code assumes that changes to the extension points must go through the registry mechanism. Allowing the extension point to be mutated directly will bleach that assumption.

As I said in #346, I think the naming of "name_items" is unfortunate: It is not the same as "name_items" in Traits's on_trait_change framework (as is demonstrated by this other test in #346). Had it been called something else, one would not have tried to convert it to "name:items" when migrating to observe. We should still find a solution to support such a tempting migration, as the name attribute is never truly set, @observe("name:items") will simply do nothing and it will be very hard to detect unless one has good test coverage on the logic.

My next idea is for ExtensionPoint.get to return an instance of HasTraits that also implements the interface of a sequence, and then we can define a trait called "items" on the instance to continue with this API.

kitchoi avatar Oct 26 '20 17:10 kitchoi

My next idea is for ExtensionPoint.get to return an instance of HasTraits that also implements the interface of a sequence, and then we can define a trait called "items" on the instance to continue with this API.

That idea also does not work because although an event can be fired for @observe("name:items"), the event is not a ListChangeEvent but a TraitChangeEvent. Developers using this API is going to expect a list-like behaviour and expect a ListChangeEvent like one would if the value is actually a TraitList.

At the end, I have to subclass TraitList and have it cached on the object, and at the same time, prevent mutations to occur directly on the list. PR coming...

kitchoi avatar Oct 28 '20 18:10 kitchoi

We had to revert #354; re-opening this issue accordingly.

mdickinson avatar Feb 06 '23 19:02 mdickinson