Allow multiple simultaneous GIF downloads.

// FREEBIE

Podfile.lock

@@ -134,7 +134,7 @@ EXTERNAL SOURCES:
   OpenSSL:
     :git: https://github.com/WhisperSystems/OpenSSL-Pod
   SignalServiceKit:
-    :path: "."
+    :path: .
   SocketRocket:
     :git: https://github.com/facebook/SocketRocket.git
 
@@ -176,6 +176,6 @@ SPEC CHECKSUMS:
   YapDatabase: cd911121580ff16675f65ad742a9eb0ab4d9e266
   YYImage: 1e1b62a9997399593e4b9c4ecfbbabbf1d3f3b54
 
-PODFILE CHECKSUM: '00831faaa7677029090c311c00ceadaa44f65c0f'
+PODFILE CHECKSUM: 00831faaa7677029090c311c00ceadaa44f65c0f
 
 COCOAPODS: 1.2.1

Signal/src/ViewControllers/GifPicker/GifPickerCell.swift

@@ -17,6 +17,10 @@ class GifPickerCell: UICollectionViewCell {
         }
     }
 
+    // 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 {
         didSet {
             AssertIsOnMainThread()
@@ -25,6 +29,8 @@ class GifPickerCell: UICollectionViewCell {
         }
     }
 
+    // We do "progressive" loading by loading stills (jpg or gif) and "full" gifs. 
+    // This is critical on cellular connections.
     var stillAssetRequest: GiphyAssetRequest?
     var stillAsset: GiphyAsset?
     var fullAssetRequest: GiphyAssetRequest?
