Inherit Event Handlers #34

Merged
kske merged 2 commits from f/handler-inheritance into develop 2022-01-12 17:19:58 +01:00
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 2022-01-09 14:17:35 +01:00
kske added 1 commit 2022-01-09 14:17:36 +01:00
Inherit event handlers
All checks were successful
zdm/event-bus/pipeline/head This commit looks good
7fb633d69f
When registering an event listener, Event Bus recursively walks the
entire inheritance tree and looks for event handlers.
kske requested review from delvh 2022-01-09 14:17:39 +01:00
kske requested review from mpk 2022-01-09 14:17:39 +01:00
delvh reviewed 2022-01-09 16:04:00 +01:00
@ -224,0 +224,4 @@
## 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.
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)
Author
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
@ -224,0 +225,4 @@
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.
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.
Owner

Or do I understand that wrong?

Or do I understand that wrong?
Author
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.
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
@ -260,0 +272,4 @@
Set<Method> methods = getMethodsAnnotatedWith(listenerClass, Event.class);
// Recursively add superclass handlers
if (listenerClass.getSuperclass() != null)
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
@ -260,0 +294,4 @@
Class<? extends Annotation> annotationClass) {
return Arrays.stream(enclosingClass.getDeclaredMethods())
.filter(m -> m.isAnnotationPresent(annotationClass))
.collect(Collectors.toSet());
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
@ -0,0 +33,4 @@
event.increment();
}
@Event
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
@ -0,0 +9,4 @@
interface SimpleEventListenerInterface {
@Event
void onSimpleEventInterfaceHandler(SimpleEvent event);
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?
Author
Owner

There is no reason why it shouldn't be.

There is no reason why it shouldn't be.
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.
Author
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 2022-01-12 15:59:48 +01:00
Test priorities for inheritance
All checks were successful
zdm/event-bus/pipeline/head This commit looks good
722bf2b999
delvh approved these changes 2022-01-12 17:02:27 +01:00
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 mpk 2022-01-12 17:19:35 +01:00
kske merged commit 999a172e72 into develop 2022-01-12 17:19:58 +01:00
kske deleted branch f/handler-inheritance 2022-01-12 17:19:58 +01:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: zdm/event-bus#34
No description provided.