Add notifier for wrapping user handler for the observer framework #1000
Conversation
…he start and the tracked target being deleted
Codecov Report
@@ Coverage Diff @@
## master #1000 +/- ##
=======================================
Coverage 76.22% 76.22%
=======================================
Files 58 58
Lines 6579 6579
Branches 1278 1278
=======================================
Hits 5015 5015
Misses 1218 1218
Partials 346 346 Continue to review full report at Codecov.
|
| dispatcher : callable(handler, event) | ||
| A callable for dispatching the handler, e.g. on a different | ||
| thread or on a GUI event loop. ``event`` is the object | ||
| created by the event factory. |
There was a problem hiding this comment.
I am a bit tempted to group handler, target, dispatcher into another object. They do always go together in other places. That object can provide the methods for equals and for whether or not the weakrefs are still live.
We could also refactor this later when all the other pieces are here.
| self.target = weakref.ref(target) | ||
|
|
||
| if isinstance(handler, types.MethodType): | ||
| self.handler = weakref.WeakMethod(handler) |
There was a problem hiding this comment.
Previously (in the PoC) there was a callback in both weakref for removing this notifier from the observable when either the handler or the target is deleted. That requires keeping a reference to the list of notifiers in the observable. If we don't do the cleanup, we may have some silenced notifiers lying around if the code that puts up the notifiers don't do a good job of taking them down (e.g. traitsui). This may be a bit inefficient, but I am not sure how common this scenario is.
There was a problem hiding this comment.
19 days after making this comment: I am now more in favour of not having a weakref callback for removing the notifier. Without the weakref callback, there is only one way to remove the notifier: remove_from. Currently remove_from raises an exception if the notifier to be removed is not found. This is very helpful for catching programming error. If the code is right, every call to add_to should be undone by a call to remove_from, like open bracket and close bracket. If we had a weakref callback to remove the notifier from the observable, this introduces a second way for notifiers to be removed, from a random thread, and remove_from can no longer raise an exception if the notifier cannot be found.
| return False | ||
| return ( | ||
| self.handler() is other.handler() | ||
| and self.target() is other.target() |
There was a problem hiding this comment.
Another discussion point: I think currently if one calls on_trait_change with the same arguments except for dispatch, the first one wins:
foo.on_trait_change(handler, "name", dispatch="same")
foo.on_trait_change(handler, "name", dispatch="ui")
The second call is ignored because "dispatch" is not considered for differentiating notifiers. Here we could differentiate them, and I am tempted to think that we should. In other words, calling the following will result in two notifiers, one to be dispatched on the same thread, and the second one to be dispatched on the GUI event loop:
obj.observe(handler, "name", dispatch="same")
obj.observe(handler, "name", dispatch="ui")
There was a problem hiding this comment.
I am in favour of this.
There was a problem hiding this comment.
👍 I will add this one line for comparing the dispatch callable (and its test).
| dispatcher : callable(handler, event) | ||
| A callable for dispatching the handler, e.g. on a different | ||
| thread or on a GUI event loop. ``event`` is the object | ||
| created by the event factory. |
There was a problem hiding this comment.
The following comment is still valid but GitHub marked it outdated because I moved the docstring after making the comment: #1000 (comment) :
I am a bit tempted to group handler, target, dispatcher into another object. They do always go together in other places. That object can provide the methods for equals and for whether or not the weakrefs are still live.
We could also refactor this later when all the other pieces are here.
There was a problem hiding this comment.
I agree with this sentiment, but also that this is sufficiently deep internals that we can change it later without major issues.
There was a problem hiding this comment.
That's right this is not going to affect the API users will interact with, so I'd be happy to do the refactoring in a separate PR, I will open an issue for that.
| dispatcher : callable(handler, event) | ||
| A callable for dispatching the handler, e.g. on a different | ||
| thread or on a GUI event loop. ``event`` is the object | ||
| created by the event factory. |
There was a problem hiding this comment.
That's right this is not going to affect the API users will interact with, so I'd be happy to do the refactoring in a separate PR, I will open an issue for that.
| return False | ||
| return ( | ||
| self.handler() is other.handler() | ||
| and self.target() is other.target() |
There was a problem hiding this comment.
👍 I will add this one line for comparing the dispatch callable (and its test).
|
Thank you both! Merging and then I will go on to fix merge conflict with #1007... |
Targeting #977
This PR
TraitEventNotifierfor wrapping user's handler in the observer system. It used to be calledTraitObserverNotifierin the PoC branch in [POC] Listener rework for EEP-3 #942.IObservableinterface. It used to be calledINotifiableObjectin [POC] Listener rework for EEP-3 #942. I realized the previous naming was a bit misleading, so I changed it. The interface is notused hereimported anywhere yet. But becauseTraitEventNotifieruses it as a dependency, it is added here. In a later PR where we implement the observers, it will need to be turned into an abstract base class so that we can register virtual subclasses to it.push_exception_handlerandpop_exception_handlersimilar to the ones we have intraits.trait_notifiers. The ones intrait_notifiersare not compatible because the handlers expect different call signatures. Furthermore this observer framework should be independent of the on_trait_change framework.Currently all the modules are private (module names have a preceding underscore). We can opt-in bits of these to the public arena later.
Edited: #1023 may provide more context. The
TraitEventNotifieris typically returned by theget_notifiermethod inIObserver.Checklist
Update API reference (All things private here.docs/source/traits_api_reference)Update User manual (All things private here.docs/source/traits_user_manual)Update type annotation hints in. All things private here and nothing is trait-ed.traits-stubs