Split @Event Parameters Into @Polymorphic and @Property, Remove Marker Interfaces #5

kske · 2021-02-15T13:53:57+01:00

kske commented

2021-02-15 13:53:57 +01:00

This pull request removes the parameters includeSubtypes and priority from the Event annotation in favor of the two annotations @Polymorphic and @Priority.

Using separate annotations for optional parameters not only improves legibility, but also allows extensions of the event API without modifying the @Event annotation or even event-bus-core for that matter.

This is an essential step in the direction of sub-modules that integrate with frameworks like Swing and JavaFX and their respective event APIs.

Additionally, the marker interfaces EventListener and IEvent have been removed as they serve no actual purpose and prevent Event Bus from directly interfacing with preexisting objects without their modification.

This pull request removes the parameters `includeSubtypes` and `priority` from the `Event` annotation in favor of the two annotations `@Polymorphic` and `@Priority`. Using separate annotations for optional parameters not only improves legibility, but also allows extensions of the event API without modifying the `@Event` annotation or even `event-bus-core` for that matter. This is an essential step in the direction of sub-modules that integrate with frameworks like Swing and JavaFX and their respective event APIs. Additionally, the marker interfaces `EventListener` and `IEvent` have been removed as they serve no actual purpose and prevent Event Bus from directly interfacing with preexisting objects without their modification.

kske self-assigned this 2021-02-15 13:53:57 +01:00

kske added 3 commits 2021-02-15 13:53:57 +01:00

Replace includeSubtypes with @Polymorphic 3a6ebe9a19

The new @Polymorphic annotation serves the exact same purpose as
@Event(includeSubtypes = true), but should be easier to read in complex
handler declarations. It has to be used in conjunction with the @Event
annotation, not instead of it.

Replace priority with @Priority 9b1c708514

The new @Priority annotation serves the exact same purpose as
@Event(priority = ...), but should be easier to read in complex handler
declarations. It has to be used in conjunction with the @Event
annotation, not instead of it.

Rename Event#eventType to Event#value for more concise usage cd2e7ad023

kske requested review from delvh 2021-02-15 13:54:01 +01:00

kske requested review from mpk 2021-02-15 13:54:01 +01:00

kske added the due date 2021-02-16

2021-02-15 13:54:12 +01:00

kske changed title from ~~Split @Event Parameters Into @Polymorphic and @Property~~ to Split @Event Parameters Into @Polymorphic and @Property, Remove Market Interfaces

2021-02-15 14:43:15 +01:00

kske changed title from ~~Split @Event Parameters Into @Polymorphic and @Property, Remove Market Interfaces~~ to Split @Event Parameters Into @Polymorphic and @Property, Remove Marker Interfaces

2021-02-15 14:45:11 +01:00

delvh approved these changes 2021-02-15 15:37:59 +01:00

delvh left a comment

👍

