Is this Android click handler using threads correctly?

I am implementing a thread like the following. Is my implementation correct?

It works well, but I need a check.

```
myList.setOnItemClickListener(new OnItemClickListener() {

@Override
public void onItemClick(AdapterView parent, final View view,
int position, long id) {
// TODO Auto-generated method stub
Runnable runnable = new Runnable() {

@Override
public void run() {
// TODO Auto-generated method stub
Looper.prepare();
Context context = view.getContext();

final String titleItem = (String) ((TextView) view.findViewById(R.id.titleItem)).getText();
final String descItem = (String) ((TextView) view.findViewById(R.id.descItem)).getText();
final Bitmap bitmap = (Bitmap) ((BitmapDrawable) ((ImageView) view.findViewById(R.id.imgItem)).getDrawable()).getBitmap();

Toast.makeText(context, "Item Title: " + titleItem + ", Item Desc: " + descItem, Toast.LENGTH_SHORT).show();
myHandler.post(new Runnable() {

@Override
public void run() {
// TODO Auto-generated method stub

Intent intent = new Intent(getApplicationContext(), ItemDetails.class);

ByteArrayOutputStream stream = new ByteArrayOutputStream();
bitmap.compress(Bitmap.CompressFormat.JPEG, 100, stream);
byte[] bytes = stream.toByteArray();

intent.putExtra("title", titleItem);
intent.putExtra("BMP",bytes);
intent.putExtra("desc", descItem);

Log.i("ON ITEM CLICK", "OK");
startActivity(intent);
}
});
Looper.loop();
}
};
new Thread(runn

@ratchetfreak's comment is correct, expanding on it here a bit:

AsyncTask is the class to use (or rather, extend and use your subclass-implementation, as AsyncTask itself is an abstract class).

You should use AsyncTask for performing operations that otherwise would block the UI-thread and thus would make your application not respond.

However, let's take a look at what your code actually does:

• Creates a runnable

• Gives the runnable a View, from which it takes a Context, two TextViews (and their Strings), and a Bitmap from an ImageView.

• Then it creates and shows a toast - this should be done on the UI-thread and not in a background thread

• Posts a runnable to your myHandler, therefore making the following code run on your UI-thread:

• Create an intent

• Compress a bitmap into JPEG

• Puts some data into the intent

• Starts a new activity

• After the runnable has been created and a new thread started, you call a method that seems to want to stop your current thread!? Why on earth would you want to do that?? Let Android take care of the thread handling! That thread is not yours to stop!

Why do you create these threads?

The longest running operation that you perform in these threads is the Compress a bitmap into JPEG, which you run on the UI-thread anyway. The few things that you do in background is so fast that you really don't need to create a new thread for it.

If your Compress a bitmap into JPEG operation takes much time, then I would suggest you create an AsyncTask to handle that. (Bitmap is the input, it does not have any progress update and therefore it says Void, and it returns a byte[] which you will send to the next activity). I don't think you need any AsyncTask at all here though.

Your new code:

With the above comments taken into consideration, here's the code you need. No extra threads. If this code causes your app to stop respond for a longer while than what is comfortable, then you can consider an AsyncTask.

myList.setOnItemClickListener(new OnItemClickListener() {

    @Override
    public void onItemClick(AdapterView parent, final View view,
            int position, long id) {
        Context context = view.getContext();
        String titleItem = (String) ((TextView) view.findViewById(R.id.titleItem)).getText();
        String descItem = (String) ((TextView) view.findViewById(R.id.descItem)).getText();
        Bitmap bitmap = (Bitmap) ((BitmapDrawable) ((ImageView) view.findViewById(R.id.imgItem)).getDrawable()).getBitmap();
        Toast.makeText(context, "Item Title: " + titleItem + ", Item Desc: " + descItem, Toast.LENGTH_SHORT).show();

        Intent intent = new Intent(getApplicationContext(), ItemDetails.class);
        ByteArrayOutputStream stream = new ByteArrayOutputStream();
        bitmap.compress(Bitmap.CompressFormat.JPEG, 100, stream);
        byte[] bytes = stream.toByteArray(); 

        intent.putExtra("title", titleItem);
        intent.putExtra("BMP",bytes);
        intent.putExtra("desc", descItem);
        Log.i("ON ITEM CLICK", "OK");
        startActivity(intent);
    }
});

Normal usage of AsyncTask

Whenever you have a longer running computational-intensive operation, then you can consider an AsyncTask. Also whenever you do network operations (download a file from internet, create a long-running socket-connection, etc.), then you need an AsyncTask. As said already, I don't think your current code needs/benefits from an AsyncTask.

Consider, however...

Your ImageView known as view.findViewById(R.id.imgItem) must have gotten its drawable from somewhere. Is it loaded from a file on the external storage? Is it loaded from a resource? Rather than compressing the drawable's bitmap data and sending the byte array, consider informing your new activity about where it can find the original data. If it's a file on the external storage, pass along a String for the path it is found in. If it's a resource in your application, pass along the resource int identifier.

If the drawable is loaded from the internet or dynamically created, then I agree about compressing it as JPEG and sending the byte array to the new activity.

StackExchange Code Review Q#49317, answer score: 6