Add UIViewPropertyAnimator reactive extension

[twittemb]

This commit adds a Rx extension to UIViewPropertyAnimator.
The purpose is to trigger a Completable(.completed) once the
animation is ended so it is easy to chain them with the syntactic
sugar 'andThen'.

I'm note sure how to amend the Readme file because it seems to be the first UIKit extension, it is not really a Rx new operator. Should we start a dedicated section in the Readme ?

I wrote an example in the Playground but I don't know if I should add UI Unit Tests as well ? I'm not very comfortable with UI Unit Tests.

[freak4pc]

Hey @twittemb - Looks great!
It's a Holiday this week so I won't be able to review this until 4/8 - If anyone else can grab this, that'd be great!

[freak4pc]

Hey @twittemb - Great PR!
Left a few points for change & discussion.

Regarding tests - Can we at least perform some basic unit tests, e.g.: - that after every "completion" the object arrives at its designated position/transform/etc ?

Thanks again!

[freak4pc]

What's the purpose of this file?

[twittemb]

My bad ... removed !

[freak4pc]

What do you think about changing both of these to Added UIViewPropertyAnimator Reactive Extensions?

[twittemb]

done.

[freak4pc]

You're right about the fact this is the first Extension we have on RxCocoa. I'm not 100% sure on how to name this thing.

Let's change for now to this - I also removed the note about "subscribing" because I think its implicit in the fact its an Observable:

**UIViewPropertyAnimator** [animate](animate) operator, returns a Completable that completes as soon as the animation ends.

eg.

• UIViewPropertyAnimator animate operator, returns a Completable that completes as soon as the animation ends.

[twittemb]

done.

[freak4pc]

Two quick notes about this example:

1. I'd love it if the animations could be different, different speeds or scale and translate etc
2. It would be great if we could have a button on screen to trigger the animation. That way its obvious how to trigger the animation from a user event
3. Can we add a note somewhere about the fact they should open the Assistant Editor to see the live view?

[twittemb]

done.

[freak4pc]

I think we usually use the shortened syntax here, e.g.

