Inherit Event Handlers #34

Merged
kske merged 2 commits from f/handler-inheritance into develop 2022-01-12 17:19:58 +01:00
5 changed files with 25 additions and 11 deletions
Showing only changes of commit 722bf2b999 - Show all commits

View File

@ -225,7 +225,10 @@ To avoid this, system events never cause system events and instead just issue a
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`. 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 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 contains an implementation, it is ignored as expected. 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 ## Debugging

View File

@ -7,7 +7,6 @@ import java.lang.reflect.*;
import java.util.*; import java.util.*;
import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Consumer; import java.util.function.Consumer;
import java.util.stream.Collectors;
import dev.kske.eventbus.core.handler.*; import dev.kske.eventbus.core.handler.*;
@ -272,8 +271,9 @@ public final class EventBus {
Set<Method> methods = getMethodsAnnotatedWith(listenerClass, Event.class); Set<Method> methods = getMethodsAnnotatedWith(listenerClass, Event.class);
// Recursively add superclass handlers // Recursively add superclass handlers
if (listenerClass.getSuperclass() != null) Class<?> superClass = listenerClass.getSuperclass();
methods.addAll(getHandlerMethods(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 // Recursively add interface handlers
for (Class<?> iClass : listenerClass.getInterfaces()) for (Class<?> iClass : listenerClass.getInterfaces())
@ -292,9 +292,12 @@ public final class EventBus {
*/ */
private Set<Method> getMethodsAnnotatedWith(Class<?> enclosingClass, private Set<Method> getMethodsAnnotatedWith(Class<?> enclosingClass,
Class<? extends Annotation> annotationClass) { Class<? extends Annotation> annotationClass) {
return Arrays.stream(enclosingClass.getDeclaredMethods()) var methods = new HashSet<Method>();
.filter(m -> m.isAnnotationPresent(annotationClass)) for (var method : enclosingClass.getDeclaredMethods())
.collect(Collectors.toSet()); 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;
} }
/** /**

View File

@ -5,7 +5,8 @@ import static org.junit.jupiter.api.Assertions.assertSame;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
/** /**
* Tests whether event handlers correctly work in the context of an inheritance hierarchy. * 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 * @author Kai S. K. Engelbart
* @since 1.3.0 * @since 1.3.0
@ -20,12 +21,12 @@ class InheritanceTest extends SimpleEventListenerBase implements SimpleEventList
var event = new SimpleEvent(); var event = new SimpleEvent();
bus.dispatch(event); bus.dispatch(event);
assertSame(4, event.getCounter()); assertSame(3, event.getCounter());
} }
@Override @Override
void onSimpleEventAbstractHandler(SimpleEvent event) { void onSimpleEventAbstractHandler(SimpleEvent event) {
event.increment(); assertSame(1, event.getCounter());
} }
@Override @Override
@ -35,6 +36,7 @@ class InheritanceTest extends SimpleEventListenerBase implements SimpleEventList
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 @Event
private void onSimpleEventPrivate(SimpleEvent event) { private void onSimpleEventPrivate(SimpleEvent event) {
assertSame(0, event.getCounter());
event.increment(); event.increment();
} }
} }

View File

@ -1,20 +1,25 @@
package dev.kske.eventbus.core; 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}. * An abstract class defining a package-private and a private handler for {@link SimpleEvent}.
* *
* @author Kai S. K. Engelbart * @author Kai S. K. Engelbart
* @since 1.3.0 * @since 1.3.0
*/ */
@Priority(200)
abstract class SimpleEventListenerBase { abstract class SimpleEventListenerBase {
@Event @Event
void onSimpleEventAbstractHandler(SimpleEvent event) { void onSimpleEventAbstractHandler(SimpleEvent event) {
event.increment(); fail("This handler should not be invoked");
} }
@Priority(150)
@Event @Event
private void onSimpleEventPrivate(SimpleEvent event) { private void onSimpleEventPrivate(SimpleEvent event) {
assertSame(1, event.getCounter());
event.increment(); event.increment();
} }
} }

View File

@ -8,6 +8,7 @@ package dev.kske.eventbus.core;
*/ */
interface SimpleEventListenerInterface { interface SimpleEventListenerInterface {
@Priority(120)
@Event @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); void onSimpleEventInterfaceHandler(SimpleEvent event);
} }