Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Android] Fix incorrect spacing in CollectionView with multiple spans #10624

Merged
merged 12 commits into from
Dec 18, 2020
Merged

[Android] Fix incorrect spacing in CollectionView with multiple spans #10624

merged 12 commits into from
Dec 18, 2020

Conversation

ChummerUA
Copy link
Contributor

@ChummerUA ChummerUA commented May 9, 2020

Description of Change

Fixed incorrect spacing in CollectionView with multiple spans

Issues Resolved

API Changes

None

Platforms Affected

  • Android

Behavioral/Visual Changes

CollectionView doesn't add additional space to some items when layout have multiple spans anymore. Spacing now also updates correctly when ObservableCollection ItemsSource is updated.

Before/After Screenshots

Before

Screenshot_1589013087
Screenshot_1589013137
Screenshot_1589013163
Screenshot_1589013195

After
Screenshot_1589013349
Screenshot_1589013354
Screenshot_1589013406
Screenshot_1589013417

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@dnfclas
Copy link

dnfclas commented May 9, 2020

CLA assistant check
All CLA requirements met.

@ChummerUA ChummerUA changed the title Fix 9125 [Android] Fix incorrect spacing for CollectionView with multiple spans May 9, 2020
@ChummerUA ChummerUA changed the title [Android] Fix incorrect spacing for CollectionView with multiple spans [Android] Fix incorrect spacing in CollectionView with multiple spans May 9, 2020
@domagojmedo
Copy link

Why is item: 5 higher than other items in first image? Same thing with item: 4 in third image

@ChummerUA
Copy link
Contributor Author

@domagojmedo they have incorrect sizes because ItemDecoration in native renderer configured incorrectly. Also adapter need extra notifications when ItemsSource is updated in order to update ItemDecoration correctly

@domagojmedo
Copy link

@ChummerUA do you know if there is an issue for that already?

@ChummerUA
Copy link
Contributor Author

@domagojmedo I linked issues that I found into description. There's your issue and another one

@hartez
Copy link
Contributor

hartez commented Dec 9, 2020

@ChummerUA @PureWeen The API 19 test failures are happening because of a weird thing with the CarouselView's custom adapter; it does some weird things with ItemCount, so the NotifyItemRangeChanged calls are grinding to a halt. I'm looking into it.

@hartez
Copy link
Contributor

hartez commented Dec 10, 2020

Anyway, I think I need to check my solution because I noticed one issue. Spacing is calculated correctly but items on the edges have incorrect size. For example, items in first and last columns of vertical grid with 3 and more than spans will have different width than ones in inner columns

@ChummerUA @PureWeen I think I see why this is happening - the GetItemOffsets method is using GetChildAdapterPosition(view) to determine an index for the item. But the index also depends on whether there are header/footer items, so with a header in place the index is off by one and the offsets for the items on the edges are calculated wrong.

I think I see how we can fix this; the SpacingItemDecoration will need to have the SpanSizeLookup item passed in from the ItemsViewRenderer; the SpanSizeLookup. I'll implement that in the morning and see if we can fix this issue.

@ChummerUA
Copy link
Contributor Author

@hartez
It is happening because inner items being clipped equally from all sides in order to achieve correct spacing, while edge items are clipped only on the inner sides. In order to achieve both same sizes and correct spacing, horizontal offsets for each column in vertical collection/vertical offsets for each row in horizontal collection must have different values

@hartez hartez removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Dec 10, 2020
@rmarinho
Copy link
Member

@hartez the tests on Android are taking more than 6H .. i m trying to figure if it's related with changes or not, but since it happens on all android environments i have a feeling is related with the changes.

@rmarinho rmarinho added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Dec 11, 2020
@hartez
Copy link
Contributor

hartez commented Dec 12, 2020

@rmarinho I ran them locally on a device and they finished in 4 hours, so I don't think it's these changes causing the problem.

@hartez hartez added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Dec 14, 2020
@hartez hartez removed this from In Review in Shell Dec 14, 2020
@rmarinho
Copy link
Member

@hartez they keep failing on CI..

@hartez hartez removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Dec 16, 2020
@rmarinho rmarinho merged commit 208f8ea into xamarin:5.0.0 Dec 18, 2020
CollectionView automation moved this from In Review to Done Dec 18, 2020
vNext+1 (5.0.0) automation moved this from In Review to Done Dec 18, 2020
@Cr15Denis
Copy link

similar problem #13044

@mhrastegari
Copy link
Contributor

we still have problem with this! when we update span count or device orientation the items size(Height) won't update correctly in Griditemlayout

@jfversluis jfversluis mentioned this pull request Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.