From 8b7d34e51c7830bf5a792eeb4f66ebe6a0540604 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 2 Oct 2017 15:24:57 -0400 Subject: [PATCH] Respond to CR. // FREEBIE --- .../GifPicker/GifPickerCell.swift | 52 +++++------ .../GifPicker/GifPickerLayout.swift | 36 ++++--- .../GifPicker/GifPickerViewController.swift | 11 ++- Signal/src/network/GiphyAPI.swift | 93 ++++++++++--------- Signal/src/network/GiphyDownloader.swift | 2 +- .../translations/en.lproj/Localizable.strings | 4 +- 6 files changed, 99 insertions(+), 99 deletions(-) diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift b/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift index 7c95d4738..d3712c5d8 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift @@ -27,19 +27,19 @@ class GifPickerCell: UICollectionViewCell { } } - // We do "progressive" loading by loading stills (jpg or gif) and "full" gifs. + // We do "progressive" loading by loading stills (jpg or gif) and "animated" gifs. // This is critical on cellular connections. var stillAssetRequest: GiphyAssetRequest? var stillAsset: GiphyAsset? - var fullAssetRequest: GiphyAssetRequest? - var fullAsset: GiphyAsset? + var animatedAssetRequest: GiphyAssetRequest? + var animatedAsset: GiphyAsset? var imageView: YYAnimatedImageView? // MARK: Initializers deinit { stillAssetRequest?.cancel() - fullAssetRequest?.cancel() + animatedAssetRequest?.cancel() } override func prepareForReuse() { @@ -50,9 +50,9 @@ class GifPickerCell: UICollectionViewCell { stillAsset = nil stillAssetRequest?.cancel() stillAssetRequest = nil - fullAsset = nil - fullAssetRequest?.cancel() - fullAssetRequest = nil + animatedAsset = nil + animatedAssetRequest?.cancel() + animatedAssetRequest = nil imageView?.removeFromSuperview() imageView = nil } @@ -62,14 +62,14 @@ class GifPickerCell: UICollectionViewCell { stillAssetRequest = nil } - private func clearFullAssetRequest() { - fullAssetRequest?.cancel() - fullAssetRequest = nil + private func clearanimatedAssetRequest() { + animatedAssetRequest?.cancel() + animatedAssetRequest = nil } private func clearAssetRequests() { clearStillAssetRequest() - clearFullAssetRequest() + clearanimatedAssetRequest() } public func ensureCellState() { @@ -84,12 +84,12 @@ class GifPickerCell: UICollectionViewCell { clearAssetRequests() return } - guard self.fullAsset == nil else { + guard self.animatedAsset == 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 { + guard let animatedRendition = imageInfo.pickAnimatedRendition() else { Logger.warn("\(TAG) could not pick gif rendition: \(imageInfo.giphyId)") clearAssetRequests() return @@ -101,7 +101,7 @@ class GifPickerCell: UICollectionViewCell { } // Start still asset request if necessary. - if stillAsset == nil && fullAsset == nil && stillAssetRequest == nil { + if stillAsset == nil && animatedAsset == nil && stillAssetRequest == nil { stillAssetRequest = GiphyDownloader.sharedInstance.requestAsset(rendition:stillRendition, priority:.high, success: { [weak self] assetRequest, asset in @@ -124,28 +124,28 @@ class GifPickerCell: UICollectionViewCell { }) } - // Start full asset request if necessary. - if fullAsset == nil && fullAssetRequest == nil { - fullAssetRequest = GiphyDownloader.sharedInstance.requestAsset(rendition:fullRendition, + // Start animated asset request if necessary. + if animatedAsset == nil && animatedAssetRequest == nil { + animatedAssetRequest = GiphyDownloader.sharedInstance.requestAsset(rendition:animatedRendition, priority:.low, success: { [weak self] assetRequest, asset in guard let strongSelf = self else { return } - if assetRequest != nil && assetRequest != strongSelf.fullAssetRequest { + if assetRequest != nil && assetRequest != strongSelf.animatedAssetRequest { owsFail("Obsolete request callback.") return } - // If we have the full asset, we don't need the still asset. + // If we have the animated asset, we don't need the still asset. strongSelf.clearAssetRequests() - strongSelf.fullAsset = asset + strongSelf.animatedAsset = asset strongSelf.tryToDisplayAsset() }, failure: { [weak self] assetRequest in guard let strongSelf = self else { return } - if assetRequest != strongSelf.fullAssetRequest { + if assetRequest != strongSelf.animatedAssetRequest { owsFail("Obsolete request callback.") return } - strongSelf.clearFullAssetRequest() + strongSelf.clearanimatedAssetRequest() }) } } @@ -173,12 +173,6 @@ class GifPickerCell: UICollectionViewCell { } private func pickBestAsset() -> GiphyAsset? { - if let fullAsset = fullAsset { - return fullAsset - } - if let stillAsset = stillAsset { - return stillAsset - } - return nil + return animatedAsset ?? stillAsset } } diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerLayout.swift b/Signal/src/ViewControllers/GifPicker/GifPickerLayout.swift index ac8410dbb..bebfbe5ec 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerLayout.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerLayout.swift @@ -54,31 +54,33 @@ class GifPickerLayout: UICollectionViewLayout { return } - let vMargin = UInt(5) - let hMargin = UInt(5) + let vInset = UInt(5) + let hInset = UInt(5) let vSpacing = UInt(3) let hSpacing = UInt(3) - // TODO: We probably want to use 2 or 3 columns. - // It might depend on the device. - // 2 columns will show fewer GIFs at a time, - // but use less network & be a more responsive experience. - let columnCount = UInt(3) + + // We use 2 or 3 columns, depending on the device. + // 2 columns will show fewer GIFs at a time, + // but use less network & be a more responsive experience. + let screenSize = UIScreen.main.bounds.size + let screenWidth = min(screenSize.width, screenSize.height) + let columnCount = UInt(max(2, screenWidth / 130)) let totalViewWidth = UInt(collectionView.width()) - let hTotalWhitespace = (2 * hMargin) + (hSpacing * (columnCount - 1)) + let hTotalWhitespace = (2 * hInset) + (hSpacing * (columnCount - 1)) let hRemainderSpace = totalViewWidth - hTotalWhitespace let columnWidth = UInt(hRemainderSpace / columnCount) // 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. - let totalHSpacing = totalViewWidth - ((2 * hMargin) + (columnCount * columnWidth)) + let totalHSpacing = totalViewWidth - ((2 * hInset) + (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]() for columnIndex in 0...columnCount-1 { - var columnX = hMargin + (columnWidth * columnIndex) + var columnX = hInset + (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 @@ -86,12 +88,12 @@ class GifPickerLayout: UICollectionViewLayout { columnX += ((totalHSpacing * columnIndex) / (columnCount - 1)) } columnXs.append(columnX) - columnYs.append(vMargin) + columnYs.append(vInset) } // Always layout all items. let imageInfos = delegate.imageInfosForLayout() - var contentBottom = vMargin + var contentBottom = vInset for (cellIndex, imageInfo) in imageInfos.enumerated() { // Select a column by finding the "highest, leftmost" column. var column = 0 @@ -117,18 +119,14 @@ class GifPickerLayout: UICollectionViewLayout { } // Add bottom margin. - let contentHeight = contentBottom + vMargin + let contentHeight = contentBottom + vInset contentSize = CGSize(width:CGFloat(totalViewWidth), height:CGFloat(contentHeight)) } override func layoutAttributesForElements(in rect: CGRect) -> [UICollectionViewLayoutAttributes]? { - var result = [UICollectionViewLayoutAttributes]() - for itemAttributes in itemAttributesMap.values { - if itemAttributes.frame.intersects(rect) { - result.append(itemAttributes) - } + return itemAttributesMap.values.filter { itemAttributes in + return itemAttributes.frame.intersects(rect) } - return result } override func layoutAttributesForItem(at indexPath: IndexPath) -> UICollectionViewLayoutAttributes? { diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift index b1dd2386d..3c9f74d20 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift @@ -98,7 +98,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect view.backgroundColor = UIColor.black - self.navigationItem.leftBarButtonItem = UIBarButtonItem(barButtonSystemItem:.stop, + self.navigationItem.leftBarButtonItem = UIBarButtonItem(barButtonSystemItem:.cancel, target:self, action:#selector(donePressed)) self.navigationItem.title = NSLocalizedString("GIF_PICKER_VIEW_TITLE", @@ -134,10 +134,12 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect searchBar.delegate = self searchBar.placeholder = NSLocalizedString("GIF_VIEW_SEARCH_PLACEHOLDER_TEXT", comment:"Placeholder text for the search field in gif view") + // Style the search bar so that it looks appropriate against a black background. searchBar.isTranslucent = false searchBar.backgroundImage = UIImage(color:UIColor.clear) searchBar.barTintColor = UIColor.black searchBar.tintColor = UIColor.white + self.view.addSubview(searchBar) searchBar.autoPinWidthToSuperview() searchBar.autoPin(toTopLayoutGuideOf: self, withInset:0) @@ -204,7 +206,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect owsFail("\(TAG) unexpected cell.") return } - guard let asset = cell.fullAsset else { + guard let asset = cell.animatedAsset else { Logger.info("\(TAG) unload cell selected.") return } @@ -213,7 +215,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect owsFail("\(TAG) couldn't load asset.") return } - let attachment = SignalAttachment(dataSource : dataSource, dataUTI: asset.rendition.utiType()) + let attachment = SignalAttachment(dataSource : dataSource, dataUTI: asset.rendition.utiType) guard let thread = thread else { owsFail("\(TAG) Missing thread.") return @@ -284,9 +286,10 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect strongSelf.imageInfos = imageInfos strongSelf.updateContents() }, - failure: { [weak self] in + failure: { [weak self] _ in guard let strongSelf = self else { return } Logger.info("\(strongSelf.TAG) search failed.") + // TODO: Present this error to the user. }) } diff --git a/Signal/src/network/GiphyAPI.swift b/Signal/src/network/GiphyAPI.swift index b3f5ca963..89723a9f1 100644 --- a/Signal/src/network/GiphyAPI.swift +++ b/Signal/src/network/GiphyAPI.swift @@ -3,7 +3,6 @@ // import Foundation -import ObjectiveC // There's no UTI type for webp! enum GiphyFormat { @@ -37,7 +36,7 @@ enum GiphyFormat { self.url = url } - public func fileExtension() -> String { + public var fileExtension: String { switch format { case .gif: return "gif" @@ -48,7 +47,7 @@ enum GiphyFormat { } } - public func utiType() -> String { + public var utiType: String { switch format { case .gif: return kUTTypeGIF as String @@ -59,6 +58,14 @@ enum GiphyFormat { } } + public var isStill: Bool { + return name.hasSuffix("_still") + } + + public var isDownsampled: Bool { + return name.hasSuffix("_downsampled") + } + public func log() { Logger.verbose("\t \(format), \(name), \(width), \(height), \(fileSize)") } @@ -101,7 +108,7 @@ enum GiphyFormat { return pickRendition(isStill:true, pickingStrategy:.smallerIsBetter, maxFileSize:kMaxFileSize) } - public func pickGifRendition() -> GiphyRendition? { + public func pickAnimatedRendition() -> GiphyRendition? { // Try to pick a small file... if let rendition = pickRendition(isStill:false, pickingStrategy:.largerIsBetter, maxFileSize:kMaxFileSize) { return rendition @@ -129,10 +136,12 @@ enum GiphyFormat { continue } // Only consider still renditions. - guard rendition.name.hasSuffix("_still") else { + guard rendition.isStill else { continue } - // Accept renditions without a valid file size. + // Accept still renditions without a valid file size. Note that fileSize + // will be zero for renditions without a valid file size, so they will pass + // the maxFileSize test. // // Don't worry about max content size; still images are tiny in comparison // with animated renditions. @@ -148,11 +157,11 @@ enum GiphyFormat { continue } // Ignore stills. - guard !rendition.name.hasSuffix("_still") else { + guard !rendition.isStill else { continue } // Ignore "downsampled" renditions which skip frames, etc. - guard !rendition.name.hasSuffix("_downsampled") else { + guard !rendition.isDownsampled else { continue } guard rendition.width >= kMinDimension && @@ -200,6 +209,7 @@ enum GiphyFormat { // MARK: - Properties static let TAG = "[GiphyAPI]" + let TAG = "[GiphyAPI]" static let sharedInstance = GiphyAPI() @@ -214,7 +224,7 @@ enum GiphyFormat { private func giphyAPISessionManager() -> AFHTTPSessionManager? { guard let baseUrl = NSURL(string:kGiphyBaseURL) else { - Logger.error("\(GiphyAPI.TAG) Invalid base URL.") + Logger.error("\(TAG) Invalid base URL.") return nil } // TODO: We need to verify that this session configuration properly @@ -236,15 +246,15 @@ enum GiphyFormat { // MARK: Search - public func search(query: String, success: @escaping (([GiphyImageInfo]) -> Void), failure: @escaping (() -> Void)) { + public func search(query: String, success: @escaping (([GiphyImageInfo]) -> Void), failure: @escaping ((NSError?) -> Void)) { guard let sessionManager = giphyAPISessionManager() else { - Logger.error("\(GiphyAPI.TAG) Couldn't create session manager.") - failure() + Logger.error("\(TAG) Couldn't create session manager.") + failure(nil) return } guard NSURL(string:kGiphyBaseURL) != nil else { - Logger.error("\(GiphyAPI.TAG) Invalid base URL.") - failure() + Logger.error("\(TAG) Invalid base URL.") + failure(nil) return } @@ -255,8 +265,8 @@ enum GiphyFormat { let kGiphyPageSize = 200 let kGiphyPageOffset = 0 guard let queryEncoded = query.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) else { - Logger.error("\(GiphyAPI.TAG) Could not URL encode query: \(query).") - failure() + Logger.error("\(TAG) Could not URL encode query: \(query).") + failure(nil) return } let urlString = "/v1/gifs/search?api_key=\(kGiphyApiKey)&offset=\(kGiphyPageOffset)&limit=\(kGiphyPageSize)&q=\(queryEncoded)" @@ -267,14 +277,14 @@ enum GiphyFormat { success: { _, value in Logger.error("\(GiphyAPI.TAG) search request succeeded") guard let imageInfos = self.parseGiphyImages(responseJson:value) else { - failure() + failure(nil) return } success(imageInfos) }, failure: { _, error in Logger.error("\(GiphyAPI.TAG) search request failed: \(error)") - failure() + failure(error as NSError) }) } @@ -282,45 +292,40 @@ enum GiphyFormat { private func parseGiphyImages(responseJson:Any?) -> [GiphyImageInfo]? { guard let responseJson = responseJson else { - Logger.error("\(GiphyAPI.TAG) Missing response.") + Logger.error("\(TAG) Missing response.") return nil } guard let responseDict = responseJson as? [String:Any] else { - Logger.error("\(GiphyAPI.TAG) Invalid response.") + Logger.error("\(TAG) Invalid response.") return nil } guard let imageDicts = responseDict["data"] as? [[String:Any]] else { - Logger.error("\(GiphyAPI.TAG) Invalid response data.") + Logger.error("\(TAG) Invalid response data.") return nil } - var result = [GiphyImageInfo]() - for imageDict in imageDicts { - guard let imageInfo = parseGiphyImage(imageDict:imageDict) else { - continue - } - result.append(imageInfo) + return imageDicts.flatMap { imageDict in + return parseGiphyImage(imageDict: imageDict) } - return result } // Giphy API results are often incomplete or malformed, so we need to be defensive. private func parseGiphyImage(imageDict: [String:Any]) -> GiphyImageInfo? { guard let giphyId = imageDict["id"] as? String else { - Logger.warn("\(GiphyAPI.TAG) Image dict missing id.") + Logger.warn("\(TAG) Image dict missing id.") return nil } guard giphyId.characters.count > 0 else { - Logger.warn("\(GiphyAPI.TAG) Image dict has invalid id.") + Logger.warn("\(TAG) Image dict has invalid id.") return nil } guard let renditionDicts = imageDict["images"] as? [String:Any] else { - Logger.warn("\(GiphyAPI.TAG) Image dict missing renditions.") + Logger.warn("\(TAG) Image dict missing renditions.") return nil } var renditions = [GiphyRendition]() for (renditionName, renditionDict) in renditionDicts { guard let renditionDict = renditionDict as? [String:Any] else { - Logger.warn("\(GiphyAPI.TAG) Invalid rendition dict.") + Logger.warn("\(TAG) Invalid rendition dict.") continue } guard let rendition = parseGiphyRendition(renditionName:renditionName, @@ -330,12 +335,12 @@ enum GiphyFormat { renditions.append(rendition) } guard renditions.count > 0 else { - Logger.warn("\(GiphyAPI.TAG) Image has no valid renditions.") + Logger.warn("\(TAG) Image has no valid renditions.") return nil } guard let originalRendition = findOriginalRendition(renditions:renditions) else { - Logger.warn("\(GiphyAPI.TAG) Image has no original rendition.") + Logger.warn("\(TAG) Image has no original rendition.") return nil } @@ -368,28 +373,28 @@ enum GiphyFormat { return nil } guard urlString.characters.count > 0 else { - Logger.warn("\(GiphyAPI.TAG) Rendition has invalid url.") + Logger.warn("\(TAG) Rendition has invalid url.") return nil } guard let url = NSURL(string:urlString) else { - Logger.warn("\(GiphyAPI.TAG) Rendition url could not be parsed.") + Logger.warn("\(TAG) Rendition url could not be parsed.") return nil } - guard let fileExtension = url.pathExtension else { - Logger.warn("\(GiphyAPI.TAG) Rendition url missing file extension.") + guard let fileExtension = url.pathExtension?.lowercased() else { + Logger.warn("\(TAG) Rendition url missing file extension.") return nil } var format = GiphyFormat.gif - if fileExtension.lowercased() == "gif" { + if fileExtension == "gif" { format = .gif - } else if fileExtension.lowercased() == "jpg" { + } else if fileExtension == "jpg" { format = .jpg - } else if fileExtension.lowercased() == "mp4" { + } else if fileExtension == "mp4" { format = .mp4 - } else if fileExtension.lowercased() == "webp" { + } else if fileExtension == "webp" { return nil } else { - Logger.warn("\(GiphyAPI.TAG) Invalid file extension: \(fileExtension).") + Logger.warn("\(TAG) Invalid file extension: \(fileExtension).") return nil } @@ -414,7 +419,7 @@ enum GiphyFormat { return nil } guard parsedValue > 0 else { - Logger.verbose("\(GiphyAPI.TAG) \(typeName) has non-positive \(key): \(parsedValue).") + Logger.verbose("\(TAG) \(typeName) has non-positive \(key): \(parsedValue).") return nil } return parsedValue diff --git a/Signal/src/network/GiphyDownloader.swift b/Signal/src/network/GiphyDownloader.swift index 8a651a1d4..417c323ae 100644 --- a/Signal/src/network/GiphyDownloader.swift +++ b/Signal/src/network/GiphyDownloader.swift @@ -378,7 +378,7 @@ extension URLSessionTask { // We write assets to the temporary directory so that iOS can clean them up. // We try to eagerly clean up these assets when they are no longer in use. let dirPath = NSTemporaryDirectory() - let fileExtension = assetRequest.rendition.fileExtension() + let fileExtension = assetRequest.rendition.fileExtension let fileName = (NSUUID().uuidString as NSString).appendingPathExtension(fileExtension)! let filePath = (dirPath as NSString).appendingPathComponent(fileName) diff --git a/Signal/translations/en.lproj/Localizable.strings b/Signal/translations/en.lproj/Localizable.strings index f83f88c27..27374de4d 100644 --- a/Signal/translations/en.lproj/Localizable.strings +++ b/Signal/translations/en.lproj/Localizable.strings @@ -605,7 +605,7 @@ "GIF_PICKER_VIEW_MISSING_QUERY" = "Please enter your search."; /* Title for the 'gif picker' dialog. */ -"GIF_PICKER_VIEW_TITLE" = "Gif Search"; +"GIF_PICKER_VIEW_TITLE" = "GIF Search"; /* Placeholder text for the search field in gif view */ "GIF_VIEW_SEARCH_PLACEHOLDER_TEXT" = "Enter your search"; @@ -1305,7 +1305,7 @@ "SECURE_SESSION_RESET" = "Secure session was reset."; /* Label for 'select gif to attach' action sheet button */ -"SELECT_GIF_BUTTON" = "Gif"; +"SELECT_GIF_BUTTON" = "GIF"; /* No comment provided by engineer. */ "SEND_AGAIN_BUTTON" = "Send Again";