Generic ChangeListener events

[andrej@…]

The proposal is inspired by #8834. Initial proposition suggests a new IResourceChangeListener interface:

class IResourceChangeListener(Interface):
    """Extension point interface for components that require notification
    when resources are created, modified, or deleted.

    'resource' parameters is instance of the a resource e.g. ticket, milestone
    etc.
    'context' is an action context, may contain author, comment etc. Context
    content depends on a resource type.
    """

    def get_subscribed_resources():
        """
        Implementation should return iterator of resource types for which
        the listener has to be notified.

        None or empty list means all types of resources.
        """

    def resource_created(resource, context):
        """
        Called when a resource is created.
        """

    def resource_changed(resource, old_values, context):
        """Called when a resource is modified.

        `old_values` is a dictionary containing the previous values of the
        resource properties that changed. Properties are specific for resource
        type.
        """

    def resource_deleted(resource, context):
        """Called when a resource is deleted."""

    def resource_version_deleted(resource, context):
        """Called when a version of a resource has been deleted."""

Please consider discussion related to this ticket on ​Trac Development group.

There is discussion on Bloodhound project about addition another ChangingListener interface for within transaction events and unifying I*Manipulator interfaces. So, the solution for the generic ChangeListener events implementation can be used as basis for other generic events.

[Andrej Golcov <andrej@…>]

Patch with IResourceChangeListener interface implementation.

[Andrej Golcov <andrej@…>]

Inspired by the discussion on the Trac Development group, I suggest an alternative proposal that employs ComponentManager for event dispatching and tries to solve problem with manual creation and maintenance of several (at least 11) I*ChangeListener interfaces per each entity.

The idea is in introduction of a new method extends_with_prefix that copies methods of base interface to applied interface with a prefix. That approach simplifies creation of multiple interfaces with similar methods with only difference: method prefixes are different e.g. ticket_created, wiki_created etc. For example:

class BasePostChangeListener(Interface):
    def created(entity, context):
    def changed(entity, old_values, context):
    #other ChangeListener methods

class IVersionPostChangeListener(Interface):
    extends_with_prefix(BasePostChangeListener, prefix="version")
#   extends_with_prefix adds prefixed methods to the IVersionPostChangeListener
#   interface. Methods documentation can also be copied
#
#    The following methods are added to the IVersionPostChangeListener interface
#
#    def version_created(entity, context):
#
#    def version_changed(entity, old_values, context):
#
#    other "version_" prefixed ChangeListener methods

This will allow consumers to use standard implements approach for listening to entity changed events of single or multiple entities:

class MultipleEntitiesEventsConsumer(Component):
    """
    Sample of consumer that listens on change events of Version and
    Ticket entities
    """
    implements(IComponentPostChangeListener, TicketPostChangeListener)

    def ticket_created(self, entity, context):
        pass

    def ticket_changed(self, entity, old_values, context):
        pass

#    other "ticket_" prefixed ChangeListener methods
    
    def version_created(self, entity, context):
        pass

    def version_changed(self, entity, old_values, context):
        pass

#    other "version_" prefixed ChangeListener methods

For exotic case when a component has to receive changed events from all entities, another helper method can be introduced that maps prefixed method calls to non-prefixed one. For example:

class AllEntitiesChangedEventsConsumer(Component):
    join_prefixed_method(BasePostChangeListener)

    def created(self, prefix, entity, context):
        pass

    def changed(self, prefix, entity, old_values, context):
        pass

    #other ChangeListener methods with prefix parameter

Comments, suggestions?

[Christian Boos]

About the initial proposal, as already pointed out some renaming needed to make the nature of the arguments more apparent, e.g.

• resource → model / object
• context → changeinfo / event

The second proposal is interesting as well. If you want to convey the idea that the *PostChangeListener is a notification happening after the change, maybe use notification in the name, e.g. *ChangeNotificationListener.

Also, the base could simply be the generic ResourceChangeNotificationListener with methods like resource_created etc. and instead of adding a prefix in specialized interfaces, have a s/resource_/<realm>_ substitution. The consumer for all events can simply implements(ResourceChangeNotificationListener), as in the ComponentManager.notify method we can dispatch the event to both the generic listeners and the specialized ones (+ ignoring non-implemented methods).

[Andrej Golcov <andrej@…>]

