Visible frame inset doesn't work as expected

[Banck]

@lkzhao Hello!
I have ComposedProvider with 3 providers(1 provider = 1 cells) with RowLayout have size == view.frame.width. I have only 3 cells and want they be displayed always, so I use this code:

 let visibleFrameInsets = UIEdgeInsets(top: 0, left: -view.frame.width * 5, bottom: 0, right: -view.frame.width * 5)
matchProvider = ComposedProvider(layout: RowLayout().insetVisibleFrame(by: visibleFrameInsets),
                                         sections: [infoProvider])
.....
            matchProvider.sections.append(videoProvider)
            matchProvider.sections.append(webViewProvider)

Those providers don't have their own layout.
But only 2 cells are initialized and Third isn't. Although I multiplied view.frame.width by 5 and visibleFrameInsets is right (-1875 left and right).
What's wrong? Seems it should work. But there is no changes between -view.frame.width and -view.frame.width * 5.

[casperzandbergenyaacomm]

insetVisibleFrame doesn't seem to do anything on the layout of a ComposedProvider, to fix your issue set the insetVisibleFrame on the videoProvider and webViewProvider.

Edit: I just tested it and it seems that that fix doesn't work, I don't know how to fix this at the moment other than avoiding ComposedProvider.

Edit 2: Hmm, the fix seems to work in the example project so I'm not sure what's going on here.

[casperzandbergenyaacomm]

Ok so I did some testing with ComposedProvider and found the bug. In FlattenedProvider.visibleIndexes(visibleFrame:) the visible frame inset of the layout is not taken into account.

A hacky fix for this is just grabbing the visible inset from the provider.

In FlattenedProvider.swift:

  // Change
  func visibleIndexes(visibleFrame: CGRect) -> [Int] {
    let insets = visibleFrameInsets(for: (provider as? LayoutableProvider)?.layout)
    let visibleFrame = visibleFrame.inset(by: insets)
    // No need to change anything else here, just add those two lines
    ...
  }
  
  // Add 
  func visibleFrameInsets(for layout: Layout?) -> UIEdgeInsets {
    if let visibleFrameInsetLayout = layout as? VisibleFrameInsetLayout {
      return visibleFrameInsetLayout.insets
    } else if let wrapper = layout as? WrapperLayout {
      return visibleFrameInsets(for: wrapper.rootLayout)
    } else {
      return .zero
    }
  }

[casperzandbergenyaacomm]

A better fix would be if the layout either returned the visibleFrame with the indexes or accepts the visibleFrame as inout so that it can change it and you can use it after calling to the layout. This would both allow for multiple WrapperLayouts or custom Layouts to change the visible frame while SectionProvider can still determine visible indexes per section.

But maybe someone that has more experience with the whole CollectionKit project can pitch in here.

[lkzhao]

Ok, so I found out the reason.
@casperzandbergenyaacomm is correct that visible insets is not being taken into account.
But the compose provider's visible indexes calculation is actually working correctly. ie. it does know that it have to display 3 sections given the visible frames. But after this step, it will ask each provider individually for their visible indexes within the visible frame. The ComposedProvider doesn't know how big is the expanded visibleFrame, so it is still using the one provided by the CollectionView. This is the location where it is not taking visibleInsets into account. The child provider might not have any cell visible within that original visibleFrame.

Here is the line that is causing the issue.
https://github.com/SoySauceLab/CollectionKit/blob/master/Sources/Provider/FlattenedProvider.swift#L93

if you change it to

let intersectFrame = sectionFrame

then it is all fixed. But this displays every cell in the section if the section is visible, which is bad for performance.

[lkzhao]

@casperzandbergenyaacomm You are right. We need a way for the layout to give back a visibleFrame somehow.

[lkzhao]

The thing is, do we feel like putting visible insets inside a layout is the right choice?
The original implementation puts visible insets on the CollectionView level, which avoids this problem and kinda make sense.
The benefit of the current implementation is to have different visible insets for different sections. But I guess it didn't work from the start.

[lkzhao]

Would be best to consider and support this as well: #109

[casperzandbergenyaacomm]

Different visible insets per section adds a lot of flexibility, for example a provider with shadowed cells needs a visible inset but not every other section in the collection view.

The only problem I see with the way it works now is that the inset would stack. If I make it so it returns the frame with visible inset then a nested provider will add it's own visible frame inset onto that. Not sure if this is expected behaviour.

I can make a branch with this behaviour (same as the hacky fix I posted) where the layout gives back the visible frame. I don't know how #109 should be implemented yet but if I see a good way to do it I'll try to add that.

[casperzandbergenyaacomm]

Ok so after toying with ComposedProvider and the visibleFrameInset I think that if we want different inset per section we need to sacrifice some optimisation.

Right now the way that Layout provides the visible indexes means that in a SectionProvider only the sections that are in the visibleFrame of the SectionProvider are shown without taking into account if those sections have a visibleFrameInset that would allow them to still be shown.

I can't figure out how to account for the visibleFrameInset of a child inside Layout so I think the easiest way to solve this is to change the way we determine if a section should be shown.

Instead of letting the Layout of the SectionProvider return the visible frames, we can ask all the sections to return their visible frames. This allows the visibleFrameInset to be stacked and for sections to have different visibleFrameInset.

[casperzandbergenyaacomm]

I rewrote some stuff to allow for nested visible inset, branch is here: https://github.com/casperzandbergenyaacomm/CollectionKit/tree/nested-visibleFrameInset-fix

Main changes:

• visibleIndexes(visibleFrame:) is now visible(in:) and returns both the indexes and the final frame.
• FlattenedProvider visible(for:) is rewritten to ask all its child sections for visible cells instead of just the ones that its section provider says are visible. This means that one section could have a higher visibleFrameInset than another section.
• Directional layouts now check if the visibleFrame is not empty or negative. We can't reliably check just the one direction because the layout can now be called from a section provider with a layout in a different direction.

Still have the problem that when you have horizontal layout providers inside a vertical layout section provider the visibleFrameInset of the horizontal providers are doubled.

We need to check this for performance and tests but I don't know how.