0

In my subclass of UIImageView, I have a UIActivityIndicatorView and a function to download images defined as so:

class FooUIImageView: UIImageView {

        var activityIndicator: UIActivityIndicatorView {
            let activityIndicator = UIActivityIndicatorView(activityIndicatorStyle: .white)
            activityIndicator.hidesWhenStopped = true
            activityIndicator.center = CGPoint(x: self.frame.width/2, y: self.frame.height/2)
            self.addSubview(activityIndicator)
            return activityIndicator
        }

        func downloadImageFrom(url: URL, imageMode: UIViewContentMode) {

            self.activityIndicator.startAnimating()
            self.activityIndicator.stopAnimating()

            contentMode = imageMode
            if let cachedImage = imageCache.object(forKey: url.absoluteString as NSString) as? UIImage {
                self.image = cachedImage
            } else {
                URLSession.shared.dataTask(with: url) { data, _, error in
                    guard let data = data, error == nil else { return }
                        DispatchQueue.main.async {
                            if let imageToCache = UIImage(data: data) {
                                self.activityIndicator.stopAnimating()
                                self.imageCache.setObject(imageToCache, forKey: url.absoluteString as NSString)
                                self.image = imageToCache
                            } else {
                                self.activityIndicator.stopAnimating()
                                self.image = nil
                            }
                        }
                }.resume()
            }

            activityIndicator.stopAnimating()
       }
}

When running my app, the activityIndicator starts loading - then the image is eventually displayed. However, the activityIndicator remains onscreen and never disappears. In none of the places where I call stopAnimating() above does the indicator actually stop animating. This is particularly confusing where I stop it right after calling it and yet it's still there.

Brandon Lee
  • 194
  • 2
  • 12
  • Run your code, wait until the image loads, and look at the view hierarchy in the debugger. Then change the property: `var activityIndicator: UIActivityIndicatorView = { // Same body }()` and look again. – jscs Oct 17 '17 at 23:50

1 Answers1

4

There are a few problems with your code. The biggest one is that you've made activityIndicator a computed property. Each time you reference it, your closure code is executed and you get a new, never-before-seen activity indicator. Try this code as a test:

print(String(format: "activityIndicator address = %X", activityIndicator))
print(String(format: "activityIndicator address = %X", activityIndicator))
print(String(format: "activityIndicator address = %X", activityIndicator))

Note that you get a different address for each print statement.

That means that EVERY time you reference activityIndicator or self.activityIndicator, you create yet another activity indicator and add it to your view hierarchy. That's not what you want.

Instead, make your variable a lazy var:

lazy var activityIndicator: UIActivityIndicatorView = {
    let activityIndicator = UIActivityIndicatorView(activityIndicatorStyle: .white)
    activityIndicator.hidesWhenStopped = true
    activityIndicator.center = CGPoint(x: self.frame.width/2, y: self.frame.height/2)
    self.addSubview(activityIndicator)
    return activityIndicator
}()

Note how that code has an assignment, and that the closure has parentheses afterwords (()).

The lazy qualifier means that the variable doesn't get created until the first time it's referenced, and then only gets created once.

The parens after the closure mean that the closure gets called like a function, and that the variable activityIndicator will contain the results of that closure.

You should also get rid of your stopAnimating() calls everywhere but inside the call to DispatchQueue.main.async inside your dataTasks completion handler. You only want to stop animating once the download completes, and you must make a UI call like that from the main thread. Extra calls to stopAnimating() are just going to cause you problems.

Duncan C
  • 128,072
  • 22
  • 173
  • 272
  • Aww, I wanted him to figure out the multiple instances on his own. :) (Note that `lazy` is not strictly necessary. That part would be resolved without it. Plus, you could then make it a constant.) – jscs Oct 18 '17 at 00:28
  • For a beginning developer "look at the view hierarchy in the debugger" may not mean anything. Subtle hints tend to get lost on newbies. – Duncan C Oct 18 '17 at 01:27
  • Well, the top Google hit for "look at the view hierarchy in the debugger" is [Apple's Debugging guide](https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/debugging_with_xcode/chapters/special_debugging_workflows.html). Then there's a Ray Wenderlich guide, followed by several Stack Overflow questions that explain it really well. [1](https://stackoverflow.com/q/5150186/) [2](https://stackoverflow.com/q/24728326/) So I think that's a pretty reasonable hint, actually. – jscs Oct 18 '17 at 01:47
  • ¯\_(ツ)_/¯ Better long-term that there's a good answer attached. – jscs Oct 18 '17 at 17:07
  • Thanks, this helps a ton. Correct me if I'm wrong but one thing I also might want to add is `[unowned self]` into the closure to avoid a strong reference cycle since I'm using a class rather than a struct? – Brandon Lee Oct 18 '17 at 17:26
  • Yes, the completion handler for your data task is an escaping closure, so doing the weak self/strongSelf capture group dance would be a good idea. I'd suggest `[weak self]` and `guard strongSelf = self else { return }`. – Duncan C Oct 18 '17 at 17:55