Skip to content

Conversation

@papegaaij
Copy link
Contributor

ListenerCollection has a note that states that the order is not guaranteed. This seems strange as the collection is simply a list and it even has a method to notify the listeners in reverse order. I suggest to change the comment and actually guarantee the order of notification to be in match with the order the listeners were added.

@martin-g
Copy link
Member

I remember @ivaynberg having a conversation with someone on the mailing lists about depending on order. His wise advice was to never rely on someone/something else to provide it for you. It is better to check the environment/context/conditions and act as necessary.
I don't remember the details now. I just try to follow this advice in my work.

@papegaaij
Copy link
Contributor Author

I wouldn't recommend to depend on listeners being added in a specific order, especially when they are added automatically, for example via a service loader. However, if you add them yourself, you are in control and we can guarantee the order of execution. This change only indicates that the listeners are notified in the order they were added, which does make sense imho, also because the class already has a method to invoke them in reverse (the reverse of an unordered set doesn't exist).

This came up when a developer at our company wanted to change a specific listener rather than added his own, because this comment stated he could not rely on their order. He specifically wanted his code to be executed before another, both listeners are added manually.

@papegaaij
Copy link
Contributor Author

@martin-g do you have objections against merging this change?

@martin-g
Copy link
Member

martin-g commented Mar 23, 2022 via email

@asfgit asfgit merged commit dcf11e6 into apache:wicket-9.x Mar 23, 2022
@papegaaij papegaaij deleted the notify-listener-doc branch March 23, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants