Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chatlayout contentOffset bug #83

Open
SongSeoYoung opened this issue Nov 19, 2024 · 2 comments
Open

chatlayout contentOffset bug #83

SongSeoYoung opened this issue Nov 19, 2024 · 2 comments
Assignees

Comments

@SongSeoYoung
Copy link

SongSeoYoung commented Nov 19, 2024

Hello.

I have a question about the CollectionviewChatLayout class code.
In the targetContentOffset function, what is the logic to calculate the newProposedContentOffset value if the proposedCompensatingOffset value is not zero and collectionView is not nil?

let newProposedContentOffset = CGPoint(
    x: proposedContentOffset.x, 
    y: max(
        minPossibleContentOffset, 
        min(collectionView.contentOffset.y + controller.proposedCompensatingOffset, maxPossibleContentOffset.y)
    )
))

In the above code, in the logic that uses the value of minPossibleContentOffset and the maximum value of min(collectionView.contentOffset.y + controller.proposedCompensatingOffset, maxPossibleContentOffset.y) to calculate the value of y, why is the value of the second argument min?

If I proceed with the code min(collectionView.contentOffset. y + controller.proposedCompensatingOffset, maxPossibleContentOffset.y), I get a bug,

I changed it to max (see code below) and it seems to work:
max(collectionView.contentOffset.y + controller.proposedCompensatingOffset, maxPossibleContentOffset.y).

I'm curious as to why you used min.

As I understand it, maxPossibleContentOffset is the maximum scrollable offset in the current collectionView.
If proposedCompenstatingOffset is non-zero, I guess I should use maxPossibleContentOffset to adjust the offset.

In most situations, I've never had a problem with the two values below being the same.

  • collectionView.contentOffset.y + controller.proposedCompensatingOffset
  • maxPossibleContentOffset.y

But in a special situation, the maxPossibleContentOffset.y value was larger, and only when I assigned this value to newProposedContentOffset did I see the offset move as well as if I applied keepContentOffsetAtBottomOnBatchUpdates.

I would be grateful if you could answer.
Thank you.

*Note: In the example code I tested, I changed the value of keepContentOffsetAtBottomOnBatchUpdates to true and didn't modify the other flag values.

@SongSeoYoung
Copy link
Author

SongSeoYoung commented Nov 20, 2024

@ekazaev And the code below should work as well, can you please check it?

  1. If keepContentOffsetAtBottomOnBatchUpdates is true, I think you can set the value of y to maxPossibleContentOffset.y.
  2. If keepContentOffsetAtBottomOnBatchUpdates is False, you can use the targetContentOffset value of the super class.
    Please let me know if I missed anything.
   open override func targetContentOffset(forProposedContentOffset proposedContentOffset: CGPoint) -> CGPoint {
        if keepContentOffsetAtBottomOnBatchUpdates,
           let collectionView {
            
            invalidationActions.formUnion([.shouldInvalidateOnBoundsChange])
            let offset = CGPoint(x: proposedContentOffset.x, y: maxPossibleContentOffset.y)
            controller.proposedCompensatingOffset = 0
            
            if needsIOS15_1IssueFix {
                collectionView.contentOffset = offset
            }
            return offset
        }
        return super.targetContentOffset(forProposedContentOffset: proposedContentOffset)
    }

Additionally, if the above code is correct, please check whether proposedCompensatingOffset is necessary.
Because in the existing code, the case where proposedCompensatingOffset is not 0 was only when keepContentOffsetAtBottomOnBatchUpdates is true. If you don't use the proposedCompensatingOffset value, you can modify the condition to only when keepContentOffsetAtBottomOnBatchUpdates is true. In this case, it seems like there is no need to set proposedCompensatingOffset.

Thank you 😀

@ekazaev
Copy link
Owner

ekazaev commented Nov 27, 2024

@SongSeoYoung I dont fully remember all the details. Some code exists there even if it seems dumb just because collection may not reflect the layout logic the way we both think it should. Some code exists also just so it easier to test edgecases. And make easier to comment out something and check how it works without it. Do not forget that in IOS 18 calculation may be different that in ios 12. Most of the code was written aroung ios 14.
When it comes to targetContentOffset it also serves rotation trying to keep you as much as possible at the same spot.
But why are you questioning it in the first place?
And what is the bug?

@ekazaev ekazaev self-assigned this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants