Various improvements, different approach on custom animations/views

[ffried]

tl;dr; I've removed the PullToRefreshViewAnimator protocol and allowed subclassing PullToRefreshView for more control over how the PullToRefresh control looks like.

What I've done in detail:

• Use NSLocalizedString for the texts to allow localization
• Make (global) constants private so nobody is going to access them
• Some smaller changes (use let instead of var, better handling of optionals, etc.)
• Use AutoLayout in all classes to ensure proper layout on all screen sizes as well as custom heights
• Remove PullToRefreshViewAnimator protocol in favor of subclassing PullToRefreshView Some notes on the last point: The PullToRefreshViewAnimator protocol worked fine but had a drawback: The layout had to be checked every time the layoutLayers() function was called. And with that there's no easy way to use AutoLayout for example. Nor was it possible to have the pull to refresh view have a different size than the default size. I've changed the approach to subclassing PullToRefreshView. Now it's possible to do all I've mentioned above within such a subclass. I've also ported the default animator to a DefaultPullToRefreshView. It basically looks the same as the old default Animator. It just also uses AutoLayout to layout the label. The functions, too, almost stayed the same. Instead of layoutLayers() you'd just override layoutSubviews(). For simplicity, I've added a state property which reflects the current state of the view. And there's the stateChanged() function which is called whenever the state changes. In this function you can update a label's text for example. Then there's the initialize() function for initializing a PullToRefreshView. It's independent of the way the view is created (IB or manually in code). In there you could for example add a fixed-height constraint if you want the view to have a fixed height.

I've updated all the existing code (including tests and samples). The tests still ran fine for me.

I know that this is a breaking change. This is why I updated the tag to 0.2 so anyone who doesn't want to update can stay < 0.2.

If you have any questions let me know.

[jcavar]

Hi, thank you for this. I will need some time to review all of this. When I do that I will let you know what I think.

[jcavar]

Hello @ffried, I finally have some time to discuss this. Sorry for waiting this long. I agree with most of your points, but I am not sure about last one. I think protocol solution is more elegant and allows more flexibility then subclassing so I would like to keep that in that way. Can you write which problems with autolayout you had? Custom height was planned and I think we can implement it with current architecture.

[ffried]

Hi @jcavar, thanks for having a look at it. About the last point: I'm not sure what flexibility is lost when using a subclass. In my opinion, more flexibility is added, because you have full control over the view. With the protocol you can only do whatever the protocol allows you to do. There should definitively be a function like my initialize() function to set up the layout (adding subviews and constraints). Doing so in layoutLayers() requires checking if a view/constraint was already added because it's called more than once. This is basically the main problem I had with the protocol. Among the fact that doing the layout in layoutLayers() requires the scrollview extension to call this function before adding it as subview so the height of the view is given. Another drawback of the protocol is, that every new feature (like custom height :wink:) has to be added manually including a corresponding protocol function while it's already given with the subclassing approach. Last but not least you only get a reference of the actual view in layoutLayers() which for example makes having the background of the whole view change while dragging difficult to implement. You'd either have to create a custom subview for this or keep a reference of the view in layoutLayers().

Of course it's not impossible to use the protocol solution. I just think that the subclassing solution is easier to maintain. What do you think about these points?

[jcavar]

I meant, you can have whatever object you want and as long as you conform to PullToRefreshViewAnimator protocol, you are good. The idea was to decouple animation part and actual UIView subclass namely PullToRefreshView which is kind of placeholder and I think it should be private(not sure why I haven't done this). It is true that you can do what protocol allows you to do, but we can have nice compile time checking what we cant have with custom subclass as far as I know. Lets say for custom height example we were talking:

1. We add var pullHeight: CGFloat {get} in PullToRefreshViewAnimator
2. We change addPullToRefreshWithAction method with this var pullToRefreshView = PullToRefreshView(action: action, frame: CGRectMake(0, -animator.pullHeight, self.frame.size.width, animator.pullHeight), animator: animator) and that should be it. And this is nice use of protocols for me. You can limit and extend your functionality as you want. As you said, we have to add it manually but that doesn't looks like problem to me. We have compile time checking and code is self documented.

Regarding initialize method, yes, I agree with you. I wasn't very happy with my solution. Maybe we could add this to protocol too? Though, not sure about that ... What do you thing about all of this?

[ffried]

What do we need compile time checks for? Animations etc. isn't something the developer has to do. If he wants to he can. Otherwise he can just ignore it. In case of the protocol solution he still has to implement all functions. With the subclassing solution he can just override them or not. You could even "secure" functions he shouldn't override by declaring them final.

As for the height. Doing it like this would require manual calculation and defeats the purpose of AutoLayout. In my solution, the height is automatically calculated by AutoLayout when the view is added to the scroll view. With AutoLayout using CGRect etc. manually almost always ends up messy.

Another reason I chose to use a subclass is that Apple does the same. Their UIRefreshControl is even a UIControl subclass. And although it's not publicly documented, you can use it with any scroll view by just adding it as subview. Refresher is almost the same (just that it has an extension on UIScrollView to make adding a PullToRefreshView easier) which I like very much.

I also see the point of using a protocol for animation controlling. Just like Apple does with custom view controller transitions. But IMO it only makes sense if there's more than one view involved (like with view controller transitioning). If it's only a small view which is responsible for the animations, it's mostly easier to use a subclass...

Anyway, I think in the end it's your decision if you want to stick to the protocol solution or switch to the subclassing solution. I'm going to hold on to my fork. I'm already using it like this in an app and it works just fine. If you want to keep the protocol solution, I could try to keep the other functionalities and restore the protocol. However, at the moment I don't have much time left. So it would take some time until I can take another look at it.

[jcavar]

Yes, I can agree with most things you wrote here. However, it is breaking change so I would like to think more about it. In meantime I will integrate some of changes from your pull request. Thanks once again.

[jcavar]

@ffried I have added basic support for custom subviews. I have extended current solution with support for custom subviews so user can choose what he want to use. It is here: https://github.com/jcavar/refresher/tree/subviews It requires more work but please take a look if you have time and let me know what you think.

Thanks

[ffried]

@jcavar Sorry for not getting back to you earlier. I was moving between countries. Anyway, I've had a look at your changes. I think I've made clear that I don't find the protocol-way the right solution here, so I'm not going to mention this every time now. :wink: Apart from that it looks good. The only thing I've noticed is that there's still some CGRect/CGPoint code. I'd completely go with AutoLayout. Because right now height changes of the view during the animation for example are impossible. With AutoLayout you'd only have to animate the changes of the constraints and you could even have a refresher view that's becoming bigger and smaller while it refreshes.

In my fork I've also already adapted the code to Swift 1.2. Maybe you can use some of my migrated code in your solution as well. :wink:

[jcavar]

Ok, thanks for your thoughts. I will think more about it.