Closed pambrose closed 2 years ago
Ah, very interesting. I had not considered custom types. Let me rework it a bit.
Hi Holger,
I played around with using generics and I can see 4 ways of dealing with it. I wanted to see what you think. Here are examples of each: 1.
addAsyncEventListener<ComponentStateChangeEvent>{ event: ComponentStateChangeEvent ->
println("onComponentStateChangeEvent: $event")
}
addEventListener(
asyncEventListener<ComponentStateChangeEvent> { event: ComponentStateChangeEvent ->
println("onComponentStateChangeEvent: $event")
}
)
addEventListener(
asyncEventListener<ComponentStateChangeEvent> {
onEvent { event: ComponentStateChangeEvent ->
println("onComponentStateChangeEvent: $event")
}
}
)
addEventListener(
asyncEventListener {
onEvent<ComponentStateChangeEvent> { event: Event ->
println("onComponentStateChangeEvent: $event")
}
}
)
Pros and cons that come to mind are:
addEventListener()
.I would prefer to not see the duplication.
addEventListener()
call and will not break any code.It also allow us to offer other versions of listeners, e.g., sync, verbose, whatever, using the same
addEventListener()
call.
onEvent()
calls to be specified, but they would all correspond to the same type of Event
(within that scope). I am not sure multiple onEvent calls is valuable in practice.onEvent()
calls, but because of inline restrictions in the implementation, the argument of each onEvent()
call is only Event
and not the T
one would want. That is not very cool.I was just playing with doing a synchronous listener for #2 and I think I like offering both options using the same addEventListner()
call:
addEventListener(
asyncEventListener<ComponentStateChangeEvent> { event: ComponentStateChangeEvent ->
println("onComponentStateChangeEvent: $event")
}
)
and
addEventListener(
syncEventListener<ComponentStateChangeEvent> { event: ComponentStateChangeEvent ->
println("onComponentStateChangeEvent: $event")
}
)
My favourite would be clearly (1) with quite a margin, as it's most concise and very readable. I don't mind the breaking change, since we/I have pointed on multiple occasions in the documentation (release notes, changelog, etc.) that the API is not yet stable.
Starting with the first major release, API stability will be for sure something to weigh carefully against cosmetical improvements.
I added an implementation of #1 and I like it. The only catch is the eventListeners value has to be public because addEventListener() is inlined and cannot reference private values. Also, I did not include async in the method name because I am assuming one would not want a synchronous version messing with the timing. Let me know if that is a bad assumption. Also, this does not break the existing API, so we are fine there.
I've slightly reworked the implementation, managed to make the listeners private again, simplified the docs, and added a test org.kalasim.test.EnvTests#it should consume events asynchronously
. This test fails at the moment. The console logs are
time current receiver action info
--------- --------------------- --------------------- ------------------------------------------------------ ----------------------------------
.00 main Created
.00 ComponentGenerator.1 Created
.00 Activated, scheduled for .00 New state: scheduled
.00 main Running +5.00, scheduled for 5.00 New state: scheduled
.00 ComponentGenerator.1 ComponentGenerator.1 Hold +1.00, scheduled for 1.00 New state: scheduled
1.00 Car.0 Created
1.00 ComponentGenerator.1 Hold +1.00, scheduled for 2.00 New state: scheduled
2.00 Car.1 Created
2.00 ComponentGenerator.1 Hold +1.00, scheduled for 3.00 New state: scheduled
3.00 Car.2 Created
3.00 ComponentGenerator.1 Hold +1.00, scheduled for 4.00 New state: scheduled
4.00 Car.3 Created
4.00 ComponentGenerator.1 Hold +1.00, scheduled for 5.00 New state: scheduled
received event {"current":"ComponentGenerator.1","receiver":"ComponentGenerator.1","action":"hold +1.00, scheduled for 4.00","details":"New state: scheduled","time":"3.00"}
So the debug print (to be removed for sure) in https://github.com/holgerbrandl/kalasim/blob/46827ec7c4cda9af15f33fa08d2f4ad83bb54bc0/src/main/kotlin/org/kalasim/misc/AsyncEventListener.kt#L19 is only called once. Only the first event is consumed by the async listener. Do you have an idea why?
I would have a second related question. Would the coroutine Channel API allow to add a second consumer as shown in the commented code in the mentioned test implementation? Or can a flow be consumed only once?
Thanks @pambrose for the great suggestion. My first experiment to enable async event consumption clearly lacked beauty. Your PR is much cleaner.
The only concern I'd like to raise is the limitation to existing event types. In the simulations we are doing, we typically create lots of other custom types which we can consume very selectively. E.g.
Actually, in some simulations we exclusively monitor the custom events and do not mind about the built-in event types, which can for performance sometimes even be suppressed via https://www.kalasim.org/advanced/#continuous-simulation
So, I wonder if the entire event API should support/follow a reified minimalistic approach for in-sync and async listeners?
Or maybe even just
addListener<T?
?Clearly, the event API should follow what's best-practise in kotlin, so that new users feel at home right very quickly.
How do you feel about that?