Use UIViewPropertyAnimator for interruptible and cancelable transitions

[marcosgriselli]

Attempt to add a workaround for #52. I'm having some good results using the example app with an iPhone 11 Pro. This is not the ideal solution but avoiding black screens is the goal for the moment.

[marcosgriselli]

@eliaxyz would you mind trying it out on your project?

[coditobot]

	2 Warnings
:warning:	Big PR, try to keep changes smaller if you can
:warning:	SwipeableTabBarController/Classes/SwipeTransitioningProtocol.swift#L11 - TODOs should be resolved ((marcosgriselli): - Support Ob…). todo SwipeTransitioningProtocol.swift:11

Current coverage for SwipeableTabBarController.framework is 79.87%

Files changed	-	-
SwipeableTabBarController.swift	76.26%	:warning:
SwipeTransitionAnimator.swift	82.35%	:white_check_mark:

---

Powered by xcov

Generated by :no_entry_sign: Danger

[wasp898]

I haven't exactly tried the proposed solution, I only took the idea of setting the selectedIndex when transitionCoordinator == nil but eventually I still saw the issue. Not sure if you can confirm. I'll see to checkout this branch soon and play with my app...

In the meantime, I actually thought that maybe the issue that we see could be caused by memory leaks of some kind. And I actually found a strong reference to self in the code. No idea if this has any effect - probably not - but let's get rid of it and see what happens?

https://github.com/marcosgriselli/SwipeableTabBarController/blob/2f7d4f7512654f6e9c47c102a63234efb1a807f9/SwipeableTabBarController/Classes/SwipeTransitionAnimator.swift#L55-L62

[marcosgriselli]

@wasp898 Hm this seems to solve the issue at least on the timer tests. Let me know how it works on your branch.

Animations are executed immediately so I don't think that self reference is causing any problem.

[eliaxyz]

@eliaxyz would you mind trying it out on your project?

Good work, Marcos! I can confirm, that the modified version is passing the race-tests without crashing.

I guess also the "black screen", which is caused by swiping too fast should be gone with this modification.

Why do you think, the solution is not perfect?

[marcosgriselli]

@eliaxyz that's great news! I'm not 100% convinced with the solution since attempting to change the selectedIndex while a transition is in progess will lead to the selectedIndex not changing. The behavior is different from the native UITabBarController but at least it won't show the black screen anymore. This leaves us in a far better position than the previous bug. I'll make sure to add a note in the Readme to address this.

[eliaxyz]

@marcosgriselli

In my first impressions while testing

attempting to change the selectedIndex while a transition is in progess will lead to the selectedIndex not changing

seems not to be a big issue for the swipe-experience. But of course it should still be possible to change the selectedIndex programmatically.

To achieve this, we could introduce a new function for changing the selectedIndex immediately (without animation). If we want to make the controller to react more like the native Controller, we could achieve the non-animated change by setting the selectedIndex, (as in the native Controller) and adding a new name for the animated changes (for example swipeToIndex or moveToIndex)

To be sure, "moveToIndex=3" or moveToIndex(3) will not crash the controller, we could queue up the command and start it as soon as the running transition is ready.

What do you think about my idea?

[marcosgriselli]

@eliaxyz yes that could work. I like the idea of keeping the native selectedIndex functionality. We'd need to define a good strategy to queue up transitions in a way that still feels responsive.

[wasp898]

@wasp898 Hm this seems to solve the issue at least on the timer tests. Let me know how it works on your branch.

I've been playing with my app on this branch for a couple of days. I can confirm that eventually the app still freezes and shows the wrong tab (which then leads to a black screen after moving to background and then foreground again). It is much harder to replicate though, so I still think it is an improvement!

Hope this is not a bummer. Actually let me know if I can help in any way.

[marcosgriselli]

Ok, it's a small step forward. I'll keep digging to see if I can nail a fix that always works.

[wasp898]

Good news on my part. I finally got to a reproducible state that is always linked to the app freezes. Not sure how to pursuit it further but maybe you can help or implement it yourself on this branch. So, by keeping track of the swiping state, it appears that sometimes we can get to calling animateTransition when actually the gesture is already in .ended state, and that I guess causes the transition to never finish somehow. Right now I'm only doing this: I set this varialble to false when the gesture recognizer get to ended or cancelled, and check the value in the SwipeAnimatorTransition to get to a nice warning in case I find that no interaction is actually ongoing. If I play enough with my app, swiping left and right, eventually I get the app to freeze, and right there I have the warning in the console waiting for me 100% of the time.

