flowkey / UIKit-cross-platform

Cross-platform Swift implementation of UIKit, mostly for Android
MIT License
594 stars 40 forks source link

only cancel bounds animation when setting contentOffset without animations #319

Closed michaelknoch closed 3 years ago

michaelknoch commented 3 years ago

Type of change: bugfix (align with ios behaviour)

Motivation (current vs expected behavior)

Due some recent changes in the coursesv2 player logic it can happen that we sometimes have two sheet animations animating back to the beginning of the song or to the beginning of the loop. It might be possible to clean this up, but from UIKit perspective this should work, because initiating another contentOffset animation should just override the existing one and then either create a new animation starting from layer.bounds or presentation.bounds depending on beginFromCurrentState. This works on iOS and it works in our UIKit when velocity scroll is disabled.

I noticed that we are removing any bounds animations when the contentOffset of a scrollView gets updated. We need this, otherwise there is a weird jump in the sheet when deceleration finishes and exoplayer updates the x position. But we also set the contentOffset when animating back in a loop or at the end of the sheet, in this case it does not make much sense to remove the animations.

I think the fix here is to remove the animations only when setting the contentOffset without animation (when set from exoplayer in our case).

https://user-images.githubusercontent.com/5617793/107763467-f74eec80-6d2e-11eb-808d-0dba7f2cc61f.mp4

Please check if the PR fulfills these requirements

codecov[bot] commented 3 years ago

Codecov Report

Merging #319 (2b616c1) into master (fbe3d88) will increase coverage by 0.01%. The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   51.46%   51.47%   +0.01%     
==========================================
  Files          87       87              
  Lines        2874     2877       +3     
==========================================
+ Hits         1479     1481       +2     
- Misses       1395     1396       +1     
Impacted Files Coverage Δ
Sources/CATransaction.swift 78.57% <ø> (ø)
Sources/UIScrollView+velocity.swift 41.66% <ø> (ø)
Sources/UIScrollView.swift 87.32% <75.00%> (-0.92%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fbe3d88...2b616c1. Read the comment docs.

michaelknoch commented 3 years ago

I am happy to revert this if there is a better solution (there probably is). But unfortunately we cannot simply check if there is a bounds animation in layer.animation because we want to know if the setContentOffset call happened within an UIView.animate block or not. It may happen that there are still pending animations on the layer at that time @ephemer