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

Merged
kske merged 4 commits from f/new-annotations into develop 2021-02-15 20:38:19 +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
3a6ebe9a19
Replace includeSubtypes with @Polymorphic
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.
9b1c708514
Replace priority with @Priority
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.
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
Owner

👍

👍
@ -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);

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

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

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

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
@ -135,3 +135,3 @@
*/
public void registerListener(EventListener listener) throws EventBusException {
public void registerListener(Object listener) throws EventBusException {
Objects.requireNonNull(listener);

This line can again be inlined

This line can again be inlined

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.
Poster
Owner

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.

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

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.
@ -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());

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.
Poster
Owner

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
@ -72,3 +78,3 @@
@Override
public int compareTo(EventHandler other) {
int priority = other.annotation.priority() - annotation.priority();
int priority = other.priority - this.priority;

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? 😉
Poster
Owner

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
@ -90,3 +98,3 @@
* @since 0.0.1
*/
void execute(IEvent event) throws EventBusException {
void execute(Object event) throws EventBusException {

Why is that now an object instead of an event?

Why is that now an object instead of an event?
Poster
Owner

Because I got rid of the IEvent interface so that every object can be used as an event.

Because I got rid of the `IEvent` interface so that every object can be used as an event.
kske marked this conversation as resolved
kske force-pushed f/new-annotations from be38c1fd02 to 002180ed3b 2021-02-15 20:35:14 +01:00 Compare
kske merged commit 39c51c8953 into develop 2021-02-15 20:38:19 +01:00
kske deleted branch f/new-annotations 2021-02-15 20:38:24 +01:00
kske added the
core
label 2021-02-20 22:21:05 +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'.

2021-02-16

Dependencies

No dependencies set.

Reference: zdm/event-bus#5
There is no content yet.