search

flowkey / UIKit-cross-platform

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

improve how multiple gesture GestureRecognizers and Responder chains work together #333

Closed michaelknoch closed 2 years ago

michaelknoch commented 3 years ago

Type of change: bugfix

Motivation (current vs expected behavior)

fixes https://github.com/flowkey/UIKit-cross-platform/issues/306

Please check if the PR fulfills these requirements

codecov[bot] commented 3 years ago

Codecov Report

Merging #333 (f9ba5c3) into master (6f92297) will decrease coverage by 0.05%. The diff coverage is 95.45%.

:exclamation: Current head f9ba5c3 differs from pull request most recent head cbb930d. Consider uploading reports for the commit cbb930d to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
- Coverage   51.25%   51.20%   -0.06%     
==========================================
  Files          87       87              
  Lines        2899     2902       +3     
==========================================
  Hits         1486     1486              
- Misses       1413     1416       +3
Impacted Files Coverage Δ
Sources/UITouch.swift 100.00% <ø> (ø)
Sources/UIView.swift 90.15% <66.66%> (-0.43%) :arrow_down:
Sources/UIGestureRecognizer.swift 89.18% <100.00%> (+0.95%) :arrow_up:
Sources/UIPanGestureRecognizer.swift 97.05% <100.00%> (-2.95%) :arrow_down:
Sources/UIScrollView.swift 87.50% <100.00%> (+0.17%) :arrow_up:
Sources/UIWindow.swift 69.69% <100.00%> (+0.94%) :arrow_up:

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 6f92297...cbb930d. Read the comment docs.

michaelknoch commented 2 years ago

I think I addressed your main feedback point from july 2021 in https://github.com/flowkey/UIKit-cross-platform/pull/333/commits/28da989780e81a7ef987dc4efebc2a43a07f8327. As far as I remember you wanted me to decouple UITouch and UIGestureRecognizer. You think this can go in? Happy to have another session to clarify about details if needed @ephemer .

michaelknoch commented 2 years ago

I went through the PR again and tried to clean it up once more. The main change of bc95ab628685c9b03393fbb8b06029e5ee8d6e6d is that we now cancel other recognizers from the child classes UITapGestureRecognizer and UIPanGestureRecognizer rather than in the state setter of UIGestureRecognizer. The reason for this is that we want a slightly different behaviour for the cancellation of other recognizers, similar to the cancellation of the hitview responder (began vs moved for UITap and UIPan).

I also reverted as much of the random new conditions as possible, I added quite a few early returns and additional checks with the first iteration. I remember that I had to add them to either fix the tests or fix the behaviour of our player. With the new flow this doesn't seem to be needed anymore.

Ok I am getting the feeling more and more that part of the fix would be ensuring that TapGestureRecognizers get set to the .changed state at some point in their lifecycle in order to make things like this work as expected? Would be good to understand what iOS does here I tried to avoid this by running individual "cancelling code" in the child classes rather than trying to cancel hitview responder and other recognizers from the UIGestureRecognizer class. It could make sense if we want to have the cancelling at one place again, but this new approach feels more flexible and more natural to me.

Happy to hear your thoughts @ephemer

ephemer commented 2 years ago

Ahh but please address the comment about the reset() function before merge as well, and reconsider whether we might want to change the exoplayer version in another PR instead

michaelknoch commented 2 years ago

finally merged! thx for your support @ephemer