Ported AndroidAsync Style Socket.IO API to standalone library #26

vinaysshenoy · 2013-07-10T01:41:37Z

I have a few doubts on how to handle certain things -

Earlier your AsyncServer would take care of queueing multiple SocketIO connect requests(for the initial setup). I'm currently using an AsyncTask that runs on a single thread. I'm thinking to have a Queue and the WebSocket's connect method runs a loop through the queue to connect websockets. We can make a relatively efficient queueing mechanism this way.
Is it worth it to switch between HttpClient and HttpUrlConnection for the webosocket handshake for device OS's? I thought of implementing it, but I felt it's not worth based on how much they are used.
The EventCallback method should be sending the event names as well. This way we can have a single listener object that listens for events. Or even route groups of events to different listeners.

addListener()

koush · 2013-07-10T01:46:04Z

Looks good, though I'm not a huge fan of changing WebSocketClient to WebSocket, and changing the Handler to distinct Callbacks. There's no need to break backwards compatibility there. Was here a reason for doing that?

I'd rather the corresponding changes were made in SocketIOClient to support WebSocketClient vs WebSocket. Should be relatively straightforward.

koush · 2013-07-10T01:47:54Z

Your questions:

If you use a send handler as in the original implementation, this would take care of queueing, wouldn't it?
Doesn't matter IMO.
Good point, please edit that and submit a fix to AndroidAsync as well if you want :)

vinaysshenoy · 2013-07-10T01:52:02Z

Oh, I thought we looking to move completely away from the old library style. I'll revert some things back to the older style.

I'll submit a fix for AndroidAsync as well. Btw, Are you planning to upgrade the README for EventCallbacks? It took me quite a while to figure out that I should be using client.addListener() instead of client.setEventCallback() which seemed to be missing.. I can submit a fix for that too.. but it might take a while as I'll do these only after completing the changes to this library.

koush · 2013-07-10T02:26:57Z

Oh, I completely forgot about the README. I had just updated the tests. I can fix that.

vinaysshenoy · 2013-07-11T16:03:08Z

I'm working on adding the earlier Handler style listener back to SocketIOClient. Would you mind if I renamed the interface to SocketIOCallbacks or SocketIOHandler instead of Handler?

koush · 2013-07-11T16:39:23Z

Go for it

vinaysshenoy · 2013-07-11T16:58:07Z

All right. And one more thing, in AndroidAsync, you can subscribe to events, while in the earlier android-websockets, you'd receive all events. Which one would you prefer to bring into this?

If we keep the new version(which I personally prefer), then in the onConnectCompleted(), we'll need to call

client.setSocketIOCallbacks(new SocketIOCallbacks());
client.listenForEvents(Arrays.asList("news", "chat"));

The final event will still come though Handler#on(event, args).

koush · 2013-07-11T17:24:49Z

Sorry, I was confused earlier. I thought you meant android.os.Handler.

Right now, none of the events fire back onto the main thread, and I thought you were fixing that.

The socket.io callbacks should remain similar to the ones exposed by AndroidAsync. Separate callbacks for Strings, JSON, and Event. And event callbacks are exposed via EventEmitter.

What are you trying to solve?

koush · 2013-07-11T17:30:46Z

To clarify, I do not want the old style SocketIOClient.Handler. In retrospect, I found that implementation kludgy.

However, I do think that events should be invoked onto the main ui thread. I'll handle that, as I need to do similar for AndroidAsync as well.

vinaysshenoy · 2013-07-11T17:32:34Z

I got confused too.. I thought that you preferred to expose socket messages via the earlier SocketIO.Handler interface.. So I added that in. And I wanted to rename it to SocketIOCallbacks since it was having the same name as an Android system class. Anyway, I'll revert it back.. Won't be too much of a problem.

koush · 2013-07-11T17:34:01Z

Cool, sorry for the confusion. Should have read your comment more thoroughly.

I suppose for the purposes of your existing code, you'd want to keep a SocketIOHandler around for easy transition. I think that can just be done outside of the library just as well though.

vinaysshenoy · 2013-07-11T17:54:06Z

Not a problem. I've already removed the interface.

As for the callbacks, I'm adding code to invoke them on the main thread now. Or rather, onto the same Handler that is passed to SocketIOClient.

vinaysshenoy · 2013-07-11T18:05:37Z

All right. It should be complete now. Could you review and let me know if I need to make any changes?

koush · 2013-07-11T18:45:04Z

There are changes to WebSocketClient and HybiParser. Member names, package renames, formatting, whitespace, etc. Those shouldn't be in the scope of this change.

The websocket/hybiparser should remain as untouched as possible.

koush · 2013-07-11T18:46:53Z

Those are originally created by @codebutler, so I want them to remain in his package prefix.

vinaysshenoy · 2013-07-11T18:50:04Z

Right, I'll fix those.

vinaysshenoy · 2013-07-11T19:10:08Z

