Cache cell sizes after resize from estimates

[hartez]

Description of Change

CollectionView on iOS utilizes the UICollectionViewFlowLayout's EstimatedItemSize property to allow virtualization; for items which have not yet been realized, the EstimatedItemSize is used for calculating properties like the UICollectionView's content size (for scrollbars, etc).

When the layout is invalidated, this size is also used for newly inserted items (which have not yet had their actual sizes determined by a cell) and for any items invalidated by insertion/removal. For single DataTemplate scenarios, this is generally fine, since the estimated size calculated from the first item is likely to be close to or exactly the size of subsequent items.

These changes now cache the size of each cell as they are updated from the initial estimated size, and reuses the cached values during future operations.

Issues Resolved

• fixes [Bug] [iOS] CollectionView - Invalid Height Measure when using different datatemplates #10842
• fixes [Bug] [iOS] CollectionView with MeasureAllItems broken Animations & Scroll #10625

API Changes

None

Platforms Affected

• iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Control Gallery -> CollectionView Gallery -> DataTemplate Galleries -> Varied Size Data Templates

Add/Insert/Remove items - the animation should be smooth, and no items other than the ones being inserted/removed should change size.

Control Gallery -> CollectionView Gallery -> ItemSize Galleries -> Chat Example

Add items - the animation should be smooth, and items should not jump around

PR Checklist

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

[Tommigun1980]

Thank you @hartez -- I believe this will fix a ton of the CollectionView issues I've been running into and am really looking forward to this PR getting merged in!

Reading the description it sounds like it may fix at least these also: #12675 #12622 #12697 #12555 #12722

When do you estimate this PR might be merged so I could try a nightly build?
Thank you.

[hartez]

Assuming all the tests pass, I'm hoping we can get it reviewed, merged, and into nightly builds next week.

[rmarinho]

@Tommigun1980 you can also grab the nuget for this PR from here:

https://dev.azure.com/xamarin/public/_build/results?buildId=31022&view=artifacts&pathAsName=false&type=publishedArtifacts

[rmarinho]

Failed iOS test is not related

[Tommigun1980]

Hi! I tested the referenced NuGet but unfortunately it hard crashes with the following when its data is populated:

2020-11-23 23:46:37.228338+0200 MyApp.iOS[3762:246719] [CollectionView] An attempt to prepare a layout while a prepareLayout call was already in progress (i.e. reentrant call) has been ignored. Please file a bug. UICollectionView instance is (<UICollectionView: 0x7fa9a26d9200; frame = (0 0; 362 543.333); clipsToBounds = YES; autoresize = W+H; gestureRecognizers = <NSArray: 0x600000692df0>; layer = <CALayer: 0x60000000c4c0>; contentOffset: {0, -10}; contentSize: {362, 0}; adjustedContentInset: {10, 0, 0, 0}; layout: <Xamarin_Forms_Platform_iOS_ListViewLayout: 0x7fa9a325ea90>; dataSource: <Xamarin_Forms_Platform_iOS_GroupableItemsViewController_1: 0x7fa9a325f160>>)
>>>>>> TemplatedCell constructor

EDIT: It was because I had accidentally left in ViewCell declarations in the data templates (as I was using a ListView earlier). The error message says "Please file a bug" so I'll leave this comment here. Maybe the error message could be more descriptive of what went wrong instead of saying to file a bug, as if you have a lot of XAML it's easy to accidentally leave in some ViewCells in the data templates when converting ListViews to CollectionViews.

[Tommigun1980]

@hartez @rmarinho @jsuarezruiz

Ok I completed my testing and unfortunately it didn't fix the main issues I've been having with collection views, and introduced two regressions.

Please see the following video:
https://drive.google.com/file/d/1YA1pCoPdFhgR9r5riNNYw7EESyVza4XW/view?usp=sharing

1. When new elements are added to the collection, they get arbitrary spacing between the elements. All of the 'speech bubbles' should have a few pixels of space between them. This I have already reported and is not a new issue. This can easily be seen at for example 0:03 into the video.

2. When new elements are added to the collection it animates them in at arbitrary positions. Look at for example the first bubble I add to the list at 0:00 - it makes all the existing items jump around wildly. This I have already reported and is not a new issue.

3. Regression: at for example 0:19 into the video you can see two notification template items visible, "Jorma created the activity" etc. At 0:20 into the video a speech bubble template item is added, but as you can see it duplicates an existing notification template item. So now the new speech bubble item is cutting into a phantom notification template item. This is especially visible at 0:50 into the video where the speech bubbles and notifications overlap completely. This didn't use to happen with the collection view earlier and is imho a catastrophic error.

4. Regression: At for example 0:28 into the video I start adding elements where their heights are different (multiple lines per speech bubble) - the collection view completely cuts them off to be the same height as the one liners. This definitely did not use to happen with CollectionViews earlier and I think this to be a catastrophic error too.
   (The newly added element is visible at its correct height for a fraction of a second [albeit at an incorrect position due to issue number 1], for example at 0:36 into the video, until the animation starts shaking things up and ultimately it gets rendered with a completely incorrect height.)

I am more than willing to offer my help with these issues as I really have to get the collection view working. I currently am using a ListView with a ton of timing based hacks, but the more entries there are in the list the more time I have to add to delays to get it to work, so it is not a permanent fix.

I'd urge not to merge this in yet, and as I said I'd be more than willing to spend time on getting this right as I am dependent on this starting to work (testing, repro cases, whatever you need).

Thank you.

Disclaimer: The photos in the videos are images I picked randomly from the web for my personal testing only, and are not real users.

[Tommigun1980]

@hartez @rmarinho @jsuarezruiz

For posterity, here is a video of the same process performed on a collection view without this PR:

https://drive.google.com/file/d/12FCvJHwiizfLIVbP7FNRFRyxVxOyUZbl/view?usp=sharing

Some notes:

1. The issues 1) and 2) are there, but not 3) and 4) which are regressions introduced in this PR.

2. As can be seen in the video 0:20 until its end, the previous version of the CollectionView seems to not display any errors until elements of different heights are added. I strongly believe the root cause to all these issues being that when there is even one element with a height different than the others it breaks down and starts animating, spacing and drawing items incorrectly. As soon as the element with a different height is added at 0:28 the collection view starts exposing issues 1) and 2) after additional elements are added. So the issues don't happen when the last element is of a different height, but it affects the positions, animations and spacing of all elements after it.

Maybe there is an incorrect calculation in the collection view that assumes that all elements have a uniform height, even when set to measure all items? It truly looks like it in all the testing I've done.
Furthermore, this PR seems to lock down a specific height on data templates, thus removing the ability to have arbitrary heights on the elements (see regression issue 4).
Tbh I fail to see how the height of a template could be cached, as the heights can differ between item instances no matter what template they used (for example the speech bubbles with multiple lines in my videos), so I don't understand how this PR could work in any other mode than MeasureFirstItem?

I know this is a lot of text but I think it'd be valuable for someone to read through it and check out the time stamps in the videos I posted, as these issues must be tackled at some point.
Thank you.

[Tommigun1980]

I tagged you also @jsuarezruiz as you've been looking into these issues and here's some additional information, so I hope you don't mind!

[modplug]

@Tommigun1980 I would like you thank you for putting in such an effort to get this working. I've been trying to make a decent chat with XF for many years now, but I've had to fall back to time-based hacks with ListView as you are describing. To the XF team I would like to say that I know such an abstraction as the CollectionView is hard to get right, and I do really appreciate all the work you've put into it but it's still missing a complete sample which would highlight all the issues. Make an example chat with it. Something that mimicks whatsapp, facebook messenger and the like. To everyone involved, keep up the good work!

[hartez]