An alternative proposal is based on ticket comments and google groups discussion. The proposal introduces IEntityChangeListener interface and extends_with_prefix instruction.

[Andrej Golcov <andrej@…>]

The patch alternative_IEntityChangeListener_extends_with_prefix_r11782.diff​ is one of the possible solutions of the problem described above. The proposal introduced a new interface IEntityChangeListener:

class IEntityChangeListener(Interface):
    def entity_created(entity, changeinfo = None):
    def entity_changed(entity, old_values, changeinfo = None):
    #other methods

And extends_with_prefix instruction applied to an entity interface. I*ChangeListener interface may use extends_with_prefix instruction in order to enable listener to dispatch events to different methods name. For example:

#Specific change listener interface
class IVersionChangeListener(IEntityChangeListener):
    extends_with_prefix("version")

class IComponentChangeListener(IEntityChangeListener):
    extends_with_prefix("component")

#Change listener implementations
class MultipleEntitiesChangeListenerMock(core.Component):
    """Sample listener for change events from multiple events
    """
    implements(IComponentChangeListener, IVersionChangeListener)

    #IComponentChangeListener method
    def component_created(self, entity, changeinfo = None):
        pass

    def component_changed(self, entity, old_values, changeinfo = None):
        pass

     #other IComponentChangeListener methods
    
    #IVersionChangeListener method
    def version_created(self, entity, changeinfo = None):
        pass

    def version_changed(self, entity, old_values, changeinfo = None):
        pass

     #other IVersionChangeListener methods

There are still some open questions to be discussed:

• Entity term versus Resource - IMO there is no need to limit the proposed mechanism to resources only. A plugin may use the same approach for their own entities
• Entity term versus Model or Object - this is just a matter of taste, I think. Suggest yours
• Possibility to subscribe to any entity change notification - we can start with current implementation and add this functionality when the real case appears.
• Should new interfaces be added to entities that already provides similar functionality e.g. Ticket, Milestone, Attachment etc.

Feedback is welcome :)

Cheers, Andrej

[Remy Blank]

Ah, I haven't followed this discussion closely, but this looks like a hell of a lot of magic.

One of the purposes of having interfaces in the first place is to clearly document the methods that need to be supported to implement each interface. Using automagically-determined method names defeats this purpose entirely.

[osimons]

I don't quite see how this is different from what we have today? Won't your implementation still need to know and import IComponentChangeListener and IVersionChangeListener and implement whatever methods those interfaces prescribes?

If I define something like an ISourceCommentChangeListener, then there is really no way for your MultipleEntitiesChangeListenerMock Component to detect this or implement its methods without knowing about it beforehand. And if you still need to know about it beforehand, why insist on sharing method signatures?

[Christian Boos]

Replying to osimons:

I'm afraid you missed Andrej's point with this MultipleEntitiesChangeListenerMock example, which was just a component listening to two specific kinds of resources, not a generic listener. The discussion and his proposal has evolved from providing only a single resource change listener interface to provide specialized but consistent interfaces in addition.

The whole thing started in #8834 (and before that in #6543) was to suggest an uniform way to treat the change notification for resources. There are two sides to this:

1. be able to have a single component listen to changes for any kind of resources (in both patches)
2. have a consistent API across different realms, avoid gratuitous differences (in the second patch only)

[Christian Boos]

Replying to rblank:

One of the purposes of having interfaces in the first place is to clearly document the methods that need to be supported to implement each interface. Using automagically-determined method names defeats this purpose entirely.

Well, if all those methods are going to have the same interface, having to document them over and over again is not optimal either. Interface inheritance alone is not enough as we want to make it possible to implement more than one such interface in a component.

Using a class decorator would make it explicit that we're manipulating a class definition, e.g.

@extends_with_prefix(IEntityChangeListener, "version")
class IVersionChangeListener(Interface):
    """Post-change notification interface for version resources."""

And of course, extra specific methods could be added if needed.

[Olemis Lang <olemis+trac@…>]

JFTR , I have imported both patches into ​olemis/trac-mq patch queue @ Bitbucket (​branch trac_t11148) and this is what I get .

$ hg qseries
t11148/t11148_r11782_IEntityChangeListener_extends_with_prefix.diff

$ hg update 1.0-stable
156 files updated, 0 files merged, 0 files removed, 0 files unresolved

