Don't distinguish notification handlers on "dispatch" value

[mdickinson]

The use of dispatch when distinguishing (and in particular when removing) notification handlers has proved surprising; I think we should revisit this for 6.2, and possibly consider removing it.

Related: PR #1195.

[mdickinson]

I strongly believe that the current behaviour is not the right one, and we should change it.

• it's surprising for users: the mental model for on_trait_change, or observe, or the observation pattern in general, is that we're connecting thing A (in this case, the notification handler) to thing B (in this case, the observed trait); disconnecting should be as simple as thingB.disconnect(thingA); the details of how the connection was made (the dispatch, for example) shouldn't need to be resupplied when disconnecting
• It's inconsistent with the on_trait_change behaviour, which adds another small roadblock for those moving from on_trait_change to observe, and for code that isn't thoroughly tested could introduce late-discovered application bugs

[mdickinson]

An example in the wild: if you look at QObject::connect, the connect call takes a method argument that specifies how the connection should be made; this is similar in meaning to our dispatch. That method argument does not (and should not) need to be resupplied at disconnect time, and it would be a usability hit if you did need to know at disconnection time how the original connection was made.

[mdickinson]

the connect call takes a method argument

Sorry; wrong argument: I meant the type argument, not the method argument.

[kitchoi]

The observe(..., remove=True) action mirrors the observe(..., remove=False) action.
So I think it is relevant to discuss what the expected behaviour for the following:

Scenario 1:

# some part of the code executes this first:
instance.observe(handler, "name", dispatch="same")

# some other part of the code executes this later:
instance.observe(handler, "name", dispatch="ui")

Should the second call fail? If not, is the handler dispatched via "same", or via "ui"?
The choice of "same" being followed by "ui" is arbitrary here. The same question applies if the call with dispatch="ui" is executed first.

If the second call should fail, that is basically the framework saying "the use case supported is that each handler should only have one dispatch method at any given time. If you need to call the handler with more than one dispatch, wrap the handler into a new callable so they do not compare equally".

If the second call does not fail, that would be consistent with on_trait_change, but is a behaviour that surprises me when I found out.

Scenario 2:

instance.observe(handler, "name", dispatch="same")

# somewhere else in the code base, the dispatch is explicitly specified.
instance.observe(handler, "name", dispatch="ui", remove=True)

Should the second call fail to help catch programming error?

Scenario 3 (depends on 1 and 2):
If first two calls do not fail, what is the expected behaviour for the last call?

# somewhere in the code that gets run first
instance.observe(handler, "name", dispatch="ui")
# somewhere in the code that gets run second
instance.observe(handler, "name", dispatch="same")
# somewhere in the code that gets run last
instance.observe(handler, "name", removed=True)

[kitchoi]

Scenario 1
...
If the second call does not fail, that would be consistent with on_trait_change, but is a behaviour that surprises me when I found out.

I should clarify that the surprise I had with on_trait_change was the fact the second call is in fact an no-op. If it does not fail, I'd expect a second notifier to be added for the second dispatch method. Or if a second notifier cannot be added, I'd expect an error.

[kitchoi]

For the record, I'd prefer keeping dispatch for symmetry. It is easier to understand both for users and for developers maintaining the behaviour. It is easier to say, "in order to undo the action of observe, copy and paste the invocation and add remove=True to it". If we had an unobserve, this is pleasing to read and straightforward to understand:

observe(handler, "name", dispatch="ui")
unobserve(handler, "name", dispatch="ui")

This issue was motivated by a PR in a project trying to convert all on_trait_change to observe where the action to add the change handler has dispatch set to "ui". The surprise was partly because on_trait_change has a different behaviour, so the old invocation to remove the change handler did not have to set dispatch to "ui", so blindly replacing on_trait_change(..., remove=True) to observe(..., remove=True) fails there. But the implementation that allows this permissive behaviour in on_trait_change also causes #237 and that the second call in the following snippet to be silently ignored, which is surprising too:

instance.observe(handler, "name", dispatch="same")
instance.observe(handler, "name", dispatch="ui")

On the argument taking inspiration from Qt API, recently I added type=... to a Qt connect call, I immediately went to the disconnect call to add the same type=... to match, and was surprised that Qt does not take the argument at all.

[kitchoi]

When I tried to reimplement Envisage's ExtensionRegistry using observe, I find the current behaviour of differentiating dispatcher (not dispatch) allow me to implement it in a simpler way. See enthought/envisage#347

These are the code blocks that are relevant:
https://github.com/enthought/envisage/blob/c8e9f0e924b5f04b97806cfbb2e232f1ca3c4c39/envisage/extension_registry.py#L244-L272
https://github.com/enthought/envisage/blob/c8e9f0e924b5f04b97806cfbb2e232f1ca3c4c39/envisage/extension_registry.py#L298-L318

Envisage wants to be able to register a callable called listener for a specific extension point ID. By defining a new dispatcher for each specific extension point ID and by distinguishing these dispatchers, I can hook and unhook the listener for specific extension point ID. If dispatcher is not distinguished as this issue proposes, then I will have to rewrap listener with the extension point ID, and reimplement all the weakref logic for instance methods and equality comparison logic. All of that logic is already implemented in traits observe. Not only that would be reinventing the wheel, the weakref and equality logic are easy to forget and hard to get right.

(Note: The issue here is about the dispatch parameter on HasTraits.observe. Byt the underlying logic is about comparing two dispatcher callables, as the dispatch string parameter on HasTraits.observe is merely a high-level interface that maps to a predefined callable.)

[aaronayres35]

What is the current status/course of action for this issue? I'm unsure if this has been discussed/decided offline or if it is still up for debate

[mdickinson]

Let's close this issue (as rejected). We'll stick with what we have right now. The plan is the migrate quite a lot of code to the observe framework in the next few weeks; we can always re-open this issue if we decide that migration is being adversely affected.