@Tommigun1980 How are you adding new items to you CollectionView? Is it just an ObservableCollection that you're appending to?

[Tommigun1980]

@Tommigun1980 How are you adding new items to you CollectionView? Is it just an ObservableCollection that you're appending to?

Hi @hartez. That is correct, it's just a regular Add:

var chatMessageEx = (await this.ModelToModelExService.ToChatMessageEx(new List<ChatMessageDTO>() { args.ChatMessage }, new List<ChatRoomDTO>() { args.ChatRoom }, this.GetCancellationToken())).Single(); // enrich chat message DTO for XAML side

this.ChatMessages.Add(chatMessageEx); // add it to the collection

The collection in question is an OptimizedObservableCollection (https://github.com/xamarinhq/xamu-infrastructure/blob/598f97d6f5cc68fc603873e913ccc9089dbc60df/src/XamU.Core/Collections/OptimizedObservableCollection.cs), but the Add() is not overridden.

[Tommigun1980]

Hi. I was wondering what the status is of this atm? I am extremely dependent on the collection view starting to work (or even the list view) for the chat portion of my app, where the collection/list views contain items of different sizes.

Did the videos I provided showcase the problems? Is there anything else I can do?

Thank you!

[Tommigun1980]

Hi again.

I need to know what will happen to the CollectionView so any update would be greatly appreciated. Did the videos make sense or is there some clarification I can do on the issues?
Thanks so much.

[hartez]

Hi again.

I need to know what will happen to the CollectionView so any update would be greatly appreciated. Did the videos make sense or is there some clarification I can do on the issues?
Thanks so much.

Just pushed an update to this PR which I think will address the problems you're seeing. Rather than caching template sizes, we're caching individual cell sizes. I created a "chat" test page with randomly sized text messages to reproduce the specific issues you are concerned about, and right now it can add new messages without the weird jumping around and resizing we were seeing before.

It's running through the automated tests right now, so I should know Monday if I missed anything major. Assuming all goes well, we've still got a pretty good shot at this making it in the the 5.0 stable release. If we somehow miss that, then it will be in 5.1.

[Tommigun1980]

Hi again.
I need to know what will happen to the CollectionView so any update would be greatly appreciated. Did the videos make sense or is there some clarification I can do on the issues?
Thanks so much.

Just pushed an update to this PR which I think will address the problems you're seeing. Rather than caching template sizes, we're caching individual cell sizes. I created a "chat" test page with randomly sized text messages to reproduce the specific issues you are concerned about, and right now it can add new messages without the weird jumping around and resizing we were seeing before.

It's running through the automated tests right now, so I should know Monday if I missed anything major. Assuming all goes well, we've still got a pretty good shot at this making it in the the 5.0 stable release. If we somehow miss that, then it will be in 5.1.

THANK YOU! Really looking forward to testing it.

[Tommigun1980]

Hi again.
I need to know what will happen to the CollectionView so any update would be greatly appreciated. Did the videos make sense or is there some clarification I can do on the issues?
Thanks so much.

Just pushed an update to this PR which I think will address the problems you're seeing. Rather than caching template sizes, we're caching individual cell sizes. I created a "chat" test page with randomly sized text messages to reproduce the specific issues you are concerned about, and right now it can add new messages without the weird jumping around and resizing we were seeing before.

It's running through the automated tests right now, so I should know Monday if I missed anything major. Assuming all goes well, we've still got a pretty good shot at this making it in the the 5.0 stable release. If we somehow miss that, then it will be in 5.1.

Oh and let me know if there’s a way to test this early so I can run it through my tests also in case you want another test pass at it.
I’ll try to find the time to extract those portions to a separate test in case something still doesn’t work.

[rmarinho]

@Tommigun1980 you can get the nupkg to test this changes from here:

https://dev.azure.com/xamarin/public/_build/results?buildId=32817&view=artifacts&pathAsName=false&type=publishedArtifacts

bug here you go
debug (15).zip

[Tommigun1980]

@rmarinho @hartez

I tested with the linked build and it's a lot better than it was but unfortunately there's still some issues.

Please see the following video:
https://drive.google.com/file/d/1X_uZwS_PAApg7iR5epL1VjJufOpozKfz/view?usp=sharing

Timestamps
0:13: The second to last line, "Asdfasdfasdfasdf" is cropped to one row only, while in reality it has multiple lines. You can see part of the second line there.
0:20: I click the chat area (that on a real device would push content upwards to allow space for the on-screen keyboard), which magically corrects the issue. Now the affected item is correctly drawn as 5 lines (the last line is deliberately an empty line).

So the issue happens less, but is definitely still there. This is still a regression that this PR would introduce, but this PR also fixes almost all of the other issues, so it's very very close!! I still think it'd be a good idea to fix this before merging it in as the issue, when it happens, is quite a serious one.

Other less fatal issues visible in the video:

• I am using ItemsUpdatingScrollMode="KeepLastItemInView", but as you can see one second into the video the view does not jump to the last message when content is populated (OptimizedObservableCollection + AddRange) - KeepLastItemInView seems to be ignored when data is populated (I need it to jump to the last message after data has been loaded).
• 0:28: I add a new item, but it leaves a lot of empty space after it. If I scroll the view it 'jumps' into place and looks correct, but this requires the 'tug'.

Also, this may not be related to this but putting it here in case it is as it's also pretty fatal: #13126 is a new issue that started to happen in 5.0.0-pre5 (didn't happen in pre4), where CollectionView contents are empty the first time they are shown. Here is a direct link to a video of it as well:
https://drive.google.com/file/d/1ZY2lJ4n7vWh0tTUdxm6on3G7wIdhstcM/view?usp=sharing