event-bus-core/src/main/java/dev/kske/eventbus/core/EventBus.java

						
				@ -68,3 +68,3 @@

					public void dispatch(IEvent event) {

					public void dispatch(Object event) {

						Objects.requireNonNull(event);

						logger.log(Level.INFO, "Dispatching event {0}", event);

delvh commented

2021-02-15 14:58:39 +01:00

theoretically you can even save a line here by inlining Objects.requireNonNull

theoretically you can even save a line here by inlining `Objects.requireNonNull`

kske commented

2021-02-15 20:21:32 +01:00

I coould, but this would mix the actual logic of the method with the logging and bloat up the line.

kske marked this conversation as resolved

event-bus-core/src/main/java/dev/kske/eventbus/core/EventBus.java

						
				@ -135,3 +135,3 @@

					 */

					public void registerListener(EventListener listener) throws EventBusException {

					public void registerListener(Object listener) throws EventBusException {

						Objects.requireNonNull(listener);

delvh commented

2021-02-15 15:01:11 +01:00

This line can again be inlined

delvh commented

2021-02-15 15:09:50 +01:00

Also something I noticed:

Maybe it wouldn't be a bad idea to use a WeakReference for referencing the object instead (or another one of these gc-able references).
Otherwise, we might end up with a memory leak in Java as no object defined as event handler can ever be garbage collected, even if they would otherwise be garbage collected.

Also something I noticed: Maybe it wouldn't be a bad idea to use a `WeakReference` for referencing the object instead (or another one of these gc-able references). Otherwise, we might end up with a memory leak **in Java** as no object defined as event handler can ever be garbage collected, even if they would otherwise be garbage collected.

kske commented

2021-02-15 20:23:15 +01:00

But isn't that what we want to achieve? A dedicated event listener would not necessarily be referenced from other classes apart from EventBus. Also, EventBus allows the deletion of event listeners, so I don't see this as much of a problem.

But isn't that what we want to achieve? A dedicated event listener would not necessarily be referenced from other classes apart from `EventBus`. Also, `EventBus` allows the deletion of event listeners, so I don't see this as much of a problem.

delvh commented

2021-02-15 23:39:51 +01:00

@kske Yes, but that assumes that a user manually unregisters any registered object that should be gc-able. But think to our own use cases: Did we ever use removeListener? I can't remember a single instance where we used that.
And as we now no longer require EventListeners, which signify that the object is supposed to be long lasting, we can now declare anything as an event listener. So, what could potentially happen is that we register i.e. a list for a certain event, and then we swap out the list for another list. The only reason why this list will stay inside the memory is because of the reference from EventBus.
Do you remember i.e. the edge cases in Envoy where we wanted to add an event listener on the content of a cell, but couldn't as it would lead to unpredictable results? Doing that would then be possible.

@kske Yes, but that assumes that a user manually unregisters any registered object that should be gc-able. But think to our own use cases: Did we ever use `removeListener`? I can't remember a single instance where we used that. And as we now no longer require EventListeners, which signify that the object is supposed to be long lasting, we can now declare anything as an event listener. So, what could potentially happen is that we register i.e. a list for a certain event, and then we swap out the list for another list. The only reason why this list will stay inside the memory is because of the reference from `EventBus`. Do you remember i.e. the edge cases in Envoy where we wanted to add an event listener on the content of a cell, but couldn't as it would lead to unpredictable results? Doing that would then be possible.

delvh commented

2021-02-15 23:42:13 +01:00

With swap I mean rereference (i.e.
eventBus.register(list);
list = new ArrayList<>()
eventBus.register(list);).
This would lead to a memory leak in the current implementation.
With the proposed mechanism this would be possible.

With _swap_ I mean rereference (i.e. `eventBus.register(list);` `list = new ArrayList<>()` `eventBus.register(list);`). This would lead to a memory leak in the current implementation. With the proposed mechanism this would be possible.

event-bus-core/src/main/java/dev/kske/eventbus/core/EventBus.java

						
				@ -173,3 +173,3 @@

					public void removeListener(EventListener listener) {

					public void removeListener(Object listener) {

						Objects.requireNonNull(listener);

						logger.log(Level.INFO, "Removing event listener {0}", listener.getClass().getName());

delvh commented

2021-02-15 15:00:43 +01:00

Isn't the Objects.requireNonNull unnecessary overhead? A null pointer exception will be thrown the moment getClass() gets called one line below.

Isn't the `Objects.requireNonNull` unnecessary overhead? A null pointer exception will be thrown the moment `getClass()` gets called one line below.

kske commented

2021-02-15 20:24:54 +01:00

It would, but Objects.requireNonNull makes it immediately obvious to the caller that the parameter has to be non-null. I agree that it doesn't make much of the difference, but as it's handled like this in multiple methods I won't bother changing it for somethign equivalent.

It would, but `Objects.requireNonNull` makes it immediately obvious to the caller that the parameter has to be non-null. I agree that it doesn't make much of the difference, but as it's handled like this in multiple methods I won't bother changing it for somethign equivalent.

delvh marked this conversation as resolved

event-bus-core/src/main/java/dev/kske/eventbus/core/EventHandler.java

						
				@ -72,3 +78,3 @@

					@Override

					public int compareTo(EventHandler other) {

						int priority = other.annotation.priority() - annotation.priority();

						int priority = other.priority - this.priority;

delvh commented

2021-02-15 15:04:38 +01:00

What about using Integer.compare(this.priority, other.priority)?
Or is that too much overhead? 😉

What about using `Integer.compare(this.priority, other.priority)`? Or is that too much overhead? 😉

kske commented

2021-02-15 20:33:01 +01:00

Actually, yes I think so. Integer.compare calls Integer.valueOf two times, generating two objects in the process just to execute a statement similar to the one I wrote.

But, to your credit, I didn't even consider the possibility, to thanks for the suggestion :)

Actually, yes I think so. `Integer.compare` calls `Integer.valueOf` two times, generating two objects in the process just to execute a statement similar to the one I wrote. But, to your credit, I didn't even consider the possibility, to thanks for the suggestion :)

kske marked this conversation as resolved

event-bus-core/src/main/java/dev/kske/eventbus/core/EventHandler.java Outdated

						
				@ -90,3 +98,3 @@

					 * @since 0.0.1

					 */

					void execute(IEvent event) throws EventBusException {

					void execute(Object event) throws EventBusException {