What to do next though? I'm not an expert on transitions and stuff at all, but I'm guessing that we should somehow force the transition to complete (or to get cancelled?). That's where you know more than me.

I tried a few things but without success. I'll keep playing with ideas tomorrow. Hope this helps. 👍

[marcosgriselli]

@wasp898 great! I'll look over at the gesture recognizer state see if I can find any workarounds

[marcosgriselli]

Well, I didn't want to rely on UIViewPropertyAnimator for this but it is the only way to fix this issue. The ability to cancel transitions on the fly.

Check it out, running 30 transitions at 0.1 intervals. If a new transition gets called then it will immediately finish the one running to start the new one. In the end you can see how swipe and tapping still work as expected after this frenzy of starting/canceling transitions. Since UIViewPropertyAnimator animations are interruptible you can swipe while a transition is in progress and make it go the other way. This should be enough to fix any potential swiping problems.

Video file

[marcosgriselli]

This fix would require the library's minimum deployment target to be iOS 10. Does anyone have a problem with that? @wasp898 @eliaxyz

[marcosgriselli]

Also, I tested the fix using the demo project which is way too simple to cause any additional problems. Maybe we find edge cases on apps with real features.

[wasp898]

This fix would require the library's minimum deployment target to be iOS 10. Does anyone have a problem with that? @wasp898 @eliaxyz

Not at all.

[eliaxyz]

Of course not

(Sent from mobile)

Am 27.06.2020 um 17:22 schrieb wasp898 notifications@github.com:  This fix would require the library's minimum deployment target to be iOS 10. Does anyone have a problem with that? @wasp898 @eliaxyz

Not at all.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[marcosgriselli]

@wasp898 @eliaxyz I just pushed the changes using UIViewPropertyAnimator to this PR. Everything seems to work now. Can you take a look at it on your apps?

[wasp898]

Great job marcos!! Thanks a lot! No freezes for me since yesterday. Plus I think the newly gained responsiveness is a big step up from the previous solution. Great!

The only few unexpected behaviors I see:

• transitions feel very slow compared to before, even though the same setting is used as animation duration
• I can easily get to a state where the selected tab is different from the view controller being presented
• swiping fast back and forth sometimes can lead to a quick jump to a whole other tab without the animation

All and all, minors. I guess only the second one could be troublesome potentially, as I guess it was the cause of the freezes and the black screens before. I have yet to see this problem with this current solution though. The first issue is a non issue since we do provide a way to change the preferred animation duration. I'm mentioning this because in my opinion, by default, swiping among tabs should feel just like swiping through pages on the Home screen.

[marcosgriselli]

@wasp898 ok so a few issues that need to be fixed. Would you mind trying to replicate the problems on the demo project? That way we'll be able to determine if having real screens might have an influence on these issues.

[wasp898]

Here's a video showing the wrong tab being shown in the tab bar: video. Notice how at the end of the video the app is showing the Settings page while the tab highlighted is Team. A further quick swipe back and forth doesn't update the tab bar. It is actually quite easy to replicate.

Here's another video showing what I meant when I talked about quick jumps: video. The end of the video especially. Let me know if it's clear.

Anyways, I've got a bit of bad news sadly. Black screens still happen to me. At least now the app never freezes, but I have seen black screens after resuming the app later on.

[wasp898]

I see this PR is still open, I hope I'm not the reason why it was halted. 😭

I still think it is a GREAT step up in performance compared to the version that is now in release. In fact, this is the version I ended up adopting in my own application, even with the black screen bug that still happens (albeit very rarely, actually).

Just my two cents... but I think that after the work you put in @marcosgriselli, this PR should be merged without hesitation.

p.s. I just noticed that the videos that I attached in the other post don't even show the tab bar 😅 sorry, it's a player problem

[marcosgriselli]

@wasp898 that's great that you're using this version and it's working for you. I'll go ahead and merge it. I'm not currently using this library on my projects so I might be a bit slow to work on fixes or improvements but I still try to respond and help out. I also saw your newly created issue with iPad on slip screen. I'll submit a fix and push a new version.