$ hg identify
d6c440b6c31b (1.0-stable) svn-11783

$ hg qpush 
applying t11148/t11148_r11782_IEntityChangeListener_extends_with_prefix.diff
unable to find 'ticket/tests/model.py' for patching
3 out of 3 hunks FAILED -- saving rejects to file ticket/tests/model.py.rej
unable to find 'ticket/model.py' for patching
16 out of 16 hunks FAILED -- saving rejects to file ticket/model.py.rej
unable to find 'core.py' for patching
2 out of 2 hunks FAILED -- saving rejects to file core.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh t11148/t11148_r11782_IEntityChangeListener_extends_with_prefix.diff

$ hg qpop -a
popping t11148/t11148_r11782_IEntityChangeListener_extends_with_prefix.diff
patch queue now empty

$ find . -name "*.rej" | xargs -I file rm -v file 
removed `./core.py.rej'
removed `./ticket/model.py.rej'
removed `./ticket/tests/model.py.rej'

$ hg update trunk
156 files updated, 0 files merged, 0 files removed, 0 files unresolved

$ hg identify
8d0c3223a818 (trunk) svn-11784/tip

$ hg qpush 
applying t11148/t11148_r11782_IEntityChangeListener_extends_with_prefix.diff
unable to find 'ticket/tests/model.py' for patching
3 out of 3 hunks FAILED -- saving rejects to file ticket/tests/model.py.rej
unable to find 'ticket/model.py' for patching
16 out of 16 hunks FAILED -- saving rejects to file ticket/model.py.rej
unable to find 'core.py' for patching
2 out of 2 hunks FAILED -- saving rejects to file core.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh t11148/t11148_r11782_IEntityChangeListener_extends_with_prefix.diff

I'll refresh it to make it work against target versions . @osimons you already have write access to the patch queue so as to hack a little if you want (, have time , …) . If anybody else wants to join patch development please either fork the patch queue or send me your user name in private so that I can grant you with write access . Sometimes one line of code is worth 1000 words ;)

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

JFTR , I have imported both patches into ​olemis/trac-mq patch queue @ Bitbucket

[…]

Done . I have added a few more ​changes based on previous comments in this ticket . Resulting patch is ​here

[Olemis Lang <olemis+trac@…>]

Replying to cboos:

Replying to osimons:

I'm afraid you missed Andrej's point with this MultipleEntitiesChangeListenerMock example, which was just a component listening to two specific kinds of resources, not a generic listener. The discussion and his proposal has evolved from providing only a single resource change listener interface to provide specialized but consistent interfaces in addition.

The whole thing started in #8834 (and before that in #6543) was to suggest an uniform way to treat the change notification for resources. There are two sides to this:

1. be able to have a single component listen to changes for any kind of resources (in both patches)
2. have a consistent API across different realms, avoid gratuitous differences (in the second patch only)

Patch proposed in comment:5 leads leads to confusion . Consider the following scenario . Initially listener component only listens for notifications in realm R thus entity_* methods are implemented . As software evolves further realms are of interest . In order to subscribe to those channels they'll be renamed to become R_* . In the patch mentioned in comment:11 consistent names are used instead .

Of course , that may be rejected , but honestly I see no point in having entity_* as well as <realm>_* methods . IMO if target realms are known beforehand then component should implement specialized interfaces defining <realm>_* methods . If target realms may vary considerably at run time then implement generic IEntityChangelistener interface defining entity_* methods .

This implies implementing inline TBD hint added by Andrej , which I'll do tomorrow .

Feedback is welcome .

[Olemis Lang <olemis+trac@…>]

Replying to cboos:

Replying to rblank:

One of the purposes of having interfaces in the first place is to clearly document the methods that need to be supported to implement each interface. Using automagically-determined method names defeats this purpose entirely.

Well, if all those methods are going to have the same interface, having to document them over and over again is not optimal either. Interface inheritance alone is not enough as we want to make it possible to implement more than one such interface in a component.

Using a class decorator would make it explicit that we're manipulating a class definition, e.g.

@extends_with_prefix(IEntityChangeListener, "version")
class IVersionChangeListener(Interface):
    """Post-change notification interface for version resources."""

And of course, extra specific methods could be added if needed.

This is implemented in patch proposed in comment:11 . It's possible to override method signature + docs by declaring it in class suite . That way it will not be overwritten . Nevertheless notice that as a result there is an impact on code readability . It's not obvious what is this interface unless readers know what's all this magic about .

[Andrej Golcov <andrej@…>]

Personally, I'm satisfied with patched proposed by olemis in comment:11. It removes the magic from the ListenerNotifier class. There is a magic decorator extends_with_prefix that developers have to be aware of but it saves a lot of copy-paste code for I*ChangeListener interfaces definition. Additionally, extends_with_prefix defines the relation between IEnitityChanageListener and a I*ChanageListener interface and a developer have to know only one interface instead of learning plenty of interfaces.

Is there any chance of something like this to be submitted into Trac?

@olemis: method _get_prefixed_method_name can be completely removed now.

[ethan.jucovy@…]

FWIW I agree with rblank's comment:6. @extends_with_prefix seems like a convenience for interface authors to avoid copy-and-pasting code at the expense of interface implementers and any other code reader, who now have to understand what extends_with_prefix does and have to open an additional file to look up the base IEntityChangeListener definition. This doesn't seem like a good trade-off to make considering the relative frequency of defining interfaces vs using them. If the copy-paste is so burdensome, wouldn't a simple code-generation script that spits out a full interface definition (trac-entity-iface-gen mynewresource > file.py) be almost as good, and without the cost to code consumers?

I don't think "reparented" should be included in this generic interface. As olemis mentioned on the associated trac-dev thread, it currently has no definition for any trac objects besides attachments. I can't even imagine a specific definition for reparenting tickets, components, milestones, etc — the only probably-unambiguous "reparent" definitions that could be built in to Trac core are attachments, wiki nodes, and file nodes. Defining "reparent" as a first-class concept in Trac core without providing a specific definition for it is a recipe for incompatible plugins, if one plugin decides "ticket_reparented" means "moved to a different milestone" and another plugin decides that it means "moved to a different product environment." I'm also concerned that this would encourage plugin authors to invent contrived definitions for reparenting their own resource classes.

Lastly I'm a bit concerned that these interfaces only work on single resources. If multiple resources are modified in bulk, implementations of these change listeners will probably be very inefficient. Admittedly this is a problem that already exists with Trac's current ITicketChangeListener, IMilestoneChangeListener, etc so it's sort of out of scope for this ticket, but I thought it's worth mentioning.

[Olemis Lang <olemis+trac@…>]

Replying to Andrej Golcov <andrej@…>:

[…]

Is there any chance of something like this to be submitted into Trac?

@olemis: method _get_prefixed_method_name can be completely removed now.

Done. Also _get_listeners_for_interface was completely removed by reusing ExtensionPoint . New patch is available ​here . If interested see ​branch/trac_t11148 changelog .

I look forward to further feedback regarding generic event listeners . Soon I'll submit something based on ​this inline TODO

+        #TBD: we can also call listeners implemented IEntityChangeListener here
+        #if community will agree that generic event listeners support is needed

[Olemis Lang <olemis+trac@…>]

Replying to ethan.jucovy@…:

FWIW I agree with rblank's comment:6. @extends_with_prefix seems like a convenience for interface authors to avoid copy-and-pasting code at the expense of interface implementers and any other code reader, who now have to understand what extends_with_prefix does and have to open an additional file to look up the base IEntityChangeListener definition. This doesn't seem like a good trade-off to make considering the relative frequency of defining interfaces vs using them. If the copy-paste is so burdensome, wouldn't a simple code-generation script that spits out a full interface definition (trac-entity-iface-gen mynewresource > file.py) be almost as good, and without the cost to code consumers?

Yeah . I'm not quite comfortable with that situation either like I just said in comment:13 .

I don't think "reparented" should be included in this generic interface.

FWIW , I do .

As olemis mentioned on the associated trac-dev thread, it currently has no definition for any trac objects besides attachments. I can't even imagine a specific definition for reparenting tickets, components, milestones, etc —

We already have a use case for that in Apache™ Bloodhound .

the only probably-unambiguous "reparent" definitions that could be built in to Trac core are attachments, wiki nodes, and file nodes. Defining "reparent" as a first-class concept in Trac core without providing a specific definition for it is a recipe for incompatible plugins,

IMO reparent events are aimed at reflecting changes performed upon parent/child resource hierarchy . With the introduction of [bh:bep:0003#resource-neighborhood resource neighborhoods] , moving resources (e.g. tickets, wiki , …) from product P1 onto product P2 should trigger reparent event in both contexts (leave, arrive ;) .

if one plugin decides "ticket_reparented" means "moved to a different milestone" and another plugin decides that it means "moved to a different product environment." I'm also concerned that this would encourage plugin authors to invent contrived definitions for reparenting their own resource classes.

Not ambiguous because this is not a meaningful resource ID .

Resource('milestone', 'milestone1').child('ticket', 1)

… but this one is valid

Neighborhood('product', 'P1').child('ticket', 1)

… at least for Apache™ Bloodhound , which is indeed a set of plugins running on top of Trac . ;)

Lastly I'm a bit concerned that these interfaces only work on single resources. If multiple resources are modified in bulk, implementations of these change listeners will probably be very inefficient. Admittedly this is a problem that already exists with Trac's current ITicketChangeListener, IMilestoneChangeListener, etc so it's sort of out of scope for this ticket, but I thought it's worth mentioning.

Yes it's an interesting subject and IMHO it is not off-topic . The fact is : what shall we do to improve listeners performance on batch modifications ? Support (resource | model | entiy | …) lists in API methods ?

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

Replying to Andrej Golcov <andrej@…>:

[…]

Is there any chance of something like this to be submitted into Trac?

@olemis: method _get_prefixed_method_name can be completely removed now.

Done.

[…]

@andrej : you may do anything you want with the patch . No need to wait for me to get things done . just fork the patch queue or send me your Bitbucket user name in private so as to grant you with write permissions ;)

don't be shy ! :)

I look forward to further feedback regarding generic event listeners . Soon I'll submit something based on ​this inline TODO

+        #TBD: we can also call listeners implemented IEntityChangeListener here
+        #if community will agree that generic event listeners support is needed

For instance , in order to get this done all other existing listener interfaces and client code should be updated as well .

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

Replying to Olemis Lang <olemis+trac@…>:

[…]

I look forward to further feedback regarding generic event listeners . Soon I'll submit something

Done . See new patch ​here as well as ​branch/trac_t11148 changelog for further details.

@andrej : please take a look .

@rblank @osimons @ethan.jucovy : I've added GenericEntitiesChangeListenerTestCase.test_custom_event_args test case illustrating how everything will still work even if using explicit interface definition as usual . No magic decorators , full code readability , etc . I'm hoping this will satisfy your expectations but , in any case, it'd be nice if you could assess the current state of the patch . Feedback is welcome .

Open questions :

• Should interface + method_name be supplied in to generic listeners in **kwargs ?
• … more … see inline TODO

For instance , in order to get this done all other existing listener interfaces and client code should be updated as well .

… I'll move forward with this i.e. reuse listener loop to reach generic listeners for all other I*ChangeListener interfaces e.g. tickets, wiki, attachments, .

[Andrej Golcov <andrej@…>]

Replying to Olemis Lang <olemis+trac@…>:

• Should interface + method_name be supplied in to generic listeners in **kwargs ?

I would really prefer to have IEntityChangeListener interface strictly defined without **kwargs - let's define contracts in code not in documentation. NotificationChangeInfo class should be used for extensibility purposes.

[Olemis Lang <olemis+trac@…>]

Replying to Andrej Golcov <andrej@…>:

Replying to Olemis Lang <olemis+trac@…>:

• Should interface + method_name be supplied in to generic listeners in **kwargs ?

I would really prefer to have IEntityChangeListener interface strictly defined without **kwargs

Just to double check : as I understand you mean to assign excess positional arguments defined in resource-specific interface to keys (dict) or attributes (object) and pass such data in changeinfo parammeter , is that it ?

• let's define contracts in code not in documentation.

It is defined in code, to be more precise in resource-specific interface e.g. ISomethingListener. Since the very same moment IEntityChangeListener is designed to be a match-them-all interface it has to lose

NotificationChangeInfo class should be used for extensibility purposes.

I'll use such instances instead of **kwargs if there's no objections . What about collisions ? I see some arguments (comment and author) already added in there . Or maybe excess event args should be added in another e.g. eventArgs (⇐ or alike) attribute ?

I look forward to your replies .

[Andrej Golcov <andrej@…>]

Replying to ethan.jucovy@…:

FWIW I agree with rblank's comment:6. @extends_with_prefix seems like a convenience for interface authors to avoid copy-and-pasting code at the expense of interface implementers and any other code reader, who now have to understand what extends_with_prefix does and have to open an additional file to look up the base IEntityChangeListener definition. This doesn't seem like a good trade-off to make considering the relative frequency of defining interfaces vs using them. If the copy-paste is so burdensome, wouldn't a simple code-generation script that spits out a full interface definition (trac-entity-iface-gen mynewresource > file.py) be almost as good, and without the cost to code consumers?

Currently, there is 11 entities planned to use I*ChangeListener interface. AFAIK, there is a discussion about addition some kind of "before change" interface in order to move I*Manipulator logic into api level. I believe, there is also need of "I*Changing" interface in order to provide plugins with possibility to write into DB during the same transaction as entity. If we assume that each interface has at least 4 methods for CRUD, there is possibility to have 33*4=132 methods to maintain and to document where each method is similar to 11 methods in other interfaces. I think we break Do Not Repeat Yourself principle here.

Using code generation is aslo not a solution because of complications when an interface needs additional methods.

But the major problem IMO is that there is no "declaration" that IEntity1ChangeListenere is the same as IEntity2ChangeListenere except methods prefix. In this case, a developer has to learn each interface carefully because there is no guaranty that methods and parameters are the same.

[Remy Blank]

Replying to Andrej Golcov <andrej@…>:

Currently, there is 11 entities planned to use I*ChangeListener interface. AFAIK, there is a discussion about addition some kind of "before change" interface in order to move I*Manipulator logic into api level. I believe, there is also need of "I*Changing" interface in order to provide plugins with possibility to write into DB during the same transaction as entity. If we assume that each interface has at least 4 methods for CRUD, there is possibility to have 33*4=132 methods to maintain and to document where each method is similar to 11 methods in other interfaces. I think we break Do Not Repeat Yourself principle here.

Using code generation is aslo not a solution because of complications when an interface needs additional methods.

You do know that the methods defined in interfaces are purely for documentation, don't you? You could remove all methods from all interfaces in Trac core and all plugins, and everything would continue working as before.

Yes, it's a lot of interfaces. But the thing is, a user will never have to know about all of them at once, she only needs to know about the one she needs to reach her goal. So let's compare what a user needs to do to find out how to be notified for a ticket change:

• With the @extends_with_prefix variant:
  • Look up the ITicketChangeListener interface.
  • Spot the @extends_with_prefix, think "What is this doing?"
  • Look up extends_with_prefix().
  • Read the implementation and try to understand what it does.
  • "Reconstruct" the method signatures and paste them into own code.
• With the standard interface variant:
  • Look up the ITicketChangeListener interface.
  • Copy the method signatures and paste them into own code.

I have quite a strong preference for the latter.

[Olemis Lang <olemis+trac@…>]

Replying to rblank:

Replying to Andrej Golcov <andrej@…>:

Currently, there is 11 entities planned to use I*ChangeListener interface. AFAIK, there is a discussion about addition some kind of "before change" interface in order to move I*Manipulator logic into api level. I believe, there is also need of "I*Changing" interface in order to provide plugins with possibility to write into DB during the same transaction as entity. If we assume that each interface has at least 4 methods for CRUD, there is possibility to have 33*4=132 methods to maintain and to document where each method is similar to 11 methods in other interfaces. I think we break Do Not Repeat Yourself principle here.

Using code generation is aslo not a solution because of complications when an interface needs additional methods.

I do not like that approach either but, if preferred , some tool outside core e.g. ​trachacks:TracPluginTemplateScript might help . Just contact me after patch acceptance if interested in such enhancement. I'm the maintainer of that plugin . […]

Yes, it's a lot of interfaces. But the thing is, a user will never have to know about all of them at once, she only needs to know about the one she needs to reach her goal. So let's compare what a user needs to do to find out how to be notified for a ticket change:

• With the @extends_with_prefix variant:
  • Look up the ITicketChangeListener interface.
  • Spot the @extends_with_prefix, think "What is this doing?"
  • Look up extends_with_prefix().
  • Read the implementation and try to understand what it does.
  • "Reconstruct" the method signatures and paste them into own code.

I follow your reasoning and I agree that it's not a trivial task to understand what is going on with extends_with_prefix . The arguments mentioned by @andrej are also valid IMO . If this function is not meant to live in core , it might be added in utils or implemented by external packages if it proves to be useful .

• With the standard interface variant:
  • Look up the ITicketChangeListener interface.
  • Copy the method signatures and paste them into own code.

I have quite a strong preference for the latter.

@rblank : As you might have noticed in test cases added in latest version of the patch the listeners notification machinery does not require extends_with_prefix function at all , and thereby afaics it's not a major reason to reject the current solution . cmiiw

I'd like to gather more feedback about **kwargs vs NotificationChangeInfo to deliver excess event arguments to generic entity listeners .

Considering recent comments , this is the detailed roadmap I'll consider while working towards next proposal (order matters) .

• Remove extends_with_prefix
• Add event_args and action members in instances of NotificationChangeInfo class to deliver excess event data to generic entity listeners .
• Integrate ITicketChangeListener, IWikiChangeListener, … into the notifications loop to reach generic entity listeners
• Add support for custom event methods defined by resource-specific interfaces

Anything missing ? Objections ? Comments ?

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

[…]

Considering recent comments , this is the detailed roadmap I'll consider while working towards next proposal (order matters) .

• Remove extends_with_prefix
• Add event_args and action members in instances of NotificationChangeInfo class to deliver excess event data to generic entity listeners .

[…]

• Add support for custom event methods defined by resource-specific interfaces

JFTR , I have refreshed the patch to implement and test these pending aspects. New version of the patch available ​here . See ​branch/trac_t11148 changelog for further details.

• Integrate ITicketChangeListener, IWikiChangeListener, … into the notifications loop to reach generic entity listeners

I'm working now on this . Should be ready by tomorrow .

[Christian Boos]

Replying to rblank:

Replying to Andrej Golcov <andrej@…>:

[…] each method is similar to 11 methods in other interfaces. I think we break Do Not Repeat Yourself principle here.

Using code generation is aslo not a solution because of complications when an interface needs additional methods.

You do know that the methods defined in interfaces are purely for documentation, don't you? You could remove all methods from all interfaces in Trac core and all plugins, and everything would continue working as before.

Indeed, we could just document it by reference, i.e.

class IVersionChangeListener(Interface):
    """Post-change notification interface for version resources.

    This interface consists of the `IEntityChangeListener` methods
    with modified names: the ``resource_`` prefix becomes ``version_``.
    """

with or without the @extends_with_prefix(IEntityChangeListener, "version"), depending if the generated documentation looks better or not.

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>:

I have refreshed previous patch (available ​here) and added a ​another one for IAttachmentChangeListener compatibility (with test cases).

See ​branch/trac_t11148 changelog for further details.

[Olemis Lang <olemis+trac@…>]

Replying to Olemis Lang <olemis+trac@…>: Latest version of the patches may be found ​here. See ​branch/trac_t11148 changelog for further details.

Patch queue state shown below :

$ hg qseries
t11148/t11148_r11782_IEntityChangeListener_v2.diff
t11148/t11148_r11784_IEntityListener_compat_attachment.diff
t11148/t11148_r11784_IEntityListener_compat_ticket.diff
t11148/t11148_r11784_IEntityListener_compat_versioncontrol.diff
t11148/t11148_r11784_IEntityListener_compat_wiki.diff

$ hg log -r qparent --template="[{svnrev}] - {desc}\n"
[11784] - 1.1.2dev: merge [11783] from 1.0-stable (fix of #10903)

$ python trac/ticket/tests/model.py
..................................................................................
----------------------------------------------------------------------
Ran 82 tests in 1.347s

OK

$ python trac/wiki/tests/model.py
.........
----------------------------------------------------------------------
Ran 9 tests in 0.145s

OK

$ python trac/tests/attachment.py
.............
----------------------------------------------------------------------
Ran 13 tests in 0.186s

OK

As you can see everything but version control listeners has been tested . I did not see any such test . I look forward to your comments after review , hoping this solution (maybe with minor modifications) will be accepted sooner rather than later . Please mention what needs to be added for this to happen .

[Olemis Lang <olemis+trac@…>]

BTW , all these interfaces are integrated now into the generic notifications loop .

$ grep -r "Listener(Interface)" trac
trac/attachment.py:class IAttachmentChangeListener(Interface):
trac/core.py:class IEntityChangeListener(Interface):
trac/ticket/api.py:class ITicketChangeListener(Interface):
trac/ticket/api.py:class IMilestoneChangeListener(Interface):
trac/ticket/api.py:class ITypeChangeListener(Interface):
trac/ticket/api.py:class IResolutionChangeListener(Interface):
trac/ticket/api.py:class IPriorityChangeListener(Interface):
trac/ticket/api.py:class ISeverityChangeListener(Interface):
trac/ticket/api.py:class IComponentChangeListener(Interface):
trac/ticket/api.py:class IVersionChangeListener(Interface):
trac/versioncontrol/api.py:class IRepositoryChangeListener(Interface):
trac/wiki/api.py:class IWikiChangeListener(Interface):

[Christian Boos]

(btw, the topic is not forgotten, last month I did work on that a lot, but I just didn't have time so far to push my branch and restart the discussion…)

[Christian Boos]

Replying to Olemis Lang <olemis+trac@…>:

BTW , all these interfaces are integrated now into the generic notifications loop .

...
trac/wiki/api.py:class IWikiChangeListener(Interface):

I've tested the branch a few days weeks ago, and I noticed that the way you implemented it was not backward compatible (i.e. plugins implementing the old interface won't work anymore, like the SpamFilter).

So I tried to take a fresh approach on the issue, with backward compatibility in mind for the old interfaces (using adapters).

• [539dcb7f/cboos.git] ApiDocs: document trac.resource and clean-up its docstrings.
• [b848dfa7/cboos.git] ticket:11148: add a generic resource change listener (IResourcePostChangeListener)

I was not satisfied by the above nevertheless, as I still want to address the concerns from comment:15 about batch modifications, the idea being to treat a single change simply as a special case of batch modifications (list of len 1), as Olemis suggested in comment:17.

[Andrej Golcov <andrej@…>]

Looks good. Just one note regarding backward compatibility when calling legacy I*ChangeListener in Legacy*ChangeListenerAdapter. As Olemis pointed out on Bloodhound mailing list, that can be a problem if Trac config enables components exactly but not with "*". In this case, user will miss change events after an upgrade. That's why I leave calling I*ChangeListener inside the model.py files.

[Andrej Golcov <andrej@…>]

Replying to Andrej Golcov <andrej@…>:

That's why I leave calling I*ChangeListener inside the model.py files.

Ah sorry, pressed Submit too fast. I mean: That's why I proposed a calling of I*ChangeListener interfaces inside model.py files in my initial proposal.

[Olemis Lang <olemis+trac@…>]

Replying to Andrej Golcov <andrej@…>:

Looks good.

:)

Just one note regarding backward compatibility when calling legacy I*ChangeListener in Legacy*ChangeListenerAdapter. As Olemis pointed out on Bloodhound mailing list, that can be a problem if Trac config enables components exactly but not with "*".

+1

In this case, user will miss change events after an upgrade. That's why I leave calling I*ChangeListener inside the model.py files.

Such undesired behavior will happen every time Legacy*ListenerAdapter components will not be enabled ( … unless required = True is specified ? )

[Jun Omae]

I think we should handle ticket's comment as resource or raise "comment modified" event of "ticket". I noticed that ITicketChangeListener interface doesn't have method for ticket's comment editing by #11377.

[Olemis Lang <olemis+trac@…>]

Replying to jomae:

I think we should handle ticket's comment as resource or raise "comment modified" event of "ticket".

Comments as resources would be nice , it's a idea I've discussed before with @lkraav so he might have also mentioned this somewhere else before as well .

I noticed that ITicketChangeListener interface doesn't have method for ticket's comment editing by #11377.

I was thinking the same when I read that ticket . Should I propose another patch + test cases for this feature (i.e. ticket comments) ?

[dmsahabandu@…]

Improvements for the IResourceChangeListener support. * Renaming of ticket related resources are reflected in search results * Python=2.6 compatibility issue fixes in Trac test suite

[dmsahabandu@…]

For Apache Bloodhound we have done the following improvements to the IResourceChangeListener support. Please assess the possibility of back porting them to Trac-1.0.14.

• Renaming of ticket related resources are reflected in search results
• Python=2.6 compatibility issue fixes in Trac test suite

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

Milestone renamed