Add callbacks to be notified of connect/disconnect/error events #34
Conversation
This enables a partial fix/workaround for parse-community#6, which otherwise wouldn't even be possible in its current state. Future changes will allow the client to automatically reconnect when the connection has been broken for any reason other than explicit disconnect().
| @@ -498,6 +539,30 @@ private static JSONObject createObjectDeleteMessage(int requestId, ParseObject p | |||
| return jsonObject; | |||
| } | |||
|
|
|||
| private static class LoggingCallbacks implements ParseLiveQueryClientCallbacks { | |||
There was a problem hiding this comment.
should this be non-static given how you instantiate it in each test?
There was a problem hiding this comment.
I don't think static vs non-static is relevant based on how it's instantiated here. It's declared static because it does not rely on anything from the outer/containing class (so it doesn't require an instance of the containing class).
It's probably not relevant in a unit test, but in general I prefer static inner class, unless it is explicitly required that the inner class instance must be associated with an instance of the outer class (such as a callback method that has to invoke something from the containing class). Not declaring the class static, even if you don't need make any calls to the outer class, can result in memory leaks since that instance of the inner class necessarily holds a reference to the instance of the outer class that created it.
There was a problem hiding this comment.
I was more worried that the use of transcript could be misused if the class isn't instantiated but it looks to be declared private so can't be accessed directly.
There was a problem hiding this comment.
Ah. Yeah the class still needs to be instantiated in order to use Transcript. It's declared final, but not static. So it is still a new Transcript instance for every instance of LoggingCallbacks (as it should be, in order to properly test that the callbacks are invoked when they're supposed to be).
Just want to make sure we're on the same page about the class and field declarations. The class being declared static does not mean that the transcript is a static field. It's still an instance field.
There was a problem hiding this comment.
Gotcha, thanks for the clarification.
| @@ -341,11 +385,13 @@ public Void then(Task<Void> task) { | |||
| @Override | |||
| public void onClose() { | |||
| Log.v(LOG_TAG, "Socket onClose"); | |||
| dispatchDisconnected(); | |||
There was a problem hiding this comment.
Do you need to put dispatchConnected() on onOpen()}
There was a problem hiding this comment.
dispatchConnected() happens only when the server responds with the op=connected message
This enables a partial fix/workaround for #6, which otherwise wouldn't even
be possible in its current state.
Future changes will allow the client to automatically reconnect when the
connection has been broken for any reason other than explicit disconnect().