search

etsy / AndroidStaggeredGrid

An Android staggered grid view which supports multiple columns with rows of varying sizes.
https://github.com/etsy/AndroidStaggeredGrid
4.76k stars 1.13k forks source link

Grid incorrectly overscroll's beyond last item #66

Open venkat230278 opened 10 years ago

venkat230278 commented 10 years ago

Occurs on master commit b81c1c6109cb86afb06ef648e989665915e09b88

Video can be found here: http://dai.ly/x1dj5iy

Test App to reproduce is at https://github.com/venkat230278/AndroidStaggeredGridViewOverscrollBug.git

Place app at same level as "library", "sample", import into Android studio and build.

Test App has a StaggeredGridView with 2 columns. Its adapter supplies 3 text views. Eventually, the three text views are laid out by the GridView as below.

A B C

All text views (A, B, C) are of equal height H. H is greater than screen height. In above layout, A and B are clipped by the screen.

Slowly start scrolling towards and beyond C. Note that it is possible to scroll beyond C. Scroll in reverse towards A. Notice that B has disappeared. Notice after a while that A too disappears.

This issue is also encountered if the user flings on A or B towards C.

It appears that the grid view considers empty columns in scroll math. We tried to put in a quick hack fix for this issue for our product.

Patch can be found here: https://github.com/venkat230278/AndroidStaggeredGridViewOverscrollBug/blob/master/patch/sg.patch

Apply patch as follows:

[Some dir]/AndroidStaggeredGrid/library/src/main$ patch -p1 < sg.patch

Things to note about the above patch:

  1. Tested only for 2 columns.
  2. Not tested with other staggered grid features such as headers or footers.
  3. Might introduce other bugs.
  4. No guarantee / warranty of any kind. Use at your own risk.
  5. Might make the staggered grid less efficient.
venkat230278 commented 10 years ago

Could be related to https://github.com/etsy/AndroidStaggeredGrid/issues/21

denizmveli commented 10 years ago

Thank you for a very detailed issue description. I'll be working on this and other small set issues for the next release 1.0.5.

jbgill commented 10 years ago

I applied @venkat230278 's patch to 1.0.4 and it improves matters, but doesn't solve it entirely. I still see issues with the views disappearing and other strangeness when scrolling to the bottom of the grid and back up - but only when I have items that are taller than the grid itself - like in landscape orientation

venkat230278 commented 10 years ago

@jbgill

I would like to look into the problem you are experiencing after applying the patch. If possible, do you have a minimal app that reproduces the issue? Thanks.

jbgill commented 10 years ago

@venkat230278 Unfortunately, I don't have anything minimal I can send you right now. I'm attempting to put this view into a large app with dynamic content from our servers. If I get some time soon I'll see if I can trim it down to a small sample with some hard-coded content. The over-scrolling seems to be fixed, but I still see cases where if I scroll past a tall item, then back up, I get views in other columns disappearing and sometimes I have to scroll back up through the tall item twice before seeing the items above it. My failing examples have been 3 columns with maybe a dozen or so items of varying heights with one or two items that are taller than the grid view.

muetzenflo commented 10 years ago

@venkat230278 Hi, I just tried version 1.0.4 and faced the problem described here. I put together a small demo app for debugging: https://content.wuala.com/contents/muetzenflo/Sonstiges/Public/StaggeredGridViewDemo.zip/?dl=1

It starts in landscape mode. If you fling fast through the list and try to get back to the top some strange things happen. Sometimes it goes all the way up, but only with 1 column instead of 2, sometimes the listItems disappear. I tried it on an HTC One mini. Maybe you need to make the hard-coded Strings a little bit longer to increase the height of each listItem.

RuiRoque commented 10 years ago

I had the same problems, even after applying the fix by venkat230278. Disappearing columns were a common issue.

So, instead of diving into the source code and get my hands dirty, I started to think what were we all doing different that Etsy isn't in their app. And I think I found out. I no longer have any problems with this issue.

There are two classes in the library: com.etsy.android.grid.util.DynamicHeightTextView com.etsy.android.grid.util.DynamicHeightImageView

They use these in the sample app, so you can see their usage. Basically, each class sets the height of the View in the onMeasure method. I was already using one of these classes in my app for the ImageView, but it was inside a LinearLayout with some other TextViews on the bottom. With this setup, I was experiencing the reported problems. But when I removed the simple TextViews and had only my custom programmatic resizable view inside a FrameLayout, it worked perfectly. I assume that the views in the adapter item must all be "pre-measurable", and that dynamic layout properties like wrap_content and match_parent doesn't work very well. Hope this helps.

muetzenflo commented 10 years ago

@RuiRoque are you sure about this? Did you try it with list items really higher than the available screen size? I just updated my DemoProject and each list item uses this simple layout:

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout
    xmlns:android="http://schemas.android.com/apk/res/android"
    android:orientation="vertical"
    android:layout_width="match_parent"
    android:layout_height="wrap_content">

<com.muetzenflo.sandbox.DynamicHeightTextView
    android:id="@+id/text"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:textSize="20sp"
    android:padding="10dp"
    />

</LinearLayout>

But I still cannot scroll the list back up from bottom to top without losing elements and/or having only one column filled and one empty. Would you mind taking a look at my demo? It's very simple, just an activity and the adapter.

https://content.wuala.com/contents/muetzenflo/Sonstiges/Public/StaggeredGridViewDemo.zip/?dl=1

Thanks for helping us here!

