Inherit Event Handlers #34

Merged
kske merged 2 commits from f/handler-inheritance into develop 7 months ago
kske commented 7 months ago
Owner

When registering an event listener, Event Bus recursively walks the entire inheritance tree and looks for event handlers.

Closes #16

When registering an event listener, Event Bus recursively walks the entire inheritance tree and looks for event handlers. Closes #16
kske self-assigned this 7 months ago
kske added 1 commit 7 months ago
7fb633d69f
Inherit event handlers
kske requested review from delvh 7 months ago
kske requested review from DieGurke 7 months ago
delvh reviewed 7 months ago
## Inheritance
When a superclass or an interface of an event listener defines event handlers, they will be detected and registered by Event Bus, even if they are `private`.
If an event handler is overridden by the listener, the `@Event` annotation of the overridden method is automatically considered present on the overriding method.
delvh commented 7 months ago
Owner

Perhaps a new annotation @ExcludeListener should be added that instructs EventBus to ignore this method if present. This would allow to override behavior of superclasses that is in some rare cases counter-productive.

(But if at all, that is beyond the scope of this PR)

Perhaps a new annotation `@ExcludeListener` should be added that instructs EventBus to ignore this method if present. This would allow to override behavior of superclasses that is in some rare cases counter-productive. (But if at all, that is beyond the scope of this PR)
kske commented 7 months ago
Poster
Owner

That would be rather difficult to implement given the edge cases. If such a need arises, I will try.

That would be rather difficult to implement given the edge cases. If such a need arises, I will try.
delvh marked this conversation as resolved
README.md Outdated
When a superclass or an interface of an event listener defines event handlers, they will be detected and registered by Event Bus, even if they are `private`.
If an event handler is overridden by the listener, the `@Event` annotation of the overridden method is automatically considered present on the overriding method.
If the overridden method contains an implementation, it is ignored as expected.
delvh commented 7 months ago
Owner

If the overridden method already contains an implementation in the superclass, the superclass implementation is ignored as expected.

If the overridden method already contains an implementation in the superclass, the superclass implementation is ignored as expected.
delvh commented 7 months ago
Owner

Or do I understand that wrong?

Or do I understand that wrong?
kske commented 7 months ago
Poster
Owner

You understood that correctly. There is a difference between the overridden method and the overriding method. One is in the superclass, the other in the subclass.

You understood that correctly. There is a difference between the overridden method and the overriding method. One is in the superclass, the other in the subclass.
delvh commented 7 months ago
Owner

The topmost comment was intended as a suggestion for the README.

The topmost comment was intended as a suggestion for the README.
kske marked this conversation as resolved
Set<Method> methods = getMethodsAnnotatedWith(listenerClass, Event.class);
// Recursively add superclass handlers
if (listenerClass.getSuperclass() != null)
delvh commented 7 months ago
Owner
		Class<?> superClass = listenerClass.getSuperclass();
		if (superClass != null && superClass != Object.class)
        			methods.addAll(getHandlerMethods(superClass));
```java Class<?> superClass = listenerClass.getSuperclass(); if (superClass != null && superClass != Object.class) methods.addAll(getHandlerMethods(superClass)); ```
kske marked this conversation as resolved
Class<? extends Annotation> annotationClass) {
return Arrays.stream(enclosingClass.getDeclaredMethods())
.filter(m -> m.isAnnotationPresent(annotationClass))
.collect(Collectors.toSet());
delvh commented 7 months ago
Owner
var set = new HashSet<Method>();
for(Method method : enclosingClass.getDeclaredMethods())
	if(method.isAnnotationPresent(annotationClass))
    	set.add(method);
        
return set;
```java var set = new HashSet<Method>(); for(Method method : enclosingClass.getDeclaredMethods()) if(method.isAnnotationPresent(annotationClass)) set.add(method); return set; ```
kske marked this conversation as resolved
event.increment();
}
@Event
delvh commented 7 months ago
Owner

If you now even use priorities you can test whether the priority is always correct.
Also, I think it would be good to explicitly override one of the superclass methods not to do anything.

If you now even use priorities you can test whether the priority is always correct. Also, I think it would be good to explicitly override one of the superclass methods **not** to do anything.
kske marked this conversation as resolved
interface SimpleEventListenerInterface {
@Event
void onSimpleEventInterfaceHandler(SimpleEvent event);
delvh commented 7 months ago
Owner

Will an interface-private method annotated with @Event be registered?
Or should we explicitly disallow that?

Will an interface-private method annotated with `@Event` be registered? Or should we explicitly disallow that?
kske commented 7 months ago
Poster
Owner

There is no reason why it shouldn't be.

There is no reason why it shouldn't be.
delvh commented 7 months ago
Owner

Yes, and that's exactly what I find so scary.
In a class, private methods are expected.
In an interface however, no one suspects that there is a private method that is responsible for changing the state.

Yes, and that's exactly what I find so scary. In a class, private methods are expected. In an interface however, no one suspects that there is a private method that is responsible for changing the state.
kske commented 7 months ago
Poster
Owner

Well, that would be a very rare case, as the event handler would only work when some class implements the interface and registers itself as an event listener. If such a situation actually arises, it should be made clear how that interface is supposed to be used.

Well, that would be a very rare case, as the event handler would only work when some class implements the interface and registers itself as an event listener. If such a situation actually arises, it should be made clear how that interface is supposed to be used.
delvh marked this conversation as resolved
kske added 1 commit 7 months ago
722bf2b999
Test priorities for inheritance
delvh approved these changes 7 months ago
Owner

What I noticed, however, is that maybe there should also be a method to not use the whole hierarchy and instead only the current class, i.e. an extra registerOnly method.

What I noticed, however, is that maybe there should also be a method to not use the whole hierarchy and instead only the current class, i.e. an extra `registerOnly` method.
kske removed review request for DieGurke 7 months ago
kske merged commit 999a172e72 into develop 7 months ago
kske deleted branch f/handler-inheritance 7 months ago

Reviewers

delvh approved these changes 7 months ago
zdm/event-bus/pipeline/head This commit looks good
The pull request has been merged as 999a172e72.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: zdm/event-bus#34
Loading…
There is no content yet.