playerDestructionBlock documentation is misleading

[tooolbox]

Issue overview

• [X] Incorrect behavior
• [ ] Feature request
• [ ] Crash
• [X] Help / documentation
• [ ] Other: classification

Description of the problem

Per the documentation, playerDestructionBlock is called before the player is nil'd out, as an opportunity to remove observers:

Three kinds of observers can be set on a player to observe its playback:

• Usual boundary time and periodic time observers, which you define on the AVPlayer instance directly by accessing the player property. You should use the player creation and destruction blocks to install and remove them reliably.

  
  /**

• Optional block which gets called right before player destruction (player changes from not nil to nil). */ @property (nonatomic, copy, nullable) void (^playerDestructionBlock)(void);

However, the player destruction block is called after the player has been replaced:

- (void)setPlayer:(AVPlayer *)player
{
    BOOL hadPlayer = (_player != nil);

    if (_player) {
        // un-register internal observers
    }

    _player = player; // player replaced

    if (player) {
        if (! hadPlayer) {
            self.playerCreationBlock ? self.playerCreationBlock(player) : nil;
        }

        // various stuff

    }
    else if (hadPlayer) {
        self.playerDestructionBlock ? self.playerDestructionBlock() : nil; // destruction block called
    }
}

Since the player isn't passed as an argument in the destruction block, and it's un-set by the time the block is called, a consumer of the library must create his own reference to that player in the creation block and use that reference to remove observers in the destruction block.

let mediaPlayer = SRGMediaPlayerController()
var timeObserver: Any?
mediaPlayer.playerCreationBlock = { player in
    timeObserver = player.addPeriodicTimeObserver(...)
}
mediaPlayer.playerDestructionBlock = { // no player available
    if let obs = timeObserver {
        mediaPlayer.player?.removePeriodicTimeObserver(obs) // <-- doesn't work, player is nil here
    }
}

You have to do this:

let mediaPlayer = SRGMediaPlayerController()
var timeObserver: Any?
var savedPlayer: AVPlayer?
mediaPlayer.playerCreationBlock = { player in 
    timeObserver = player.addPeriodicTimeObserver(...)
    savedPlayer = player
}
mediaPlayer.playerDestructionBlock = {
    if let obs = timeObserver {
        savedPlayer?.removePeriodicTimeObserver(obs)
    }
    savedPlayer = nil
}

Reproducibility

• [X] Always reproducible
• [ ] Difficult to reproduce

Suggested Handling

Call the playerDestructionBlock just before reassigning that value, and include the old player as an argument in the block. This will allow someone using SRGMediaPlayer to add & remove observers from the AVPlayer reliably and in a straightforward fashion.

Like so:

let mediaPlayer = SRGMediaPlayerController()
var timeObserver: Any?
mediaPlayer.playerCreationBlock = { player in 
    timeObserver = player.addPeriodicTimeObserver(...)
}
mediaPlayer.playerDestructionBlock = { player in
    if let obs = timeObserver {
        player.removePeriodicTimeObserver(obs)
    }
}

[defagos]

PR #47 has been merged into develop. The improvement will be available in version 2.3.

[tooolbox]

Yay!