The Apple documentation for scheduledTimer says:

target
The object to which to send the message specified by aSelector when the timer fires. The timer maintains a strong reference to target until it (the timer) is invalidated.

As the Timer is holding a strong reference to the InspectorWindowController, that it is not being invalidated is definitely a defect that must be fixed. Good catch!

The Apple documentation for addObserver says:

If your app targets iOS 9.0 and later or macOS 10.11 and later, you do not need to unregister an observer that you created with this function. If you forget or are unable to remove an observer, the system cleans up the next time it would have posted to it.

Are we sure the code to remove the observers is required? I'm thinking it is only the Timer that is the problem?

If we do want to retain the code to remove the observers, then can't we simplify it and always use NotificationCenter.default.removeObserver(self) instead of keeping each observer in an array and removing each individually?

Are we sure the code to remove the observers is required? I'm thinking it is only the Timer that is the problem?

I do think this is correct. This PR more than just resource cleanup. It's also about improving efficiency by cutting down on wasted work. The Timer, if not stopped, will continue to call updateInfo() once per second until IINA is shut down. (I was able to confirm this by adding a simple log statement to the start of updateInfo()). And as far as I can tell, updateInfo() is doing just as much work with the window closed as with it open. It will still recalculate layout and redraw of views. And there are a bunch of calls to query from mpv.

The listeners are not as bad, since they will only do work when iinaFileLoaded (new file started) or iinaMainWindowChanged (player window gains or loses focus) are fired. But any work they do is completely unnecessary while the Inspector window is closed, and they do slightly more work with the call to updateInfo(dynamic: false).

If we do want to retain the code to remove the observers, then can't we simplify it and always use NotificationCenter.default.removeObserver(self) instead of keeping each observer in an array and removing each individually?

I have never gotten a firm grasp on how observers are supposed to work. I suspect there are many variations. And I seem to be really terrible at finding documentation on this stuff. Thanks for the note on addObserver since I somehow missed that. So it looks like I can safely remove the code from deinit. This will only be called at application termination anyway.

...However, I tried replacing the loop in removeTimerAndListeners() with NotificationCenter.default.removeObserver(self) , so it looked like this:

  private func removeTimerAndListeners() {
    updateTimer?.invalidate()
    updateTimer = nil

    NotificationCenter.default.removeObserver(self)
  }

Very strange behavior ensued. Somehow, the new call prevented the Timer from invalidating. It continued firing on schedule even after the Inspector was closed - even though the call to invalidate() happened before the call to removeObserver. Then I tried another run, after just commenting out theremoveObserver line so that only the Timer stuff remained. In this case, the Timer was successfully stopped, which confirms it was removeObserver causing the interference. (And of course this didn't prevent updateInfo() continued to be called from the two observers.)

So... as far as I can tell, the observers array is still needed. It would be a nice refactor task to figure out how to pull out all this observer boilerplate code so it doesn't need to be repeated all over the place.

That certainly is not expected behavior. Thank you AppKit!

We can still simplify this and remove the need for the observers array by using the removeObserver(_:name:object:) method:

NotificationCenter.default.removeObserver(self, name: .iinaFileLoaded, object: nil)
NotificationCenter.default.removeObserver(self, name: .iinaMainWindowChanged, object: nil)

That should avoid whatever this interference with Timer is.

If there were a lot of observers I'd be inclined to go with the array. With only two, I'm kind of preferring this way. What do you think?

I tried this:

NotificationCenter.default.removeObserver(self, name: .iinaFileLoaded, object: nil)
NotificationCenter.default.removeObserver(self, name: .iinaMainWindowChanged, object: nil)

They did not interfere with the Timer, but they did not detach the listeners.

Looks to me like Apple's documentation is buggy as the doc for addObserver(forName:object:queue:using:) says:

To unregister an observer, use removeObserver(:) or removeObserver(:name:object:) with the most specific detail possible.

Seems like a cut-n-paste error as apparently removeObserver(_:name:object:) can't be used if the observer was added with that particular add method.

Closed source APIs like this are irritating. No way to tell if removeObserver found and removed an observer. No way to iterate over observers.