Thanks so much for working on these issues. The CollectionView is so close now!!

PS. The test users in the test app are random images I found on the net and not real users of any kind.

[hartez]

0:13: The second to last line, "Asdfasdfasdfasdf" is cropped to one row only, while in reality it has multiple lines. You can see part of the second line there.

I can't get this problem to reproduce, but I might be using a different combination of controls. What's the ItemTemplate you are using in your video? Can you share the XAML?

[Tommigun1980]

Hi. I’ll provide the XAML soon, but of note is that it happened in only ONE of all the elements I tested with. I almost missed it myself as the other speech bubbles seemed to work, and I can’t say what’s special with that one (except maybe it ending with a blank line, I’m not at a computer atm so can’t test it). I tested maybe 30 different speech bubble contents and for some reason that particular one exposes the problem, and it corrects itself when the CollectionView is re-layouted (as seen by invoking what would be the on-screen keyboard on a physical device). The way it corrects itself is slightly similar to how the ‘keep scrolled to bottom’ setting corrects itself after scrolling the contents, also visible in the video. I’ll get back with more info later. Thank you.

…

On Monday, December 21, 2020, E.Z. Hart ***@***.***> wrote: 0:13: The second to last line, "Asdfasdfasdfasdf" is cropped to one row only, while in reality it has multiple lines. You can see part of the second line there. I can't get this problem to reproduce, but I might be using a different combination of controls. What's the ItemTemplate you are using in your video? Can you share the XAML? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12919 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOL6P6GWVSOOSVFARELJCRLSV6FARANCNFSM4T35LRYA> .

--

[Tommigun1980]

0:13: The second to last line, "Asdfasdfasdfasdf" is cropped to one row only, while in reality it has multiple lines. You can see part of the second line there.

I can't get this problem to reproduce, but I might be using a different combination of controls. What's the ItemTemplate you are using in your video? Can you share the XAML?

@hartez I can not share the exact XAML but I made a very close representation repro project, which is available at #13231.

[rmarinho]

Failing test not related

[PureWeen]

Looks good. Logged a separate issue here #13251