-
-
Notifications
You must be signed in to change notification settings - Fork 32
Rename interface to OkHttp3SocketClientFactory.java. #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
import java.net.URI; | ||
import java.util.Locale; | ||
|
||
/* package */ class TubeSockWebSocketClient implements WebSocketClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also remove the gradle dependency then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah somehow it was stashed but wasn't committed, repushed
f2fe211
to
b56d6ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions and suggestions before going forward
return new OkHttp3WebSocketClient(mClient, webSocketClientCallback, hostUrl); | ||
} | ||
|
||
class OkHttp3WebSocketClient implements WebSocketClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you didn't keep the Socket implementation into its own file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TubeSocketClient has it reversed (the factory is inside TubeSocketClient). That works for static classes, but this class requires an instance to OkHttp3 so Java complains. I reversed it because it's actually the factory class we need access to...the constructors in OkHttp3WebSocketCient/TubeSocketClient are private methods and I don't think the user needs access to that stuff in general so keeping it in the same file avoids having to expose those constructors publicly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
this.webSocketClientCallback.stateChanged(); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks
} | ||
|
||
/* package */ ParseLiveQueryClientImpl(URI uri, OkHttpClient okHttpClient) { | ||
this(uri, new OkHttp3SocketClientFactory(okHttpClient), Task.BACKGROUND_EXECUTOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could just accept factory instead of a specific OkHttpClient, so we keep the constructors more generic (interface). Just by passing a WebSocketClientFactory instance (new OkHttp3SocketClientFactory(new OkHttpClient())
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Changed.
I kept the default implementation (line 36) as the default.
…QueryClient to accept OkHttpClient instances.
54ab88b
to
1fc1ba7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 Small nits then it's all good
import okhttp3.WebSocketListener; | ||
import okio.ByteString; | ||
|
||
/* package */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm I think you forgot that one? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks fixed
@@ -21,10 +20,18 @@ public static ParseLiveQueryClient getClient() { | |||
return new ParseLiveQueryClientImpl(); | |||
} | |||
|
|||
public static ParseLiveQueryClient getClient(WebSocketClientFactory webSocketClientFactory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few more interface methods (so you don't need to specify the Uri thanks to changes to @hermanliang
still good |
Thanks! |
Modify ParseLiveQueryClient to accept OkHttpClient instances.
@mmimeault