Inherit Event Handlers #34

Merged
kske merged 2 commits from f/handler-inheritance into develop 2022-01-12 17:19:58 +01:00
8 changed files with 163 additions and 6 deletions

View File

@ -221,6 +221,15 @@ The same applies when an exception event handler throws an exception.
To avoid this, system events never cause system events and instead just issue a warning to the logger.
## 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 marked this conversation as resolved
Review

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)
Review

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.
If the overridden method already contains an implementation in the superclass, the superclass implementation is ignored as expected.
kske marked this conversation as resolved Outdated
Outdated
Review

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.
Outdated
Review

Or do I understand that wrong?

Or do I understand that wrong?
Outdated
Review

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.
Outdated
Review

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

The topmost comment was intended as a suggestion for the README.
The `@Priority` and `@Polymorphic` annotations are inherited both on a class and on a method level.
If the priority or polymorphism has to be redefined on an inherited handler, the `@Event` annotation has to be added explicitly.
## Debugging
In more complex setups, taking a look at the event handler execution order can be helpful for debugging.

View File

@ -2,7 +2,8 @@ package dev.kske.eventbus.core;
import java.lang.System.Logger;
import java.lang.System.Logger.Level;
import java.lang.reflect.InvocationTargetException;
import java.lang.annotation.Annotation;
import java.lang.reflect.*;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Consumer;
@ -14,9 +15,8 @@ import dev.kske.eventbus.core.handler.*;
* <p>
* A singleton instance of this class can be lazily created and acquired using the
* {@link EventBus#getInstance()} method.
* <p>
* This is a thread-safe implementation.
*
* @implNote This is a thread-safe implementation.
* @author Kai S. K. Engelbart
* @since 0.0.1
* @see Event
@ -237,7 +237,7 @@ public final class EventBus {
priority = listener.getClass().getAnnotation(Priority.class).value();
registeredListeners.add(listener);
for (var method : listener.getClass().getDeclaredMethods()) {
for (var method : getHandlerMethods(listener.getClass())) {
Event annotation = method.getAnnotation(Event.class);
// Skip methods without annotations
@ -257,6 +257,49 @@ public final class EventBus {
listener.getClass().getName());
}
/**
* Searches for event handling methods declared inside the inheritance hierarchy of an event
* listener.
*
* @param listenerClass the class to inspect
* @return all event handling methods defined for the given listener
* @since 1.3.0
*/
private Set<Method> getHandlerMethods(Class<?> listenerClass) {
// Get methods declared by the listener
Set<Method> methods = getMethodsAnnotatedWith(listenerClass, Event.class);
// Recursively add superclass handlers
Class<?> superClass = listenerClass.getSuperclass();
if (superClass != null && superClass != Object.class)
kske marked this conversation as resolved Outdated
Outdated
Review
		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)); ```
methods.addAll(getHandlerMethods(superClass));
// Recursively add interface handlers
for (Class<?> iClass : listenerClass.getInterfaces())
methods.addAll(getHandlerMethods(iClass));
return methods;
}
/**
* Searches for declared methods with a specific annotation inside a class.
*
* @param enclosingClass the class to inspect
* @param annotationClass the annotation to look for
* @return all methods matching the search criteria
* @since 1.3.0
*/
private Set<Method> getMethodsAnnotatedWith(Class<?> enclosingClass,
Class<? extends Annotation> annotationClass) {
var methods = new HashSet<Method>();
for (var method : enclosingClass.getDeclaredMethods())
if (method.isAnnotationPresent(annotationClass))
kske marked this conversation as resolved Outdated
Outdated
Review
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; ```
methods.add(method);
return methods;
}
/**
* Registers a callback listener, which is a consumer that is invoked when an event occurs. The
* listener is not polymorphic and has the {@link #DEFAULT_PRIORITY}.

View File

@ -18,6 +18,7 @@ import java.lang.annotation.*;
* @see Event
*/
@Documented
@Inherited
@Retention(RUNTIME)
@Target({ METHOD, TYPE })
public @interface Polymorphic {

View File

@ -21,6 +21,7 @@ import java.lang.annotation.*;
* @see Event
*/
@Documented
@Inherited
@Retention(RUNTIME)
@Target({ METHOD, TYPE })
public @interface Priority {

View File

@ -0,0 +1,42 @@
package dev.kske.eventbus.core;
import static org.junit.jupiter.api.Assertions.assertSame;
import org.junit.jupiter.api.Test;
/**
* Tests whether event handlers correctly work in the context of an inheritance hierarchy. The
* effect of handler priorities is also accounted for.
*
* @author Kai S. K. Engelbart
* @since 1.3.0
*/
class InheritanceTest extends SimpleEventListenerBase implements SimpleEventListenerInterface {
EventBus bus = new EventBus();
@Test
void test() {
bus.registerListener(this);
var event = new SimpleEvent();
bus.dispatch(event);
assertSame(3, event.getCounter());
}
@Override
void onSimpleEventAbstractHandler(SimpleEvent event) {
assertSame(1, event.getCounter());
}
@Override
public void onSimpleEventInterfaceHandler(SimpleEvent event) {
event.increment();
}
kske marked this conversation as resolved
Review

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.
@Event
private void onSimpleEventPrivate(SimpleEvent event) {
assertSame(0, event.getCounter());
event.increment();
}
}

View File

@ -1,9 +1,31 @@
package dev.kske.eventbus.core;
/**
* A simple event for testing purposes.
* A simple event for testing purposes. The event contains a counter that is supposed to be
* incremented when the event is processed by a handler. That way it is possible to test whether all
* handlers that were supposed to be invoked were in fact invoked.
*
* @author Kai S. K. Engelbart
* @since 0.0.1
*/
class SimpleEvent {}
class SimpleEvent {
private int counter;
@Override
public String toString() {
return String.format("SimpleEvent[%d]", counter);
}
void increment() {
++counter;
}
int getCounter() {
return counter;
}
void reset() {
counter = 0;
}
}

View File

@ -0,0 +1,25 @@
package dev.kske.eventbus.core;
import static org.junit.jupiter.api.Assertions.*;
/**
* An abstract class defining a package-private and a private handler for {@link SimpleEvent}.
*
* @author Kai S. K. Engelbart
* @since 1.3.0
*/
@Priority(200)
abstract class SimpleEventListenerBase {
@Event
void onSimpleEventAbstractHandler(SimpleEvent event) {
fail("This handler should not be invoked");
}
@Priority(150)
@Event
private void onSimpleEventPrivate(SimpleEvent event) {
assertSame(1, event.getCounter());
event.increment();
}
}

View File

@ -0,0 +1,14 @@
package dev.kske.eventbus.core;
/**
* An interface defining a single handler for {@link SimpleEvent}.
*
* @author Kai S. K. Engelbart
* @since 1.3.0
*/
interface SimpleEventListenerInterface {
@Priority(120)
@Event
delvh marked this conversation as resolved
Review

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?
Review

There is no reason why it shouldn't be.

There is no reason why it shouldn't be.
Review

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.
Review

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.
void onSimpleEventInterfaceHandler(SimpleEvent event);
}