[WIP] JavaDocs #1
Conversation
Main concepts and changes: - ListenerList is merged into EventBus. Groups of them are called a BusGroup now. - Posting, registration, etc is all done directly on the events you care about - Events can be records or mutable classes and can opt-into various characteristics - Events can support multi-inheritance with an opt-in, and selectively opt-out certain subclasses in the inheritance chain using `@MarkerEvent` - Only monitoring events can receiveCancelled now - Cancellation and monitoring state are decoupled from the event objects themselves, allowing for records and other immutable data structures (such as the upcoming Valhalla value classes) - Strong encapsulation with module system enforcement and sealed types, good separation between API spec and implementation to allow freedom for future internal enhancements
- If the event is final or a record, use a shared empty list. - If the event is sealed, pre-size the list based on the number of known children
The inheritance didn't sadly play nicely with generics, so made it optional
While possibly useful, I can't see anywhere this could actually be used in Forge and don't want to add too many features out of the gate that might not have any meaningful usages
…used generic param
Allows for less verbose and more semantical usages without requiring static imports, such as `MyEvent() implements Cancellable, RecordEvent`
In Forge we prevent adding game events to the mod bus with a baseType check, adding this feature back in will allow us to continue doing so.
Library users should rely on its subtypes instead, such as `RecordEvent`. By making the supertype internal, it discourages external wrappers and unintended usage patterns.
This was a mistake - event listeners are not meant to be specific to a particular bus group because all event behaviour is on the event itself
NullAway doesn't support the `onlyNullMarked` option for modules yet - only packages and classes. Once they do, the package-info files can be removed in favour of the single `@NullMarked` annotation in the module-info.
It doesn't work as documented and I want to focus on getting more frequently used features stable
Tests that a listener can remove itself during post and that it won't be called again on the next event post
Jonathing
force-pushed
the
feat/jonathing/novabus/javadocs
branch
from
32fb383 to
3a4cf58
Compare
There was a problem hiding this comment.
Great start, left some comments.
Regarding the three event types, I think there's some misunderstanding that needs to be cleared up...
RecordEvent
For read-only or shallowly-immutable records. For example, think of a leaderboard event that gives you a list you can mutate, but doesn't let you set the list field to null or an immutable list and crash the game. Or maybe an event where you are notified of a player joining a server and can cancel it to kick them (when combined with the Cancellable characteristic), but can't set the player to someone else.
MutableEvent
For mutable event classes. Anything where you want to allow setting multiple specific fields inside the event object. More advanced techniques like protected fields and methods are also possible, where the supertype may be a protected abstract class with some internals handled for you but only the concrete types are actual events.
InheritableEvent
This is a hybrid of an event characteristic and an event type. With this, you opt-into event inheritance going all the way down the tree. You can apply this to classes or interfaces. One use-case for this is separating API and impl - you could have a public, exported, sealed event interface that implements InheritableEvent and the actual event be a record in an internal package. This allows for hiding the event's constructor while still being a record and other fancy Java module encapsulation features.
I recommend cloning the Forge integration PoC PR and browsing the migrated events to see how each type can be used. I'd be happy to hop on a call and answer any questions you have as well.
| * @throws IllegalArgumentException If a BusGroup with the given name already exists | ||
| * @apiNote In theory, it is possible to use any base type when creating a BusGroup. However, it is recommended to | ||
| * either use a direct subtype of {@link Event} or use {@link #create(String)} which uses the default type. | ||
| * @see #create(String) |
There was a problem hiding this comment.
The main purpose of baseType is to restrict EventBus grouping to specific user-defined categories of events rather than event types. You can restrict to specific event types if you want (e.g. a BusGroup that only allows record events), but you don't have to. Think of this as how Forge restricts only IModBusEvent being registered on mod buses - where the mod bus events could be interfaces (InheritableEvent), records (RecordEvent) or classes (MutableEvent).
| * Registers all static methods annotated with {@link SubscribeEvent} in the given class. | ||
| * | ||
| * @param callerLookup {@code MethodHandles.lookup()} from the class containing listeners | ||
| * @param callerLookup {@link MethodHandles#lookup()} from the class containing listeners |
There was a problem hiding this comment.
I use a code snippet here to make it obvious to Java noobs that they can copy-paste that into their code and don't have to care about what it does internally.
| EventListener addListener(ObjBooleanBiConsumer<T> listener); | ||
|
|
||
| /** | ||
| * Creates a new CancellableEventBus for the given event type on the {@linkplain BusGroup#DEFAULT default} {@link BusGroup}. |
There was a problem hiding this comment.
Needs the same warning as EventBus#create regarding static final storage
| * SPDX-License-Identifier: LGPL-2.1-only | ||
| */ | ||
| /** | ||
| * This package houses classes related to the characterists that |
There was a problem hiding this comment.
| * This package houses classes related to the characterists that | |
| * This package houses classes related to the characteristics that |
| * implementations provided by the EventBus API to be inherited from. This is an intentional design choice and allows | ||
| * for significant optimizations within the internal implementation. |
There was a problem hiding this comment.
Good, but I'd mention that it's more about API design than internal optimisations in this specific case.
Event is internal and sealed for a few reasons:
- To avoid ambiguity in the system where different events in the inheritance chain may technically be events but not declare their type
- To discourage bad usages by library users that could hurt performance. For example, having all listeners bounce to a generic
void handleEvent(Event e)method and doing chains ofinstanceofchecks. Another example being FML wrapping event dispatch in multiple allocating lambdas and ThreadLocal lookups. - To minimise public API surface - the more we have to support, the more restricted and complicated internals become
| * <p>Cancellable events are given special treatment by the EventBus system, as they have the unique ability to stop the | ||
| * execution of the posting of an event before the remaining listeners are able to interact with it. Additionally, when | ||
| * an event is cancelled, the only listeners that will receive it from that point on are | ||
| * {@linkplain Priority#MONITOR monitoring} listeners.</p> |
There was a problem hiding this comment.
Would be worth mentioning that monitoring listeners run last and are read-only (can't modify the event)
Jonathing
force-pushed
the
feat/jonathing/novabus/javadocs
branch
from
09a21d4 to
a5066c0
Compare
Jonathing
force-pushed
the
novabus
branch
2 times, most recently
from
1f4259e to
51f6169
Compare
2 participants
As promised, I'm expanding on (or adding) extensive JavaDocs to the work on NovaBus. Please leave your feedback and clarifications as much or as often as possible! I may be misremembering things.