Scrolling with 0 velocity will not land on the correct page

[MaherKSantina]

If we drag a bit backwards to the previous page and remove our finger, the paging will always go to the previous page. The expected behavior should be to stay on the current page if the threshold is low

Same goes to when scrolling to the next page with no velocity, no matter how much we scroll, we'll always end up on the current page. The expected behavior is to go to the next page if the scroll is greater than the threshold

[kayumovtd]

There is, as a result, another bug: If we try to scroll to the next item and then click on the item while scrolling, it returns us to the previous item.

I fixed this by changing MSCollectionViewPaging.collectionViewWillEndDragging horizontal case:

public func collectionViewWillEndDragging(scrollDirection: UICollectionView.ScrollDirection, withVelocity velocity: CGPoint, targetContentOffset: UnsafeMutablePointer<CGPoint>) {
    switch scrollDirection {
    case .horizontal:
        if velocity.equalTo(.zero) {
            targetContentOffset.pointee = CGPoint(x: currentContentOffset, y: 0)
        } else {
            targetContentOffset.pointee = CGPoint(x: getNewTargetOffset(startingOffset: currentContentOffset, velocity: velocity.x, targetOffset: targetContentOffset.pointee.x), y: targetContentOffset.pointee.y)
            currentContentOffset = targetContentOffset.pointee.x
        }
    case .vertical:
        targetContentOffset.pointee = CGPoint(x: targetContentOffset.pointee.x, y: getNewTargetOffset(startingOffset: currentContentOffset, velocity: velocity.y, targetOffset: targetContentOffset.pointee.y))
        currentContentOffset = targetContentOffset.pointee.y
    default:
        assertionFailure("Not Implemented")
    }
}

It's not the perfect solution, but it works better now.

[MaherKSantina]

Thanks @TimGrell so much for raising this bug and spending time to find a solution! I'll take a look at this after work and see how we can fix this

Cheers!

[MaherKSantina]

Again @TimGrell thanks for the solution! This looks like it fixes some of the edge cases but I just realized that I don't use scroll threshold at all 😆 I'm using a velocity threshold only (When you scroll faster than the threshold velocity, it moves to another cell). I'm going to make an update to introduce the scroll threshold and hopefully it will fix this issue

[kayumovtd]

It would be nice. Thank you for this library. 👍

[Errortype520]

Are there any updates? I tried the proposed solution which fixed the snap back, but made it so slowly pulling the collection and releasing snaps back to whatever was current before the scroll.

I made the following change instead:

    public func indexForItemAtPoint(point: CGPoint) -> Int {
        // at 375 * 200, peeking = 20, spacing = 20, item length = 137.5
        // at index = 2, point is 315
        // at index = 4, point is 630
        // at index = 6, point is 945

        let pointOffset: CGFloat
        switch scrollDirection {
        case .horizontal:
            pointOffset = point.x
        case .vertical:
            pointOffset = point.y
        default:
            assertionFailure("Not implemented")
            return 0
        }

        return min(max(0, Int(round(pointOffset / (itemLength(axis: .main) + spacingLength)))), numberOfItems)
    }

This seems to fix both issues, but introduces a new issue where a short fast drag from left to right snaps the collection to the next cell on the right strangely. It must have something to do with velocity but I have not worked it out yet.

Thank you for the library, hopefully we can find a solution that works for all cases.

---

EDIT: Not sure why but the delta calculated during a short fast swipe was causing the issue.

Changed the case where current and target index are the same to the following and everything is working:

case let (x, y, v) where x == y && v > scrollThreshold:
            delta = Int(velocity * 100)
            offset = 1

It is definitely not the cleanest solution but just making delta related to the velocity in this instance fixed the odd scrolling behavior with a quick swipe. I will keep checking back for a better solution.

[MaherKSantina]

Thank you so much @Errortype520 for spending time and raising this issue! I also appreciate you supplying a potential solution for this! <3

As for the solution, I'm not sure if rounding is a long term solution for this. It seems like it's just a patch and doing something like this might introduce other edge cases, and I can see that you're aware of that. That also applies to the velocity change

But you're definitely looking in the right place! getNewTargetOffset is a monster of a function and there's too much stuff going on. Right now it's a bit difficult to change the logic and make sure that all previous features are working properly. So I'm currently adding some unit tests to this function so that any changes we introduce will not affect previous logic.

Maybe we can figure a way to prioritize the scroll threshold when the velocity is 0, this way we don't jump to a neighboring cell if the user didn't swipe.

If you're interested I can push a change to the repo that includes the required tests and maybe we can investigate this thing together how about that?

Cheers!

[Errortype520]

@MaherKSantina I added rounding because it seemed to prefer the previous cell position even if next cell was closer to center when dragging stopped.

Looking at the func indexForItemAtPoint(point: CGPoint) -> Int function more, it looks like it takes the (item size + spacing) and the offset to find the index.

What about the initial peek?

It looks like index is calculated off of the left edge of the item. Shouldn't the index also be adjusted to consider the center of the item?

[MaherKSantina]

Thanks @Errortype520 for your comment! I'm sorry it takes me time to reply to you because we're in different timezones

The indexForItemAtPoint function is just a converter from an offset to an index. It basically will behave the same way even if we use the center to get the index. Suppose we have a width of 100 and no peeking, here's now it does it now: 0 offset -> 0 index 100 offset -> 1 index 200 offset -> 2 index ...

If we want to change the offset calculation to the center, it would be something like this: 50 offset -> 0 index 150 offset -> 1 index 250 offset -> 2 index

The peeking shouldn't affect this logic, because the peeking is something related to how the cells should be displayed on the screen. The spacing affects it so that's why I'm adding the spacingLength to mitigate this offset change

[MaherKSantina]

As for the rounding fix, I was wonder what would happen if we got a whole number after doing the equation? Wouldn't the value be the same and we end up with the wrong cell?

[MaherKSantina]

@Errortype520 , I've created a new branch to work on the fix (fix-scroll-issue). I've added some tests which are failing that we need to make them pass. If you have some free time feel free to try some things to make the tests pass. I'll also do the same thing and hopefully this issue will be gone forever!

[MaherKSantina]

@Errortype520 you're a genius! After digging into it I found out that you were right about the rounding for the index. It makes more sense because as you said it was preferring the left cell instead of getting the nearest. I've just merged a PR that fixes the issue https://github.com/MaherKSantina/MSPeekCollectionViewDelegateImplementation/pull/57 and I'll create a new version of the library that contains the fix. Thank you so much for helping me out hunt and fix this bug! Have a great day! @TimGrell thanks so much for you too for raising this issue. Feel free to re-open this issue if you found out that it's not fixed yet