Fix reloading after sections updates

[icepat]

Details

The CollectionView reload is triggered by this func from the MagazineLayout Library :

If we follow that call it comes from the override public func prepare() here :

The prepareActions is a bit field, and you check for updateLayoutMetrics (which triggers the updates) and just bellow that highlighted line of code on the screenshot the following condition checks for recreateSectionModels which are both set to true in our situation.

The fix

The recreateSectionModels code should be called before updateLayoutMetrics if recreateSectionModels is set to true because there is no need to update the sections if we are going to recreate them anyway right after the update.

Related Issue

We have a very old crash that is occurring on our main iPad app.
The crash happens following these steps :

• Launch the iPad App in portrait mode(logged in), land on the Home Page
• Go to settings and Sign Out
• Attempt to sign in again
• Rotate to landscape & cancel the Login Modal
• Go to the Home Page
  -> CRASH

It crashes because there is more sections on the Home Page when the user is logged (34 sections) in compared to when the user is logged out (28 sections), some sections are based on the user's preferences and his content, like recommended, suggested...).
The current code implementation tries to update the existing sections and an out of bounds crash will occur because the number of sections logged out are inferior and it will attempt to update a section that just doesn't exist.

Motivation and Context

The current behaviour is the following :

• Check if updates are required, if yes, updates them
• Check if the sections should be completely re-created, if yes, recreates them

This fix just swap the 2 steps, check for recreation first and then for updates.

How Has This Been Tested

The reproducibility of this crash was 100% in our situation (following steps detailed up here).

Tested on :

• Simulators, iPad mini, iPad 11 Pro, 13 Pro
• Devices, multiple iPad mini, & iPad pros
• Tested by QA team and Engineers over a course of a week
• Reproduction rate of 0% after the fix

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.

[bryankeller]

Thanks for the change! It would be helpful if a repro case for this bug was provided. Would this be easy to provide?

One other question: would it be simpler to just not update the layout metrics for existing sections if we're recreating them from scratch anyways? Maybe the change is even simpler:

// Update layout metrics if necessary
if prepareActions.contains(.updateLayoutMetrics) {
  ...

becomes

// Update layout metrics if necessary
if !prepareActions.contains(.recreateSectionModels), prepareActions.contains(.updateLayoutMetrics) {
  ...

Let me know if this works. And if you can provide a repro-case, that would be really helpful so I can better-understand what application behavior triggers this.

[icepat]

Apologises for the slow reply, unfortunately I did not get my manager's approval to provide a repro case or a video about our project, apart from what I can share here :

• We have a different number or rows in our collection view if you are Logged In (more) than Logged Out
• If the user is Logged In and the app is in portrait we show them 30 rows (with personnal suggestions or bookmarks for example)
• If the user changes tab (the collection view is not on the screen at that point), then log out, then rotate the device, then returns to the Home Page
• The collection view will attempt to reload BUT it will crash because the number of rows has changed from the previous state when the user was on the Home Page & Logged In

I can confirm that your suggested changes does indeed fix the issue as well, so I guess this PR can be cancelled if you guys do push your suggested change instead.