@@ -85,6 +91,8 @@ class GifPickerCell: UICollectionViewCell {
         guard self.fullAsset == nil else {
             return
         }
+        // The Giphy API returns a slew of "renditions" for a given image. 
+        // 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()

Signal/src/ViewControllers/GifPicker/GifPickerLayout.swift

@@ -8,6 +8,7 @@ protocol GifPickerLayoutDelegate: class {
     func imageInfosForLayout() -> [GiphyImageInfo]
 }
 
+// A Pinterest-style waterfall layout.
 class GifPickerLayout: UICollectionViewLayout {
     let TAG = "[GifPickerLayout]"
 
@@ -69,16 +70,17 @@ class GifPickerLayout: UICollectionViewLayout {
         // due to rounding error.
         let totalHSpacing = totalViewWidth - ((2 * hMargin) + (columnCount * columnWidth))
 
+        // columnXs are the left edge of each column.
         var columnXs = [UInt]()
+        // columnYs are the top edge of the next cell in each column.
         var columnYs = [UInt]()
-        // Note that columnIndex is a one-based index, not a zero-based index.
-        for columnIndex in 1...columnCount {
-            var columnX = hMargin + (columnWidth * (columnIndex - 1))
+        for columnIndex in 0...columnCount-1 {
+            var columnX = hMargin + (columnWidth * columnIndex)
             if columnCount > 1 {
                 // We want to unevenly distribute the hSpacing between the columns
                 // so that the left and right margins are equal, which is non-trivial
                 // due to rounding error.
-                columnX += ((totalHSpacing * (columnIndex - 1)) / (columnCount - 1))
+                columnX += ((totalHSpacing * columnIndex) / (columnCount - 1))
             }
             columnXs.append(columnX)
             columnYs.append(vMargin)
@@ -86,6 +88,7 @@ class GifPickerLayout: UICollectionViewLayout {
 
         // Always layout all items.
         let imageInfos = delegate.imageInfosForLayout()
+        var contentBottom = vMargin
         for (cellIndex, imageInfo) in imageInfos.enumerated() {
             // Select a column by finding the "highest, leftmost" column.
             var column = 0
@@ -107,13 +110,11 @@ class GifPickerLayout: UICollectionViewLayout {
             itemAttributesMap[UInt(cellIndex)] = itemAttributes
 
             columnYs[column] = cellY + cellHeight + vSpacing
+            contentBottom = max(contentBottom, cellY + cellHeight)
         }
 
-        var contentHeight = vMargin
-        for columnY in columnYs {
-            contentHeight = max(contentHeight, columnY)
-        }
-        contentHeight += vMargin
+        // Add bottom margin.
+        let contentHeight = contentBottom + vMargin
         contentSize = CGSize(width:CGFloat(totalViewWidth), height:CGFloat(contentHeight))
     }
 

Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift

@@ -132,7 +132,6 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect
         return imageInfos.count
     }
 
-    // The cell that is returned must be retrieved from a call to -dequeueReusableCellWithReuseIdentifier:forIndexPath:
     public  func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
         let imageInfo = imageInfos[indexPath.row]
 

Signal/src/environment/NotificationsManager.m

@@ -90,12 +90,16 @@ NSString *const kNotificationsManagerNewMesssageSoundName = @"NewMessage.aifc";
  */
 - (void)presentMissedCall:(SignalCall *)call callerName:(NSString *)callerName
 {
+    TSContactThread *thread = [TSContactThread getOrCreateThreadWithContactId:call.remotePhoneNumber];
+    OWSAssert(thread != nil);
+
     UILocalNotification *notification = [UILocalNotification new];
     notification.category = PushManagerCategoriesMissedCall;
     NSString *localCallId = call.localId.UUIDString;
     notification.userInfo = @{
         PushManagerUserInfoKeysLocalCallId : localCallId,
-        PushManagerUserInfoKeysCallBackSignalRecipientId : call.remotePhoneNumber
+        PushManagerUserInfoKeysCallBackSignalRecipientId : call.remotePhoneNumber,
+        Signal_Thread_UserInfo_Key : thread.uniqueId
     };
 
     if ([self shouldPlaySoundForNotification]) {

Signal/src/network/GifDownloader.swift

@@ -9,14 +9,22 @@ enum GiphyRequestPriority {
     case low, high
 }
 
+// Represents a request to download a GIF.
+//
+// Should be cancelled if no longer necessary.
 @objc class GiphyAssetRequest: NSObject {
     static let TAG = "[GiphyAssetRequest]"
 
     let rendition: GiphyRendition
     let priority: GiphyRequestPriority
-    let success: ((GiphyAsset) -> Void)
-    let failure: (() -> Void)
+    // 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)
+
     var wasCancelled = false
+    // This property is an internal implementation detail of the download process.
     var assetFilePath: String?
 
     init(rendition: GiphyRendition,
@@ -31,10 +39,48 @@ enum GiphyRequestPriority {
     }
 
     public func cancel() {
+        AssertIsOnMainThread()
+
         wasCancelled = true
+
+        // Don't call the callbacks if the request is cancelled.
+        clearCallbacks()
+    }
+
+    private func clearCallbacks() {
+        AssertIsOnMainThread()
+
+        // Replace success and failure with no-ops.
+        success = { _ in
+        }
+        failure = {
+        }
+    }
+
+    public func requestDidSucceed(asset: GiphyAsset) {
+        AssertIsOnMainThread()
+
+        success(asset)
+
+        // Only one of the callbacks should be called, and only once.
+        clearCallbacks()
+    }
+
+    public func requestDidFail() {
+        AssertIsOnMainThread()
+
+        failure()
+
+        // Only one of the callbacks should be called, and only once.
+        clearCallbacks()
     }
 }
 
+// Represents a downloaded gif asset.
+//
+// The blob on disk is cleaned up when this instance is deallocated,
+// so consumers of this resource should retain a strong reference to
+// this instance as long as they are using the asset.
 @objc class GiphyAsset: NSObject {
     static let TAG = "[GiphyAsset]"
 
@@ -48,6 +94,7 @@ enum GiphyRequestPriority {
     }
 
     deinit {
+        // Clean up on the asset on disk.
         let filePathCopy = filePath
         DispatchQueue.global().async {
             do {
@@ -60,6 +107,7 @@ enum GiphyRequestPriority {
     }
 }
 
+// A simple LRU cache bounded by the number of entries.
 class LRUCache<KeyType: Hashable & Equatable, ValueType> {
 
     private var cacheMap = [KeyType: ValueType]()
@@ -102,6 +150,7 @@ class LRUCache<KeyType: Hashable & Equatable, ValueType> {
 
 private var URLSessionTask_GiphyAssetRequest: UInt8 = 0
 
+// This extension is used to punch an asset request onto a download task.
 extension URLSessionTask {
     var assetRequest: GiphyAssetRequest {
         get {
@@ -121,6 +170,7 @@ extension URLSessionTask {
 
     static let sharedInstance = GifDownloader()
 
+    // A private queue used for download task callbacks.
     private let operationQueue = OperationQueue()
 
     // Force usage as a singleton
@@ -147,10 +197,19 @@ extension URLSessionTask {
         return session
     }
 
+    // 100 entries of which at least half will probably be stills.  
+    // Actual animated GIFs will usually be less than 3 MB so the 
+    // max size of the cache on disk should be ~150 MB.  Bear in mind
+    // that assets are not always deleted on disk as soon as they are
+    // evacuated from the cache; if a cache consumer (e.g. view) is 
+    // still using the asset, the asset won't be deleted on disk until
+    // it is no longer in use.
     private var assetMap = LRUCache<NSURL, GiphyAsset>(maxSize:100)
-    // TODO: We could use a proper queue.
+    // TODO: We could use a proper queue, e.g. implemented with a linked
+    // list.
     private var assetRequestQueue = [GiphyAssetRequest]()
-    private var isDownloading = false
+    private let kMaxAssetRequestCount = 3
+    private var activeAssetRequests = Set<GiphyAssetRequest>()
 
     // The success and failure handlers are always called on main queue.
     // The success and failure handlers may be called synchronously on cache hit.
@@ -165,69 +224,60 @@ extension URLSessionTask {
             return nil
         }
 
-        var hasRequestCompleted = false
         let assetRequest = GiphyAssetRequest(rendition:rendition,
                                              priority:priority,
-                                             success : { asset in
-                                                DispatchQueue.main.async {
-                                                    // Ensure we call success or failure exactly once.
-                                                    guard !hasRequestCompleted else {
-                                                        return
-                                                    }
-                                                    hasRequestCompleted = true
-
-                                                    self.assetMap.set(key:rendition.url, value:asset)
-                                                    self.isDownloading = false
-                                                    self.downloadIfNecessary()
-                                                    success(asset)
-                                                }
-        },
-                                             failure : {
-                                                DispatchQueue.main.async {
-                                                    // Ensure we call success or failure exactly once.
-                                                    guard !hasRequestCompleted else {
-                                                        return
-                                                    }
-                                                    hasRequestCompleted = true
-
-                                                    self.isDownloading = false
-                                                    self.downloadIfNecessary()
-                                                    failure()
-                                                }
-        })
+                                             success : success,
+                                             failure : failure)
         assetRequestQueue.append(assetRequest)
         downloadIfNecessary()
         return assetRequest
     }
 
+    private func assetRequestDidSucceed(assetRequest: GiphyAssetRequest, asset: GiphyAsset) {
+        DispatchQueue.main.async {
+            self.assetMap.set(key:assetRequest.rendition.url, value:asset)
+            self.activeAssetRequests.remove(assetRequest)
+            assetRequest.requestDidSucceed(asset:asset)
+            self.downloadIfNecessary()
+        }
+    }
+
+    private func assetRequestDidFail(assetRequest: GiphyAssetRequest) {
+        DispatchQueue.main.async {
+            self.activeAssetRequests.remove(assetRequest)
+            assetRequest.requestDidFail()
+            self.downloadIfNecessary()
+        }
+    }
+
     private func downloadIfNecessary() {
         AssertIsOnMainThread()
 
         DispatchQueue.main.async {
-            guard !self.isDownloading else {
+            guard self.activeAssetRequests.count < self.kMaxAssetRequestCount else {
                 return
             }
             guard let assetRequest = self.popNextAssetRequest() else {
                 return
             }
             guard !assetRequest.wasCancelled else {
-                DispatchQueue.main.async {
-                    self.downloadIfNecessary()
-                }
+                // Discard the cancelled asset request and try again.
+                self.downloadIfNecessary()
                 return
             }
-            self.isDownloading = true
+            self.activeAssetRequests.insert(assetRequest)
 
             if let asset = self.assetMap.get(key:assetRequest.rendition.url) {
                 // Deferred cache hit, avoids re-downloading assets already in the
                 // asset cache.
-                assetRequest.success(asset)
+
+                self.assetRequestDidSucceed(assetRequest : assetRequest, asset: asset)
                 return
             }
 
             guard let downloadSession = self.giphyDownloadSession() else {
                 Logger.error("\(GifDownloader.TAG) Couldn't create session manager.")
-                assetRequest.failure()
+                self.assetRequestDidFail(assetRequest:assetRequest)
                 return
             }
 
@@ -266,32 +316,32 @@ extension URLSessionTask {
         let assetRequest = task.assetRequest
         guard !assetRequest.wasCancelled else {
             task.cancel()
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
         if let error = error {
             Logger.error("\(GifDownloader.TAG) download failed with error: \(error)")
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
         guard let httpResponse = task.response as? HTTPURLResponse else {
             Logger.error("\(GifDownloader.TAG) missing or unexpected response: \(task.response)")
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
         let statusCode = httpResponse.statusCode
         guard statusCode >= 200 && statusCode < 400 else {
             Logger.error("\(GifDownloader.TAG) response has invalid status code: \(statusCode)")
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
         guard let assetFilePath = assetRequest.assetFilePath else {
             Logger.error("\(GifDownloader.TAG) task is missing asset file")
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
         let asset = GiphyAsset(rendition: assetRequest.rendition, filePath : assetFilePath)
-        assetRequest.success(asset)
+        assetRequestDidSucceed(assetRequest : assetRequest, asset: asset)
     }
 
     // MARK: URLSessionDownloadDelegate
@@ -300,7 +350,7 @@ extension URLSessionTask {
         let assetRequest = downloadTask.assetRequest
         guard !assetRequest.wasCancelled else {
             downloadTask.cancel()
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
 
@@ -321,7 +371,7 @@ extension URLSessionTask {
         let assetRequest = downloadTask.assetRequest
         guard !assetRequest.wasCancelled else {
             downloadTask.cancel()
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
     }
@@ -330,7 +380,7 @@ extension URLSessionTask {
         let assetRequest = downloadTask.assetRequest
         guard !assetRequest.wasCancelled else {
             downloadTask.cancel()
-            assetRequest.failure()
+            assetRequestDidFail(assetRequest:assetRequest)
             return
         }
     }

Signal/src/network/PushManager.m

@@ -134,7 +134,7 @@ NSString *const Signal_Message_MarkAsRead_Identifier = @"Signal_Message_MarkAsRe
 - (void)application:(UIApplication *)application didReceiveLocalNotification:(UILocalNotification *)notification {
     DDLogInfo(@"received: %s", __PRETTY_FUNCTION__);
 
-    NSString *threadId = notification.userInfo[Signal_Thread_UserInfo_Key];
+    NSString *_Nullable threadId = notification.userInfo[Signal_Thread_UserInfo_Key];
 
     if (threadId) {
         [Environment presentConversationForThreadId:threadId];