pierredavidbelanger / chatter-bot-api

A Mono/.NET, JAVA, Python and PHP chatter bot API that supports Cleverbot, JabberWacky and Pandorabots.
181 stars 72 forks source link

Networking on main thread - Android issue #33

Closed DarrienG closed 7 years ago

DarrienG commented 7 years ago

You're doing networking on the main thread with at least the Java version of Cleverbot, which means if you want to integrate this into an Android project, it will crash when being called.

I was at a hackathon the other day, and got around this by spawning a new thread, firing off an event using the greenrobot eventbus, and receiving the event on the main thread.

You can see the modified Cleverbot.java here: https://github.com/IntelliDrive/IntelliDrive-Android/blob/master/app/src/main/java/com/dglasser/intellidrive/CleverBotInterface/Cleverbot.java

And the event listener here: https://github.com/IntelliDrive/IntelliDrive-Android/blob/master/app/src/main/java/com/dglasser/intellidrive/MainActivity.java#L276

While this works, it really feels like a hack. Properly making a new thread, and then having a callback is probably the best way to do this. And even outside of Android, blocking the main thread with networking isn't very good practice.

pierredavidbelanger commented 7 years ago

Indeed, it feels like a hack, because threading should be done at the (your) application level, not in this library. This lib should not have to care about the threading use case of the caller application.

The same way you have to make HTTP requests in a background thread, you need to make chatter-bot-api calls in a background thread.

DarrienG commented 7 years ago

Now that I'm more coherent, I can actually make a recommendation.

I would still recommend making an interface that allows the user to create a "listener" for the event. In my case, where there's thread jumping, it allows for an easy access point back to the main thread, and in most cases, I'd imagine the user would want networking off the main thread anyway. Any time you're working with asynchronous you'll want a listener of some sort.

You could make this an inline interface in the base interface like this:

public interface ThoughtListener() {
    public void onThoughtCompleted(String thought);
}

Give your bots a data member: thoughtListener, and make a mutator for it.

Then instead of having think(String text) return a String, make it void, and call:

thoughtListener.onThoughtCompleted(think(thought).getText);

Then the user implements onThoughtCompleted() with something like:

bot1.setOnThoughtListener(new OnThoughtListener() {
    void onThoughtCompleted(String text) {
        // do things with text
    }    
});

This makes it easier to run it on a separate thread, which arguably all networking should be done on, and should have little impact on other users once they create the listener. For reference, this is a standard design pattern with listeners on Android.

I can pull up a PR some time later with an example some time when I get a spare couple of minutes. Worst case scenario, there's this post people working on Android or working asynchronously can see so they know how to deal with it without hacking something together like I did on an hours of sleep. :laughing:

pierredavidbelanger commented 7 years ago

The listener pattern you talk about, will not make call magically asynchronous.

Consider this actual code:

        public String think(String text) throws Exception {
            ChatterBotThought thought = new ChatterBotThought();
            thought.setText(text);
            return think(thought).getText();
        }

Then, transformed into this one you suggest:

        ...
        private ThoughtListener listener;
        public void setListener(ThoughtListener listener) { this.listener = listener; }
        ...
        public void think(String text) throws Exception {
            ChatterBotThought thought = new ChatterBotThought();
            thought.setText(text);
            listener.onThoughtCompleted(think(thought).getText());
        }

The listener.onThoughtCompleted() call will be executed on whatever thread the void think(String text) method was call on.

pierredavidbelanger commented 7 years ago

Why not just use the tools provided by Android?

I am not an Android guru, but a little search on Google, and I found you can do something like this:

public void onClick(View v) {

    // start a background thread
    new Thread(new Runnable() {
        public void run() {

            // call chatterbot on the background thread
            final String responseText = botSession.think(myRequestTextView.getText());

            // queue a runnable on the main thread
            myResponseTextView.post(new Runnable() {
                public void run() {

                    // update the ui with the chatterbot response on the main thread
                    myResponseTextView.setText(responseText);
                }
            });
        }
    }).start();
}

EDIT: Here is the link where I found inspiration for this snippet.

DarrienG commented 7 years ago

Ah, good call. Everything looks like a nail that I can just hit with a listener after working with nothing but Android all weekend.

This definitely doesn't need any changes, and I still need some sleep. Thanks for being patient with me.

pierredavidbelanger commented 7 years ago

Thanks to you for your comments and your interest in my project.

Do not hesitate if you have other questions.