0

I have this code, taken from this answer: https://stackoverflow.com/a/29099066/406322

extension NSNotificationCenter {
    func setObserver(observer: AnyObject, selector: Selector, name: String?, object: AnyObject?) {
        NSNotificationCenter.defaultCenter().removeObserver(observer, name: name, object: object)
        NSNotificationCenter.defaultCenter().addObserver(observer, selector: selector, name: name, object: object)
    }
}

Now, in my view controller, I am setting my observers in viewDidLoad():

override func viewDidLoad() {
        super.viewDidLoad()
        setObservers()
}

func setObservers() {
        NSNotificationCenter.defaultCenter().setObserver(self, selector: #selector(BaseController.handleComment(_:)), name: "newComment", object: nil)
}

However, even with using this extension, where the observer is removed before getting added, each time I exit the view controller, and return to it, I get multiple notifications (one extra each time).

How is this possible?

Community
  • 1
  • 1
Prabhu
  • 12,995
  • 33
  • 127
  • 210
  • Are you sure the old view controller is being deallocated? Most likely the extra notifications are going to the previous instances of the view controller. – rmaddy Dec 12 '16 at 17:05
  • How do i deallocate them, sorry a beginner t Swift here. – Prabhu Dec 12 '16 at 17:08

1 Answers1

1

If you need this setObserver extension, you are very likely doing something wrong. You should be able to balance your registration and removal easily. If you can't, your notification management is very likely too complicated or in the wrong place.

Typically the correct place to add observations is in viewWillAppear (or viewDidAppear, either is fine), and remove them in viewDidDisappear (or viewWillDisappear). This ensures that you do not receive notifications while you are offscreen, even if the view controller still exists (which is common).

If your view controller requires that it receive notifications while it is offscreen, then you have a design problem. View controllers should only manage onscreen views. If they're doing anything else, you have put too much of the model into the controller.

As @rmaddy notes, your specific problem is likely that you have two instances of this view controller. That's may be fine or it might be a mistake (it depends on how the view controller works). But if you balance adding and removing your registration when going on and offscreen, that part will be fine.

Rob Napier
  • 286,113
  • 34
  • 456
  • 610
  • Thank you, that makes sense, will try moving them to viewWillAppear/viewWillDisappear. As I'm new to Swift, I'm trying to understand, how there could be two instances of the ViewController. In my situation, I have a close button on this view controller, which does this navigationController?.popViewControllerAnimated(true), and then in the parent view, there is a button that segues back to this view controller. Could that be the reason? Is that bad design, and should I be deallocating something each time I popout? – Prabhu Dec 12 '16 at 17:30
  • And yes moving them to viewWillAppear fixed the issue. – Prabhu Dec 12 '16 at 17:36
  • 1
    Keep in mind you may still have a problem with extra view controllers. Verify that `deinit` is being called when a view controller is dismissed. – rmaddy Dec 12 '16 at 17:49
  • Your segue scheme sounds fine. I would expect a retain loop somewhere that is leaking your view controller. Something like a repeating NSTimer (very common cause of retain loops on view controllers) or a closure that strongly captures `self`. – Rob Napier Dec 12 '16 at 18:23
  • I do have a timer in the controller I'm segueing from, and some "self" references in closures in this view controller. Is that not good practice? Should I convert all of them to weak self? – Prabhu Dec 13 '16 at 17:27
  • Repeating timers that reference self absolutely create a retain loop. You have to invalidate them when you go off screen. Closures may or may not; it depends on whether something holds onto the closure that self is holding onto. See http://stackoverflow.com/a/26935314/97337 – Rob Napier Dec 13 '16 at 18:58