5

I'm using a GridView to show some images and I had a problem, as the onClickListener wasn't working for the first image. I found some other questions here at SO with the same problem, but I don't like their "correct answers", as most of them take the same approach of:

Basically, instantiating the view every time getview is called. This is awful for performance and they will probably face out-of-memory issues in many devices.

In my case, I display in the GridView the images located inside a sub-folder in the assets folder.

My original code with the "first item" issue (actually, my original code implemented the viewholder pattern, but this one is a bit simpler and faces the same issue):

    @Override
public View getView(int position, View convertView, ViewGroup parent) {
    ImageView imageView;

    if (convertView == null) {
        imageView = new ImageView(_activity);

    } else {
        imageView = (ImageView) convertView;
    }

    // get screen dimensions
    AssetManager assetManager = _activity.getAssets();
    InputStream assetIn = null;
    try {
        assetIn = assetManager.open(_assets_subdir + File.separator + _filePaths.get(position));
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    Bitmap image = BitmapFactory.decodeStream(assetIn);
    imageView.setScaleType(ImageView.ScaleType.CENTER_CROP);
    imageView.setLayoutParams(new GridView.LayoutParams(imageWidth, imageWidth));
    imageView.setImageBitmap(image);

    // image view click listener
    imageView.setOnClickListener(new OnImageClickListener(position));

    return imageView;

}

My final code solving the issue:

    @Override
public View getView(int position, View convertView, ViewGroup parent) {
    ImageView imageView;

    if (convertView == null) {
        imageView = new ImageView(_activity);
        imageView.setLayoutParams(new GridView.LayoutParams(imageWidth, imageWidth));

    } else {
        imageView = (ImageView) convertView;
    }

    // get screen dimensions
    AssetManager assetManager = _activity.getAssets();
    InputStream assetIn = null;
    try {
        assetIn = assetManager.open(_assets_subdir + File.separator + _filePaths.get(position));
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    Bitmap image = BitmapFactory.decodeStream(assetIn);
    imageView.setScaleType(ImageView.ScaleType.CENTER_CROP);
    imageView.setImageBitmap(image);

    // image view click listener
    imageView.setOnClickListener(new OnImageClickListener(position));

    return imageView;

}

The issue was solved moving the code imageView.setLayoutParams(new GridView.LayoutParams(imageWidth, imageWidth));.

But why? I'm not sure.

I read somewhere (in SO) that it could be happening because of trying to access a view that has not been inflated yet, and the user recommended using getViewTreeObserver(), but I tried that approach and couldn't fix the problem.

So, I decided to trial-and-error the code to see where to bottleneck could be and found the given solution.

Anyone knows why is this solving the problem?

Community
  • 1
  • 1
Alejandro Colorado
  • 6,034
  • 2
  • 28
  • 39
  • This could be of some help [link] [1]: http://stackoverflow.com/questions/11778228/onclicklistener-not-working-for-first-item-in-gridview – Anish Dec 07 '13 at 06:26
  • Thanks @anish but that is an example of the (IMHO) wrong fixes I mention in the question. His fix is, literally, "ended up fixing it by just instatiating the holder in every pass, instead of getting it by tag(which is better for performance, but oh well)." `getView()`can be called multiple times and he is bypassing the usefulness of `convertView` by inflating the layout and calling `findViewById` two times EVERY time `getView()` is called. Using images in the GridView will probably lead to OutOfMemory crashes. – Alejandro Colorado Dec 07 '13 at 10:20
  • The likely reason adding LayoutParams is resolving your problem, is that it's triggering another layout pass. This seems to suggest that your view is being reused elsewhere, and its OnClickListener is being replaced. I recall having run into this odd behavior myself, but don't quite remember how I worked around it. I'll see if I can figure out which project it was related to. – Paul Lammertsma Dec 13 '13 at 10:29
  • OK thanks! I would appreciate that extra info. – Alejandro Colorado Dec 13 '13 at 22:44
  • Just in case, is it normal this: `GridView.LayoutParams(imageWidth, imageWidth)`? It should be: `GridView.LayoutParams(imageWidth, **imageHeight**)` or it's just a copy/paste error? – Blo Dec 18 '13 at 14:02
  • Using the imageWidth in place of imageHeight is probably to enforce a square image view – FunkTheMonk Dec 18 '13 at 18:33

2 Answers2

1

GridView extends from AbsListView -> GridView.LayoutParams is actually AbsListView.LayoutParams, which includes some state data about the view's type and whether or not it is recycled.

By initialising a new set of layout params each time getView is called (and not just when the view is created anew), you were losing this state data.

I'm not (currently) sure why it was only causing an issue for the first item in the grid.

FunkTheMonk
  • 10,908
  • 1
  • 31
  • 37
0

You only have an ImageView in your GridView ? Why don't you use setOnItemClickListener instead of OnItemClickListener ?

Like this, in your Activity : 

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    // ...
    gridView.setOnItemClickListener(new OnItemClickListener() {
            @Override
            public void onItemClick(AdapterView<?> parent, View v,
                    int position, long id) {
                    // here you have what you want ...
            }
    });
}
Blo
  • 11,903
  • 5
  • 45
  • 99
Andros
  • 4,069
  • 1
  • 22
  • 30
  • Sorry, I didn't understand you: do you mean using setOnItemClickListener instead of setOnClickListener? – Alejandro Colorado Dec 17 '13 at 23:39
  • By settings the imageView to be clickable rather than setOnItemClickListener, there are potentially areas around the imageView which doesn't respond to touch events – FunkTheMonk Dec 18 '13 at 19:02
  • Well, as @FunkTheMonk says, I actually wanted the whole area to respond, although it may not make too much a difference. – Alejandro Colorado Dec 19 '13 at 00:37
  • If you want the whole area (including space created by a center gravity) to respond, rather than just the imageView, you should use the onItemClickListener – FunkTheMonk Dec 19 '13 at 08:23