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

Merged
kske merged 4 commits from f/new-annotations into develop 2 years ago
kske commented 2 years ago
Owner

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 2 years ago
kske added 3 commits 2 years ago
3a6ebe9a19
Replace includeSubtypes with @Polymorphic
9b1c708514
Replace priority with @Priority
kske requested review from delvh 2 years ago
kske requested review from DieGurke 2 years ago
kske added the due date 2021-02-16 2 years ago
kske changed title from Split @Event Parameters Into @Polymorphic and @Property to Split @Event Parameters Into @Polymorphic and @Property, Remove Market Interfaces 2 years ago
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 2 years ago
delvh approved these changes 2 years ago
delvh left a comment
Owner

👍

👍
public void dispatch(IEvent event) {
public void dispatch(Object event) {
Objects.requireNonNull(event);
logger.log(Level.INFO, "Dispatching event {0}", event);
delvh commented 2 years ago
Owner

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 2 years ago
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
*/
public void registerListener(EventListener listener) throws EventBusException {
public void registerListener(Object listener) throws EventBusException {
Objects.requireNonNull(listener);
delvh commented 2 years ago
Owner

This line can again be inlined

This line can again be inlined
delvh commented 2 years ago
Owner

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 2 years ago
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.
delvh commented 2 years ago
Owner

@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 2 years ago
Owner

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.
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 2 years ago
Owner

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 2 years ago
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
@Override
public int compareTo(EventHandler other) {
int priority = other.annotation.priority() - annotation.priority();
int priority = other.priority - this.priority;
delvh commented 2 years ago
Owner

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 2 years ago
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
* @since 0.0.1
*/
void execute(IEvent event) throws EventBusException {
void execute(Object event) throws EventBusException {
delvh commented 2 years ago
Owner

Why is that now an object instead of an event?

Why is that now an object instead of an event?
kske commented 2 years ago
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 2 years ago
kske merged commit 39c51c8953 into develop 2 years ago
kske deleted branch f/new-annotations 2 years ago
kske added the
core
label 2 years ago

Reviewers

DieGurke was requested for review 2 years ago
delvh approved these changes 2 years ago
The pull request has been merged as 39c51c8953.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

2021-02-16

Dependencies

No dependencies set.

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