Completable.create { completable in

[twittemb]

done.

[freak4pc]

What do you think about adding a let strongBase = base outside of the closure? I think that otherwise there's a risk of retaining self here.

[twittemb]

done but with a slightly different way. Until now I didn't notice we could use the capture list that way:

{ [base] completable in ... }

What do you think of this syntax ?

[freak4pc]

Same here about trailing closure syntax, and I'd also use a guard:

base.addCompletion { position in 
    guard position == .end else { return }
    completable(.completed)
}

[twittemb]

done.

[freak4pc]

can't we squash down both the method and property to a single method with a default value ?

e.g.

public func animate(afterDelay delay: Double = 0) -> Completable

/// Usage
animator.rx.animate()
animator.rx.animate(afterDelay: 5)

I think it would also better be on-par with the fact its an "operator". Computed properties have an implicit meaning of just returning things and not having side-effects necessarily.

[twittemb]

done. Good catch.

[freak4pc]

Why is this needed? I think since UIViewPropertyAnimator is NSObject, you get this out-of-the-box.

[twittemb]

done.

[twittemb]

Hi @freak4pc

All points have been addressed, even the Unit Test. I hope this will meet your expectations.

Thibault.

[freak4pc]

Hey @twittemb, I made a few minor changes:

• The extension you made wasn't public so it wasn't visible outside of the extension itself.
• You used @available(iOSApplicationExtension 10.0, *) instead of @available(iOS 10.0, *)
• You used a Double instead of a TimeInterval (I know its a type alias, but when dealing with time its best to leave contextual meaning)
• Cleaned up the playground page to trigger the animations based on the button tap in a Rx way (with a flatMap) and fixed the link from the main playground page
• Cleaned the tests, moving the availability clause to the test itself

I also rebased your changes on top of master since there were some changes.

There are two final issues we need to resolve:

• If the user taps twice on the button, the playground will crash with: A paused animator (<UIViewPropertyAnimator(0x6000001f5400) [active] interruptible>) cannot be started with a delay!. I'm not sure if this is acceptable - should we find a way to terminate the animation, or should we generate the property animators on-the-fly when the user taps on the button, inside the flatMap possibly?

• Still need to decide how to deal with the README. I'll think about this a bit more :)

Thanks again for the hard work. @twittemb

[twittemb]

Hi @freak4pc

I have pushed a new version that prevents the demo from crashing. I disable the button once the animation is completed. If the developer wants to see it again he will have to launch the demo again. Seems an acceptable compromise.

[groue]

Seems an acceptable compromise.

I certainly don't want to tell you what to do, @twittemb. But UIViewPropertyAnimator is a nasty little API that crashes as soon as the hosting app doesn't take enormous care of its under-documented fragile inner state machine. And if it works on iOS11, this doesn't mean it does on iOS10. It's really nasty. I'd... spend a little more time tuning this RxSwiftExt addition, if I were you and if I had time. This is just experience talking: the crash experienced by @freak4pc rings a big warning bell in my mind.

[groue]

I'm afraid I did spread some FUD in my previous message, I apologize.

I'll try to put it in another, more positive way:

A demo app is a great way to check the ergonomics of a proposed API in context. If the most simple way to use an API can lead to a crash, then maybe something has to be fixed.

button.rx.tap
    .flatMap { [unowned self] in
        self.animator1.rx.animate()
            .andThen(self.animator2.rx.animate(afterDelay: 0.2))
            .andThen(self.animator3.rx.animate(afterDelay: 0.1))
            .debug("animation sequence")
    }
    .subscribe()

The demo code above looks pretty innocuous, and can be described as the "most simple way to use the API".

(Side note: unowned self should only be recommended if it could never explode in the user's hands).

It would be great if this code would never crash (and the sample code can stay as it is), or, if it crashes, explains how to avoid the crash (and the sample code would be updated with the mandatory work-around that avoids the crash, along with a comment that explains everything).

This would improve this PR with nice properties: ergonomics check, and proper documentation of eventual caveats.

[twittemb]

Hi @groue

Thanks for your feedback ... No offense taken 😊

In the last version I pushed, the demo app doesn't crash anymore BUT you make a fair point saying that we should be careful not to spread bad practices.

My PR is about "bootstrapping" a Rx way of chaining animations and hopefully it will be amended with new features in the future. My point is that it is up to the developer to use UIPropertyViewAnimator is a safe way, with or without an Rx extension.

Perhaps, if you're ok with that, I could add a comment in the demo app to warn the developers about handling correctly the inner state of the animation ?

See ya.

[groue]

Perhaps, if you're ok with that, I could add a comment in the demo app to warn the developers about handling correctly the inner state of the animation ?

Sure, I'd like to know how that crash can be avoided. Put in another way: is it a known issue of the PR (<insert work-around here>), or a bug in UIKit? It's still a little unclear to me :-)

[freak4pc]

@groue @twittemb

It's not a bug in UIKit, it's an expected and confusing behavior of UIKit where you can't "reuse" them AFAIK.

It is a programmer error to call this method while the state of the animator is set to stopped.

https://developer.apple.com/documentation/uikit/uiviewanimating/1649786-startanimation

The other option is to use the class method like UIView.animate(withDuration...), e.g. :

https://developer.apple.com/documentation/uikit/uiviewpropertyanimator/1648367-runningpropertyanimator

In practice, it might be better to make a Reactive wrapper for the static method and not the UIViewPropertyAnimator instance, I guess?

Otherwise, should we just create the UIViewPropertyAnimator instances in the playground "on the fly", so you get fresh ones whenever you tap the button? (we can do it in a .do(onNext:) before the flatMap, possibly).

[freak4pc]

@twittemb @groue I think this is a good solution.
I'm just making fresh UIViewPropertyAnimator on every invocation which reversed it.

Also, when the user doesn't provide a delay (delay = 0), I call the regular startAnimation() instead of the delay counterpart, as they actually have different behaviors. (the regular one won't crash in a paused state).

[groue]

Thanks @freak4pc. Your explanation is pretty clear, and the sample code is much better (and more playful)!

[twittemb]

@freak4pc @groue

Great job guys. Perhaps one of the three animations should use the "afterDelay" parameter ?

[freak4pc]

Right, good point :) Since we're creating them fresh anyways.

[twittemb]

@freak4pc do you want me to make the fix ?

[freak4pc]

@twittemb I'll amend my last commit, no worries.
Now only thing left is to thing about the README a bit ... I'll think about this today and make some fix tomorrow and merge this :)

[freak4pc]

Donezo @twittemb! I'll think about how to add this to the Readme until tomorrow.

For now, I'm inviting @RxSwiftCommunity/contributors to add any feedback on this Reactive Extension before its merged.

Thanks ! :)

[freak4pc]

@twittemb Let me know your thoughts on the proposed Readme.

[twittemb]

Hi guys

It sounds great to me !!!

Thanks a lot for the help.

[twittemb]

Thanks !

See ya 😊