Extend the `@on` decorator so that it will match superclasses #2691

davep · 2023-05-30T12:14:26Z

This PR seeks to implement the requirement laid out in #2630. In short, the idea is that the the @on decorator should match a given message and any message that inherits from it. To borrow from the issue this stems from, it means that this sort of hierarchy:

class PersonChanged(Message):
    ...

class PersonAdded(PersonChanged):
    ...

class PersonEdited(PersonChanged):
    ...

class PersonRemoved(PersonChanged):
    ...

no longer needs something like this:

@on(PersonAdded)
@on(PersonEdited)
@on(PersonRemoved)
def person_changed(self, event ):
    ...

but can be handles something like this:

@on(PersonChanged)
def person_changed(self, event ):
    ...

One other small change included in this PR is to events.Unmount, which used to inherit from events.Mount; as of this PR it inherits from events.Event; just like all other events. The point of this change being that a developer van safely @on(Mount) without needing to handle fact that events.Unmount would trigger the handler too.

See Textualize#2630. This is, at the moment, an experimental change. This breaks one unit test (surprisingly *only* one!), but otherwise works as intended. Next up will be investigating why that one particular test fails.

@on

Leaning on the newer method of matching messages with @on -- this is pretty much the bit of code that inspired this in the first place.

This doesn't solve the problem I'm trying to solve, but looking at the code the following day this is neater, I think.

I don't think it makes too much of a difference.

…ages

rodrigogiraoserrao · 2023-05-30T14:46:59Z

Hold on, shouldn't the title say "[...] it will match subclasses"?

davep · 2023-05-30T14:59:33Z

Hold on, shouldn't the title say "[...] it will match subclasses"?

Possibly. Depends on whose frame of reference you're you're looking from. In this case from the code that goes seeking handlers (in which case it goes looking for any superclasses that hit). From the dev's point of view: t'other way round.

willmcgugan · 2023-05-30T15:25:48Z

src/textual/message_pump.py

+                for check_message in message.__class__.__mro__:
+                    # If we've hit something that isn't derived from Message
+                    # we can give up.
+                    if not issubclass(check_message, Message):


This may fail in the case of multiple inheritance or mixins. I would hope that nobody uses those in the context of messages, but maybe we should continue rather than break.

Fair point, I'll add some testing for this first. The intent here was to not go further up the MRO than Message, to save looping more than necessary (although of course, at the moment, that's only really going to save one more trip round).

Test added and the "get out as soon as possible" code changed to "wander up the MRO and check anything that looks like a Message): 02b862c

src/textual/message_pump.py

rodrigogiraoserrao · 2023-05-30T15:41:44Z

Hold on, shouldn't the title say "[...] it will match subclasses"?

Possibly. Depends on whose frame of reference you're you're looking from. [...]

Ah, of course.

Textualize#2691 (review) being the driving force here. Also adds a unit test for a non-message mixin into a message class hierarchy.

The test is added as a passing test, but I don't think this is the result we really want.

@on

See Textualize#2691 (comment) The point of this change being that if the developer writes something like: @on(Some.Parent) @on(Some.Child) def some_handler(...): ... then when `Some.Child` is posted, `some_handler` only gets triggered once for that posting. Until this change it would get triggered twice because `Some.Child` matches both `Some.Parent` and `Some.Child`.

Not sure yet what the correct result here should be; based on our conversation so far it seems to be that it's working as intended; but I'm not sure it's working as intended on purpose. I think there's still a breaking test to be found and added here.

This was the breakage I was looking for.

I'm like 90% sure about this. This was a "dump it all and work from scratch" evening experiment, and I think it does what's needed, but I want to read it back over again in the morning with a post-coffee brain. Pushing to the repo so I can review then. Also added lotsa comments so tomorrow me will know what this evening me was thinking.

Be 100% sure that combined classes make a hit

Be sure that a combined set of classes with one not matching doesn't match.

willmcgugan · 2023-06-06T14:45:14Z

Solution is good, but there were improvements on efficiency. Closing in favor of #2746

