diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift b/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift index dfb43c5d0..2727f860f 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift @@ -13,19 +13,17 @@ class GifPickerCell: UICollectionViewCell { didSet { AssertIsOnMainThread() - ensureLoad() + ensureCellState() } } // Loading and playing GIFs is quite expensive (network, memory, cpu). // Here's a bit of logic to not preload offscreen cells that are prefetched. - // - // TODO: Go a step farther and stop playback of cells that scroll offscreen. - var shouldLoad = false { + var isCellVisible = false { didSet { AssertIsOnMainThread() - ensureLoad() + ensureCellState() } } @@ -53,7 +51,7 @@ class GifPickerCell: UICollectionViewCell { super.prepareForReuse() imageInfo = nil - shouldLoad = false + isCellVisible = false stillAsset = nil stillAssetRequest?.cancel() stillAssetRequest = nil @@ -74,18 +72,22 @@ class GifPickerCell: UICollectionViewCell { fullAssetRequest = nil } - private func clearAssetRequest() { + private func clearAssetRequests() { clearStillAssetRequest() clearFullAssetRequest() } - private func ensureLoad() { - guard shouldLoad else { - clearAssetRequest() + private func ensureCellState() { + guard isCellVisible else { + // Cancel any outstanding requests. + clearAssetRequests() + // Clear image view so we don't animate offscreen GIFs. + imageView?.removeFromSuperview() + imageView = nil return } guard let imageInfo = imageInfo else { - clearAssetRequest() + clearAssetRequests() return } guard self.fullAsset == nil else { @@ -95,41 +97,58 @@ class GifPickerCell: UICollectionViewCell { // It's critical that we carefully "pick" the best rendition to use. guard let fullRendition = imageInfo.pickGifRendition() else { Logger.warn("\(TAG) could not pick gif rendition: \(imageInfo.giphyId)") - clearAssetRequest() + clearAssetRequests() return } guard let stillRendition = imageInfo.pickStillRendition() else { Logger.warn("\(TAG) could not pick still rendition: \(imageInfo.giphyId)") - clearAssetRequest() + clearAssetRequests() return } if stillAsset == nil && fullAsset == nil && stillAssetRequest == nil { stillAssetRequest = GifDownloader.sharedInstance.requestAsset(rendition:stillRendition, priority:.high, - success: { [weak self] asset in + success: { [weak self] assetRequest, asset in guard let strongSelf = self else { return } + if assetRequest != nil && assetRequest != strongSelf.stillAssetRequest { + // Ignore obsolete requests. + return + } strongSelf.clearStillAssetRequest() strongSelf.stillAsset = asset strongSelf.tryToDisplayAsset() }, - failure: { [weak self] in + failure: { [weak self] assetRequest in guard let strongSelf = self else { return } + if assetRequest != strongSelf.stillAssetRequest { + // Ignore obsolete requests. + return + } strongSelf.clearStillAssetRequest() }) } if fullAsset == nil && fullAssetRequest == nil { fullAssetRequest = GifDownloader.sharedInstance.requestAsset(rendition:fullRendition, priority:.low, - success: { [weak self] asset in + success: { [weak self] assetRequest, asset in guard let strongSelf = self else { return } - strongSelf.clearAssetRequest() + if assetRequest != nil && assetRequest != strongSelf.fullAssetRequest { + // Ignore obsolete requests. + return + } + // If we have the full asset, we don't need the still asset. + strongSelf.clearAssetRequests() strongSelf.fullAsset = asset strongSelf.tryToDisplayAsset() }, - failure: { [weak self] in + failure: { [weak self] assetRequest in guard let strongSelf = self else { return } - strongSelf.clearAssetRequest() + if assetRequest != strongSelf.fullAssetRequest { + // Ignore obsolete requests. + return + } + strongSelf.clearFullAssetRequest() }) } } diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift index 5145c9fdd..5c47e594e 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift @@ -177,7 +177,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect return } // We only want to load the cells which are on-screen. - cell.shouldLoad = true + cell.isCellVisible = true } public func collectionView(_ collectionView: UICollectionView, didEndDisplaying cell: UICollectionViewCell, forItemAt indexPath: IndexPath) { @@ -185,7 +185,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect owsFail("\(TAG) unexpected cell.") return } - cell.shouldLoad = false + cell.isCellVisible = false } // MARK: - Event Handlers diff --git a/Signal/src/network/GifDownloader.swift b/Signal/src/network/GifDownloader.swift index 1d1c93ba1..2c0b19a63 100644 --- a/Signal/src/network/GifDownloader.swift +++ b/Signal/src/network/GifDownloader.swift @@ -21,8 +21,8 @@ enum GiphyRequestPriority { // Exactly one of success or failure should be called once, // on the main thread _unless_ this request is cancelled before // the request succeeds or fails. - private var success: ((GiphyAsset) -> Void) - private var failure: (() -> Void) + private var success: ((GiphyAssetRequest?, GiphyAsset) -> Void)? + private var failure: ((GiphyAssetRequest) -> Void)? var wasCancelled = false // This property is an internal implementation detail of the download process. @@ -30,8 +30,8 @@ enum GiphyRequestPriority { init(rendition: GiphyRendition, priority: GiphyRequestPriority, - success:@escaping ((GiphyAsset) -> Void), - failure:@escaping (() -> Void) + success:@escaping ((GiphyAssetRequest?, GiphyAsset) -> Void), + failure:@escaping ((GiphyAssetRequest) -> Void) ) { self.rendition = rendition self.priority = priority @@ -51,17 +51,14 @@ enum GiphyRequestPriority { private func clearCallbacks() { AssertIsOnMainThread() - // Replace success and failure with no-ops. - success = { _ in - } - failure = { - } + success = nil + failure = nil } public func requestDidSucceed(asset: GiphyAsset) { AssertIsOnMainThread() - success(asset) + success?(self, asset) // Only one of the callbacks should be called, and only once. clearCallbacks() @@ -70,7 +67,7 @@ enum GiphyRequestPriority { public func requestDidFail() { AssertIsOnMainThread() - failure() + failure?(self) // Only one of the callbacks should be called, and only once. clearCallbacks() @@ -212,17 +209,19 @@ extension URLSessionTask { private let kMaxAssetRequestCount = 3 private var activeAssetRequests = Set() - // The success and failure handlers are always called on main queue. - // The success handler may be called synchronously on cache hit. + // The success and failure callbacks are always called on main queue. + // + // The success callbacks may be called synchronously on cache hit, in + // which case the GiphyAssetRequest parameter will be nil. public func requestAsset(rendition: GiphyRendition, priority: GiphyRequestPriority, - success:@escaping ((GiphyAsset) -> Void), - failure:@escaping (() -> Void)) -> GiphyAssetRequest? { + success:@escaping ((GiphyAssetRequest?, GiphyAsset) -> Void), + failure:@escaping ((GiphyAssetRequest) -> Void)) -> GiphyAssetRequest? { AssertIsOnMainThread() if let asset = assetMap.get(key:rendition.url) { // Synchronous cache hit. - success(asset) + success(nil, asset) return nil }