DroidsOnRoids / SwiftCarousel

Lightweight, written natively in Swift, circular UIScrollView.
http://www.thedroidsonroids.com/blog/ios/circular-scroll-view-swiftcarousel/
MIT License
144 stars 43 forks source link

copyView doesn't fully copy a custom view #11

Closed irodrigo17 closed 8 years ago

irodrigo17 commented 8 years ago

Hi there,

First of all, thanks for sharing SwiftCarousel, it looks great!

I'm trying to use it in my project and just found out that internally it tries to copy my views using the copyView() method defined in the UIView+SwiftCarousel extension, but the implementation of the copyView() method doesn't fully copy my views, resulting in runtime exceptions.

The views I'm adding to the carousel are custom UIView subclasses and have multiple custom properties that are not copied properly by the archive/unarchive approach.

I'm taking a look at SwiftCarousel's code to see if it can be adapted to my needs, and made more general in the process for others to reuse, so I have a couple of questions:

  1. Is it really necessary to copy the views, or could we change the implementation somehow to avoid doing it?
  2. In case it is really necessary to copy the views, is it possible to add support for a custom copy method?
aocenas commented 8 years ago

Hi, I had similar problem, because copyView() does a shallow copy and my views have nested image view and label. What I did was rasterize the view and add just a flattened image view into carousel:

UIGraphicsBeginImageContext(view.bounds.size);
view.layer.renderInContext(UIGraphicsGetCurrentContext()!);
let img = UIGraphicsGetImageFromCurrentImageContext();
UIGraphicsEndImageContext();

let imageView = UIImageView(image: img)

In case of changes to the code, it would be best to supply just a factory function that would construct the view when needed, similar to table view data source IMHO.

sunshinejr commented 8 years ago

Hey @irodrigo17 πŸŽ‰

Thank you so much for the kind words! ☺️The general idea of carousel was that you can swipe once and the carousel could do 2 or even 3 rounds, depending on the force of the swipe gesture. Using three sections was a natural choice for me, because in that way we can make sure that the spin will happen. If you have any other idea, I'm really eager to hear from you!

About the copying, so we can't really use the same view with different position. So I've tried with copying the views and usually it worked OK (seems like it is not now :D).

Also the idea of @aocenas about factoring it is kinda cool, maybe instead of a delegate/data source, we can actually use closure in a more Swifty way? What do you guys think? πŸ€”

irodrigo17 commented 8 years ago

Hey guys, thank you both for your quick replies!

@aocenas: In my case, it's a bit more complex than that, because my view has a reference to the underlaying view model, which I need to access when the view is selected and its nil because of the shallow copy. So rasterizing the view doesn't help in my case. I do like the idea about a delegate method to return the view for any given index in an UITableViewDataSource style.

@sunshinejr: I understand, and also like the idea of using closures instead of a delegate actually to keep it Swiftier. I haven't had the chance to dive into the code yet, as this is for a side project I'm working on my own free time, but will take a look later today and see what can I come up with. What I did in the meantime was to store the original views in an array and return those in didSelectItem and didDeselectItem, using the index parameter. That seems to be working fine on my initial tests, but I will try to come up with a better solution later today anyway, and comment back here.

sunshinejr commented 8 years ago

@irodrigo17 Perfect! Looking for that one! πŸŽ‰

sunshinejr commented 8 years ago

Hey, I did PR #14 with factory and update on view copying, I'd love if you could check this out @irodrigo17, @aocenas. πŸŽ‰

irodrigo17 commented 8 years ago

@sunshinejr Hey! Sorry for the delay, I have been really busy these days. Checking it out now, thanks a lot!

sunshinejr commented 8 years ago

This one could be closed now, for reference check #14, #16, #17.