Grid incorrectly overscroll's beyond last item
venkat230278 opened this issue · 19 comments
Occurs on master commit b81c1c6
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:
- Tested only for 2 columns.
- Not tested with other staggered grid features such as headers or footers.
- Might introduce other bugs.
- No guarantee / warranty of any kind. Use at your own risk.
- Might make the staggered grid less efficient.
Could be related to #21
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.
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
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.
@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.
@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.
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.
@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...
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.
Thank you for the patch venkat.
@RuiRoque Thanks for your explanation and code! As far as I understand all the code examples I can find, they either
- use random height values for testing purposes or
- use height values which are known, because the height of the corresponding content (in your case an image) is fixed or calculated by a columnswidth:imageratio combination.
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:
- A DynamicHeightImageView for the icon next to the title and the image in the middle
- A DynamicHeightTextView for the title and the "text of unknown length"
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:
- 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?
- How can I calculate the height of the "text of unknown length" before it is rendered on screen?
Thank you for your help!
In my case, @venkat230278 's patch fixed the problem. Thanks a lot!
I'm still having this issue with 1.0.5.
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.
Hey @denizmveli is there any update on when this will be addressed?
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);
}
- where ivHeight -is int height of ImageView that must be diplayed
- (int)Utils.getDimension(R.dimen.common_padding_large) - some padding to make ImageView height smaller than grid.
This helped, but not in 100% of cases. Owerscroll became more rare but it still happens. For example - if I roatate device a lot of times, and each time trying to scroll grid, in proccess of rotation and after it. Here is my adapters getView() code to make picture more clear for you. http://pastebin.com/w8UkWQtE
Please, reply - what do you think after getting this info? Is this bug connected only with view heght?
- I use the clean 1.0.5 , without patches
Can any one tell me how to use the layout in horizontal mode. Pls any can help me. Using the staggergridview library.
Take a look at the Android RecyclerView, it can go horizontal easily.
Use the RecyclerView with StaggeredGridLayoutManager
http://stackoverflow.com/questions/26860875/recyclerview-staggeredgridlayoutmanager-refrash-bug