davep added 9 commits May 29, 2023 15:55

Enable triggering of message handlers up the inheritance tree

283a8b6

See Textualize#2630. This is, at the moment, an experimental change. This breaks one unit test (surprisingly *only* one!), but otherwise works as intended. Next up will be investigating why that one particular test fails.

Add a test for inherited messages triggering parent message handlers

185eff4

Simplify the selection list message tests

785cdd2

Leaning on the newer method of matching messages with @on -- this is pretty much the bit of code that inspired this in the first place.

Tidy up the way the candidate messages are walked

2994da7

This doesn't solve the problem I'm trying to solve, but looking at the code the following day this is neater, I think.

Just go via the class

7797db8

I don't think it makes too much of a difference.

Change events.Unmount so it doesn't inherit from events.Mount

7c35114

Add missing type hint to a test

2f57ad8

Add a test for using @on and on_ while also leaning on inherited mess…

f567b66

…ages

Merge branch 'main' into on-handler-message-hierarchy

5c4d164

davep added enhancement New feature or request Task labels May 30, 2023

davep linked an issue May 30, 2023 that may be closed by this pull request

Consider the idea of having the @on message handler capture and fire on base messages #2630

Closed

Update the ChangeLog

28f3950

davep marked this pull request as ready for review May 30, 2023 13:35

davep requested review from willmcgugan, rodrigogiraoserrao and darrenburns May 30, 2023 13:43

rodrigogiraoserrao approved these changes May 30, 2023

View reviewed changes

Merge branch 'main' into on-handler-message-hierarchy

45b83ec

willmcgugan reviewed May 30, 2023

View reviewed changes

davep added 4 commits May 30, 2023 18:35

Walk the whole MRO handling messages, rather than getting out early

02b862c

Textualize#2691 (review) being the driving force here. Also adds a unit test for a non-message mixin into a message class hierarchy.

Fix up a commenting hangover

fb4dab9

Add a test for parent/child decoration on the same handler

aae9b24

The test is added as a passing test, but I don't think this is the result we really want.

davep requested a review from willmcgugan May 31, 2023 10:27

davep added 2 commits May 31, 2023 15:35

Make the selector test break

f2878ba

This was the breakage I was looking for.

davep marked this pull request as draft May 31, 2023 20:10

davep added 7 commits May 31, 2023 22:00

Merge branch 'main' into on-handler-message-hierarchy

2606aad

Bit of final explanation

7efd963

Extend the @on selector testing

7291a8b

Add another @on selector test

4ec1a84

Be 100% sure that combined classes make a hit

Add yet more @on decorator testing

e1a1e74

Be sure that a combined set of classes with one not matching doesn't match.

Merge branch 'main' into on-handler-message-hierarchy

006f623

davep marked this pull request as ready for review June 1, 2023 09:26

willmcgugan mentioned this pull request Jun 6, 2023

on super class #2746

Merged

davep closed this Jun 6, 2023

davep deleted the on-handler-message-hierarchy branch June 13, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the `@on` decorator so that it will match superclasses #2691

Extend the `@on` decorator so that it will match superclasses #2691

davep commented May 30, 2023

rodrigogiraoserrao commented May 30, 2023

davep commented May 30, 2023

willmcgugan May 30, 2023

davep May 30, 2023

davep May 30, 2023

rodrigogiraoserrao commented May 30, 2023

willmcgugan commented Jun 6, 2023

Extend the @on decorator so that it will match superclasses #2691

Extend the @on decorator so that it will match superclasses #2691

Conversation

davep commented May 30, 2023

rodrigogiraoserrao commented May 30, 2023

davep commented May 30, 2023

willmcgugan May 30, 2023

Choose a reason for hiding this comment

davep May 30, 2023

Choose a reason for hiding this comment

davep May 30, 2023

Choose a reason for hiding this comment

rodrigogiraoserrao commented May 30, 2023

willmcgugan commented Jun 6, 2023

Extend the `@on` decorator so that it will match superclasses #2691

Extend the `@on` decorator so that it will match superclasses #2691