HybiParser and WebSocketClient are mostly the same as earlier(apart from the package, which I'll be changing).

The parts I did change corresponds to the WebSocketClient#Listener interface to conform to the callback methods in the attach() method in SocketIOConnection. Should I revert those as well?

koush · 2013-07-11T20:15:20Z

WebSocket is still using separate callbacks instead of Listener. I'll leave comments inline

koush · 2013-07-11T20:16:21Z

src/com/codebutler/android_websockets/HybiParser.java

 public class HybiParser {
    private static final String TAG = "HybiParser";

-    private WebSocketClient mClient;


This rename is unnecessary, let's leave that alone.

If you want to change WebSocket internals around, we should do that in a separate pull request.

koush · 2013-07-11T20:18:29Z

Cool, almost there, just need to go back to the original websocket implementation (as much as is reasonably possible) and we can accept.

codebutler · 2013-07-11T20:19:41Z

FYI I have some patches from someone that cleans up the code a bit and removes the android dependency. codebutler#10 I'd eventually like to get this thing packaged into a proper library (as tinywebsockets or something not-android-specific) and posted on maven central so projects like this could easily depend on it.

koush · 2013-07-11T20:19:59Z

src/com/codebutler/android_websockets/WebSocketClient.java

 public class WebSocketClient {
    private static final String TAG = "WebSocketClient";

-    private URI                      mURI;


Though I'm not a fan of the tab/spacing here, let's not change it (or do it in a separate patch). Future merges with codebutler's fork would be a pain.

Try to avoid whitespace changes in general, or put them in a separate pull request so they can be reviewed as they are separate issues. Commits and pull requests should tackle one problem at a time, rather than bunch up a number of different changes.

Yeah, sorry about that. That's my Eclipse formatter. I've got into the habit of hitting Ctrl+Shift+F just before I save and it ends up realigning some stuff. I'll have to fix that.

I suspect this may have just been your Eclipse or whatever autoformatting.

koush · 2013-07-11T20:22:56Z

@codebutler Awesome, thanks.

koush · 2013-07-11T20:40:52Z

src/com/codebutler/android_websockets/WebSocketClient.java

@@ -173,7 +196,7 @@ public void send(byte[] data) {
        sendFrame(mParser.frame(data));
    }

-    public boolean isConnected() {
+    public boolean isOpen() {
        return mConnected;


The isConnected() method is unchanged from the original. Did you mean to indicate something else?

Yes, isConnected was renamed to isOpen

vinaysshenoy · 2013-07-12T16:54:54Z

@koush Done. All switched back to the original WebSocketClient/HybiParser implementation. Changes are limited to the SocketIO classes only.

koush · 2013-07-12T16:57:09Z

Perfect, thanks!

Ported AndroidAsync Style Socket.IO API to standalone library

Vinay S Shenoy added 2 commits July 9, 2013 23:26

Ported AndroidAsync SocketIO framework

ad5a9f3

Removed eventcallback addition to SocketIOClient as we need to use

92911b8

addListener()

Added wrappers around Callbacks to mimic earlier Handler interface

e706535

Removed Handler interface, Invoking callbacks on the passed Handler

58c1f5e

Repackaged HybiParser and WebSocketClient

e61b05e

koush reviewed Jul 11, 2013
View reviewed changes

Switched back to original WebsocketClient and HybiParser

7215aea

koush added a commit that referenced this pull request Jul 12, 2013

Merge pull request #26 from vinaysshenoy/v2

0e0a95f

Ported AndroidAsync Style Socket.IO API to standalone library

koush merged commit 0e0a95f into koush:master Jul 12, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ported AndroidAsync Style Socket.IO API to standalone library #26

Ported AndroidAsync Style Socket.IO API to standalone library #26

vinaysshenoy commented Jul 10, 2013

koush commented Jul 10, 2013

koush commented Jul 10, 2013

vinaysshenoy commented Jul 10, 2013

koush commented Jul 10, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

koush Jul 11, 2013

koush commented Jul 11, 2013

codebutler commented Jul 11, 2013

koush Jul 11, 2013

vinaysshenoy Jul 11, 2013

koush Jul 11, 2013

koush commented Jul 11, 2013

koush Jul 11, 2013

vinaysshenoy commented Jul 12, 2013

koush commented Jul 12, 2013

Ported AndroidAsync Style Socket.IO API to standalone library #26

Ported AndroidAsync Style Socket.IO API to standalone library #26

Conversation

vinaysshenoy commented Jul 10, 2013

koush commented Jul 10, 2013

koush commented Jul 10, 2013

vinaysshenoy commented Jul 10, 2013

koush commented Jul 10, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

koush commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

vinaysshenoy commented Jul 11, 2013

koush commented Jul 11, 2013

koush Jul 11, 2013

Choose a reason for hiding this comment

koush commented Jul 11, 2013

codebutler commented Jul 11, 2013

koush Jul 11, 2013

Choose a reason for hiding this comment

vinaysshenoy Jul 11, 2013

Choose a reason for hiding this comment

koush Jul 11, 2013

Choose a reason for hiding this comment

koush commented Jul 11, 2013

koush Jul 11, 2013

Choose a reason for hiding this comment

vinaysshenoy commented Jul 12, 2013

koush commented Jul 12, 2013