Core: Move onReady and onDisconnect from Remote to Controller + Add new Controller Lifecycle Callbacks
TinaTiel opened this issue · 4 comments
As it is now, the onReady and onDisconnect callbacks are in the RemoteController.
This doesn't make sense for several reasons:
- onReady shouldn't be called until onIdentified has been called in Communicator.
- onDisconnect shouldn't be called until onClose has been called in Communicator.
- In both cases above, as it is now due to thread timing onReady may likely be called before onIdentifed, or onDisconnect may be called before onClose
Rather than coming up with a complex structure change to send callbacks back through the communicator to the controller, it would be simpler (and more correct) to move these callbacks into the Communicator--closest to their source.
We should instead add onStartRequested, onStarting, onStopRequested, onStopping, and onIgnored as lifecycle callbacks in the controller; e.g. for cases where a start is requested while already starting, or a stop is requested while stopped.
So, on CommunicatorListener we'd have:
onConnect
onHello
onIdentified
onReady
onClose (reminder: auth failures happen here; OBS Websockets closes the connection with an error code)
onDisconnect
onError (for connector errors...NPEs, or possibly serialization/deserialization problems)
And on RemoteListener we'd only have:
onStartRequested (new/recommended)
onStarting (new/recommended)
onStopRequested (new/recommended)
onStopping (new/recommended)
onIgnored (new/recommended)
onError (for controller errors...Obs unreachable, or unexpected Jetty WebSocketClient exceptions)
These changes should enable fixing #34 , but that will be done in a separate body of work.
What's the difference between :
- onStartRequested (new/recommended)
- onStarting (new/recommended)
same goes for :
- onStopRequested (new/recommended)
- onStopping (new/recommended)
The onXRequested is always called, as an acknowledgement of receipt.
From there, onIgnored or onX will be called depending on whether the request can be fulfilled or will be ignored.
They could be optional tbh, as onIgnored and onX are most important; onXRequested provide mostly completeness/optics.
For now, let's forgo the extra events...There's enough as-is in this issue, and adding those events implies having the ability to detect readiness from the controller (same problem this issue addresses, lol). We could always add those later if needed.