PS: If it really were the case that EVERY child of a list item has to be pre-measurable it would not be so cool, since I am using the official YouTubeThumbnailView from the YouTubePlayer API. This ThumbnailView is a final class and therefore cannot be overridden...

https://developers.google.com/youtube/android/player/reference/com/google/android/youtube/player/YouTubeThumbnailView?hl=ko

RuiRoque commented 10 years ago

Yes, this worked for me.

You are using the DynamicHeightTextView, but still not setting the height in the Adapter. The DynamicHeightTextView uses a ratio to determine the height, which was not very handy for me. So, I build my own DynamicHeightTextView (in my case, for an ImageView) to set the height and not the ratio:

public class DynamicHeightTextView extends TextView {

private int imageHeight;

public DynamicHeightTextView(Context context) {
    super(context);
}

public DynamicHeightTextView(Context context, AttributeSet attrs) {
    super(context, attrs);
}

public DynamicHeightTextView(Context context, AttributeSet attrs, int defStyle) {
    super(context, attrs, defStyle);
}

public void setHeight(int height) {
    if (imageHeight != height) {
        imageHeight = height;
        requestLayout();
    }
}

@Override
protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
    if (imageHeight > 0) {
        int width = MeasureSpec.getSize(widthMeasureSpec);
        setMeasuredDimension(width, imageHeight);
    } else {
        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
    }
}

}

With this, in the adapter, try to set the height. Something like:

holder.text.setHeight(someHeightInPixels);

And also: holder.text.getLayoutParams().width = maximumImageWidth; holder.text.getLayoutParams().height = someHeightInPixels;

The maximumImageWidth is something like: maximumImageWidth = (maximumScreenWidthAccordingToOrientation / numberOfRows) - (staggeredGridViewMargin * numberOfRows);

And for the someHeightInPixels, build some random generator to get different values.

Frank1234 commented 10 years ago

Thank you for the patch venkat.

muetzenflo commented 10 years ago

@RuiRoque Thanks for your explanation and code! As far as I understand all the code examples I can find, they either

None of this is applicable for me. My list items look like this:

-----------------
| X | Title     |
-----------------
|               |
|   Image       |
|               |
-----------------
| Text of       |
| unknown       |
| length        |
-----------------

So, am I right when I understand it that I have to create:

These DynamicHeightXXViews both have a height value that I set in the getView()-method of my StaggeredGridViewAdapter. And to set this height value I have to calculate the exact height of each DynamicHeightXXXView?

If yes, this would bring two new question for me:

  1. The image under the title is a YouTubeThumbnailView which is a final class and can therefore not be extended or modified. How would I handle this? Maybe an invisible fake View above the YouTubeThumbnailView with the same height?
  2. How can I calculate the height of the "text of unknown length" before it is rendered on screen?

Thank you for your help!

wu32 commented 10 years ago

In my case, @venkat230278 's patch fixed the problem. Thanks a lot!

dalton-miller commented 10 years ago

I'm still having this issue with 1.0.5.

aenain commented 10 years ago

In my case, neither using only DymanicHeightXXViews nor @venkat230278 's patch did help, but gave me an idea how to deal with it. I am using version 1.0.5. I need to make it work for two columns, but it can be easily adjusted if you need to support more.

My approach is completely different and I decided to fix it using an invisible placeholder which is added when scrolled to last element if there is too big height difference between columns (more than height of the grid view, so one column has all of the views recycled). Maybe someone will find it useful :)

https://gist.github.com/aenain/8a3efdf7f96fff0d6807

Solution consists of two main elements: StaggeredGridViewProxy and GridHeightPlaceholder.

Former delegates methods I use to an underlying instance of StaggeredGridView and exposes mColumnBottoms private variable using reflection. Not the perfect solution, but this way I don't have to add etsy's library source code to my project and it won't break if the variable is renamed or something (the fix won't be used then). Exposing this variable in 1.0.6 would be appreciated though ;)

Latter listens to scroll and displays placeholder at the bottom of column if needed. Placeholder item is displayed using the same layout as other items but it sets its inner container's alpha to 0. It cannot be applied to item's container, because there is a guard in GridLayoutParams which automatically sets layout params. If you want to reuse view which served as a placeholder while scrolling back to top, make sure to set alpha back to 1 and layout height to ViewGroup.LayoutParams.WRAP_CONTENT of content view.

Note that I set grid's item margin using dimen attribute.

This solution supports adding & removing elements using adapter.

lawloretienne commented 9 years ago

Hey @denizmveli is there any update on when this will be addressed?

Truerall commented 9 years ago

Hi. I whant to add some info about this bug. I've read this thread, and decided to fix it using next workaround. As far as problem appears only in case when childView has height more than gridHeight, I ve added such code to my getView();

if (ivHeight >= parent.getHeight() ) //TODO remove after GridView lib new release { ivHeight = parent.getHeight() - (int)Utils.getDimension(R.dimen.common_padding_large); }

Please, reply - what do you think after getting this info? Is this bug connected only with view heght?

vinod1988 commented 9 years ago

Can any one tell me how to use the layout in horizontal mode. Pls any can help me. Using the staggergridview library.

Frank1234 commented 9 years ago

Take a look at the Android RecyclerView, it can go horizontal easily.

pseveryn commented 9 years ago

Use the RecyclerView with StaggeredGridLayoutManager http://stackoverflow.com/questions/26860875/recyclerview-staggeredgridlayoutmanager-refrash-bug