cbpowell / MarqueeLabel

A drop-in replacement for UILabel, which automatically adds a scrolling marquee effect when the label's text does not fit inside the specified frame
MIT License
4.19k stars 561 forks source link

Possible Bug: didMoveToWindow should call removeObserver from NotificationCenter when self.window is nil #252

Closed ussu99 closed 3 years ago

ussu99 commented 4 years ago

Usage Details

Expected Behavior

Function didMoveToWindow should Remove from Notification center if window is nil!

 override open func didMoveToWindow() {
        if self.window == nil {
            shutdownLabel()
        } else {
            updateAndScroll()
        }
    }

Suggested Code

 override open func didMoveToWindow() {
        if self.window == nil {
            NotificationCenter.default.removeObserver(self) // < ----- Label was removed so no Notification
            shutdownLabel()
        } else {
            updateAndScroll()
        }
    }

Currently deinit is never called and therefore no remove from NotificationCenter occurs.

cbpowell commented 4 years ago

Thanks for the suggestion, and sorry it took so long for me to get on this!

So what are the effects of not removing the label as an observer? The label does check if window is nil in the labelReadyForScroll() function before starting a scroll animation, so the notifications shouldn't attempt to fire off animations without a window. Also the class functions that trigger the notifications require the controller to be passed in, and if the label has been removed from a window and responder chain I don't think it will "match" on the passed-in controller (but that may not always be true if you have multiple view controllers in the hierarchy).

If the label is being otherwise retained but has been removed from the window, from my (admittedly limited) testing it seems like there's basically no effect. If the label is subsequently re-added to a window, we would need a way to also re-subscribe to the Notification Center. Certainly doable, but would require changing up when the label subscribes in the code.

cbpowell commented 3 years ago

Closing this due to inactivity.