Don't Eagerly Trigger Relayout in Wait

[Adlai-Holler]

This was added in #3017 and it's causing some issues for Pinterest, due to it triggering the initial load of data for collections that are (0, 0). @nguyenhuy Do you remember the details of why this was added?

[Adlai-Holler]

I haven't run tests locally so they may fail. I will adjust them later.

[Adlai-Holler]

I've modified the tests to add the manual layoutIfNeeded call.

The context here is:

• Before the data controller refactor
  • Always did "data loads" by calling performBatchUpdates
  • Immediately causes an animated update & UIKit requery.
  • Range controller gets updated.
  • Batch fetching gets checked.
• After the refactor
  • We do data loading using native reloadData
  • Much faster and better
  • Does not trigger any immediate update or data load
  • Need to simulate next layout pass to get range controller & batch fetching updated.
  • Having layoutIfNeeded in waitUntilUpdatesAreCommitted causes issues in practice
  • Forces data load & remeasure at size (0, 0)
  • Could try to be smarter and sometimes trigger the layout pass
  • For now prefer simplicity. Can't think of many use cases for forcing UIKit to update in this way in practice.

[appleguy]

@Adlai-Holler I have also had issues with 0, 0 layouts.

It does seem important though that this method behaves consistently, e.g. a user should not have to know if the layout pass has occurred for waitUntilAllOperationsAreCommitted to "work". We really should rename this method, but I would guess the desired semantics are like "flush": "ensure UIKit is completely up to date with the current state of the asyncDataSource, whether or not initial load has happened, and including any edit operations recently performed"

[appleguy]

maybe we could call this waitOnLayout, flushLayout, or updateSynchronously?

[Adlai-Holler]

@appleguy I agree. My current estimate is, that it should trigger the initial data load to start if needed, but otherwise not trigger a layout pass.

[nguyenhuy]

@Adlai-Holler: thanks for working on this.

I forced a layout pass in '-waitUntilAllUpdatesAreCommitted' to avoid a breaking behavioral change in that method. Many users have used it as a way to ensure all updates were "flushed" and the collection/table is up-to-date with its data source. Our tests also relies on this behavior. But now that we do native reloadData, that expectation won't be true until the next layout pass happens.

Renaming is probably a great idea. I'd prefer a breaking API change that the compiler will pick up over a silent breaking behavioral change.

Edit: sorry I misspelled your name, Adlai!

[Adlai-Holler]

We need to be careful to separate two issues here:

• Whether the method should trigger initial reload or not.
  • I believe it should but last time I implemented it, it caused a ton of issues so I backed off.
• Whether the method should force a layout pass after flushing, to ensure that, if reloadData was included, UIKit has requeried us.
  • I believe it should not. It is sufficient for us to say "we called reloadData on the UICollectionView. In that sense, we have committed all updates."

[Adlai-Holler]

Since this is causing immediate issues in Pinterest, I'm landing this for now and we can continue discussion about this later.