From a46dd706c8a809b8abcd6d54ddc84b322175f7b8 Mon Sep 17 00:00:00 2001 From: onevcat Date: Wed, 30 Dec 2020 12:21:48 +0900 Subject: [PATCH 1/8] Refactor of image downloader - extract callback creation --- Sources/Networking/ImageDownloader.swift | 27 +++++++++++++----------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index 8aafa06ee..6320936c4 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -30,6 +30,8 @@ import AppKit import UIKit #endif +typealias DownloadResult = Result + /// Represents a success result of an image downloading progress. public struct ImageLoadingResult { @@ -193,6 +195,18 @@ open class ImageDownloader { } } + // Wraps `completionHandler` to `onCompleted` respectively. + private func createCompletionCallBack(_ completionHandler: ((DownloadResult) -> Void)?) -> Delegate? { + return completionHandler.map { block -> Delegate in + + let delegate = Delegate, Void>() + delegate.delegate(on: self) { (self, callback) in + block(callback) + } + return delegate + } + } + // MARK: Dowloading Task /// Downloads an image with a URL and option. Invoked internally by Kingfisher. Subclasses must invoke super. /// @@ -232,20 +246,9 @@ open class ImageDownloader { return nil } - // Wraps `completionHandler` to `onCompleted` respectively. - - let onCompleted = completionHandler.map { - block -> Delegate, Void> in - let delegate = Delegate, Void>() - delegate.delegate(on: self) { (_, callback) in - block(callback) - } - return delegate - } - // SessionDataTask.TaskCallback is a wrapper for `onCompleted` and `options` (for processor info) let callback = SessionDataTask.TaskCallback( - onCompleted: onCompleted, + onCompleted: createCompletionCallBack(completionHandler), options: options ) From 5421f1324429a46a612b2463e831c07e05400f7d Mon Sep 17 00:00:00 2001 From: onevcat Date: Wed, 30 Dec 2020 13:25:12 +0900 Subject: [PATCH 2/8] More function splitting and moving --- Sources/Networking/ImageDownloader.swift | 172 +++++++++++++---------- 1 file changed, 99 insertions(+), 73 deletions(-) diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index 6320936c4..25fe5c125 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -207,6 +207,98 @@ open class ImageDownloader { } } + private func createTaskCallback( + _ completionHandler: ((DownloadResult) -> Void)?, + options: KingfisherParsedOptionsInfo + ) -> SessionDataTask.TaskCallback + { + return SessionDataTask.TaskCallback( + onCompleted: createCompletionCallBack(completionHandler), + options: options + ) + } + + private func addDownloadTask( + for url: URL, + with request: URLRequest, + options: KingfisherParsedOptionsInfo, + callback: SessionDataTask.TaskCallback + ) -> DownloadTask + { + // Ready to start download. Add it to session task manager (`sessionHandler`) + let downloadTask: DownloadTask + if let existingTask = sessionDelegate.task(for: url) { + downloadTask = sessionDelegate.append(existingTask, url: url, callback: callback) + } else { + let sessionDataTask = session.dataTask(with: request) + sessionDataTask.priority = options.downloadPriority + downloadTask = sessionDelegate.add(sessionDataTask, url: url, callback: callback) + } + return downloadTask + } + + private func startDownloadTask(_ downloadTask: DownloadTask, url: URL, request: URLRequest, options: KingfisherParsedOptionsInfo) { + + let sessionTask = downloadTask.sessionTask + guard !sessionTask.started else { + return + } + + sessionTask.onTaskDone.delegate(on: self) { (self, done) in + // Underlying downloading finishes. + // result: Result<(Data, URLResponse?)>, callbacks: [TaskCallback] + let (result, callbacks) = done + + // Before processing the downloaded data. + do { + let value = try result.get() + self.delegate?.imageDownloader( + self, + didFinishDownloadingImageForURL: url, + with: value.1, + error: nil + ) + } catch { + self.delegate?.imageDownloader( + self, + didFinishDownloadingImageForURL: url, + with: nil, + error: error + ) + } + + switch result { + // Download finished. Now process the data to an image. + case .success(let (data, response)): + let processor = ImageDataProcessor( + data: data, callbacks: callbacks, processingQueue: options.processingQueue) + processor.onImageProcessed.delegate(on: self) { (self, result) in + // `onImageProcessed` will be called for `callbacks.count` times, with each + // `SessionDataTask.TaskCallback` as the input parameter. + // result: Result, callback: SessionDataTask.TaskCallback + let (result, callback) = result + + if let image = try? result.get() { + self.delegate?.imageDownloader(self, didDownload: image, for: url, with: response) + } + + let imageResult = result.map { ImageLoadingResult(image: $0, url: url, originalData: data) } + let queue = callback.options.callbackQueue + queue.execute { callback.onCompleted?.call(imageResult) } + } + processor.process() + + case .failure(let error): + callbacks.forEach { callback in + let queue = callback.options.callbackQueue + queue.execute { callback.onCompleted?.call(.failure(error)) } + } + } + } + delegate?.imageDownloader(self, willDownloadImageForURL: url, with: request) + sessionTask.resume() + } + // MARK: Dowloading Task /// Downloads an image with a URL and option. Invoked internally by Kingfisher. Subclasses must invoke super. /// @@ -246,81 +338,15 @@ open class ImageDownloader { return nil } - // SessionDataTask.TaskCallback is a wrapper for `onCompleted` and `options` (for processor info) - let callback = SessionDataTask.TaskCallback( - onCompleted: createCompletionCallBack(completionHandler), - options: options - ) - // Ready to start download. Add it to session task manager (`sessionHandler`) + let downloadTask = addDownloadTask( + for: url, + with: request, + options: options, + callback: createTaskCallback(completionHandler, options: options) + ) - let downloadTask: DownloadTask - if let existingTask = sessionDelegate.task(for: url) { - downloadTask = sessionDelegate.append(existingTask, url: url, callback: callback) - } else { - let sessionDataTask = session.dataTask(with: request) - sessionDataTask.priority = options.downloadPriority - downloadTask = sessionDelegate.add(sessionDataTask, url: url, callback: callback) - } - - let sessionTask = downloadTask.sessionTask - - // Start the session task if not started yet. - if !sessionTask.started { - sessionTask.onTaskDone.delegate(on: self) { (self, done) in - // Underlying downloading finishes. - // result: Result<(Data, URLResponse?)>, callbacks: [TaskCallback] - let (result, callbacks) = done - - // Before processing the downloaded data. - do { - let value = try result.get() - self.delegate?.imageDownloader( - self, - didFinishDownloadingImageForURL: url, - with: value.1, - error: nil - ) - } catch { - self.delegate?.imageDownloader( - self, - didFinishDownloadingImageForURL: url, - with: nil, - error: error - ) - } - - switch result { - // Download finished. Now process the data to an image. - case .success(let (data, response)): - let processor = ImageDataProcessor( - data: data, callbacks: callbacks, processingQueue: options.processingQueue) - processor.onImageProcessed.delegate(on: self) { (self, result) in - // `onImageProcessed` will be called for `callbacks.count` times, with each - // `SessionDataTask.TaskCallback` as the input parameter. - // result: Result, callback: SessionDataTask.TaskCallback - let (result, callback) = result - - if let image = try? result.get() { - self.delegate?.imageDownloader(self, didDownload: image, for: url, with: response) - } - - let imageResult = result.map { ImageLoadingResult(image: $0, url: url, originalData: data) } - let queue = callback.options.callbackQueue - queue.execute { callback.onCompleted?.call(imageResult) } - } - processor.process() - - case .failure(let error): - callbacks.forEach { callback in - let queue = callback.options.callbackQueue - queue.execute { callback.onCompleted?.call(.failure(error)) } - } - } - } - delegate?.imageDownloader(self, willDownloadImageForURL: url, with: request) - sessionTask.resume() - } + startDownloadTask(downloadTask, url: url, request: request, options: options) return downloadTask } From ad67710a696026d14f697313552d48219fbd5343 Mon Sep 17 00:00:00 2001 From: onevcat Date: Wed, 30 Dec 2020 13:31:07 +0900 Subject: [PATCH 3/8] Combine related parameters to context --- Sources/Networking/ImageDownloader.swift | 45 +++++++++++++----------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index 25fe5c125..95627333c 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -219,25 +219,23 @@ open class ImageDownloader { } private func addDownloadTask( - for url: URL, - with request: URLRequest, - options: KingfisherParsedOptionsInfo, + context: DownloadingContext, callback: SessionDataTask.TaskCallback ) -> DownloadTask { // Ready to start download. Add it to session task manager (`sessionHandler`) let downloadTask: DownloadTask - if let existingTask = sessionDelegate.task(for: url) { - downloadTask = sessionDelegate.append(existingTask, url: url, callback: callback) + if let existingTask = sessionDelegate.task(for: context.url) { + downloadTask = sessionDelegate.append(existingTask, url: context.url, callback: callback) } else { - let sessionDataTask = session.dataTask(with: request) - sessionDataTask.priority = options.downloadPriority - downloadTask = sessionDelegate.add(sessionDataTask, url: url, callback: callback) + let sessionDataTask = session.dataTask(with: context.request) + sessionDataTask.priority = context.options.downloadPriority + downloadTask = sessionDelegate.add(sessionDataTask, url: context.url, callback: callback) } return downloadTask } - private func startDownloadTask(_ downloadTask: DownloadTask, url: URL, request: URLRequest, options: KingfisherParsedOptionsInfo) { + private func startDownloadTask(_ downloadTask: DownloadTask, context: DownloadingContext) { let sessionTask = downloadTask.sessionTask guard !sessionTask.started else { @@ -254,14 +252,14 @@ open class ImageDownloader { let value = try result.get() self.delegate?.imageDownloader( self, - didFinishDownloadingImageForURL: url, + didFinishDownloadingImageForURL: context.url, with: value.1, error: nil ) } catch { self.delegate?.imageDownloader( self, - didFinishDownloadingImageForURL: url, + didFinishDownloadingImageForURL: context.url, with: nil, error: error ) @@ -271,7 +269,7 @@ open class ImageDownloader { // Download finished. Now process the data to an image. case .success(let (data, response)): let processor = ImageDataProcessor( - data: data, callbacks: callbacks, processingQueue: options.processingQueue) + data: data, callbacks: callbacks, processingQueue: context.options.processingQueue) processor.onImageProcessed.delegate(on: self) { (self, result) in // `onImageProcessed` will be called for `callbacks.count` times, with each // `SessionDataTask.TaskCallback` as the input parameter. @@ -279,10 +277,10 @@ open class ImageDownloader { let (result, callback) = result if let image = try? result.get() { - self.delegate?.imageDownloader(self, didDownload: image, for: url, with: response) + self.delegate?.imageDownloader(self, didDownload: image, for: context.url, with: response) } - let imageResult = result.map { ImageLoadingResult(image: $0, url: url, originalData: data) } + let imageResult = result.map { ImageLoadingResult(image: $0, url: context.url, originalData: data) } let queue = callback.options.callbackQueue queue.execute { callback.onCompleted?.call(imageResult) } } @@ -295,7 +293,7 @@ open class ImageDownloader { } } } - delegate?.imageDownloader(self, willDownloadImageForURL: url, with: request) + delegate?.imageDownloader(self, willDownloadImageForURL: context.url, with: context.request) sessionTask.resume() } @@ -338,15 +336,12 @@ open class ImageDownloader { return nil } + let context = DownloadingContext(url: url, request: request, options: options) // Ready to start download. Add it to session task manager (`sessionHandler`) let downloadTask = addDownloadTask( - for: url, - with: request, - options: options, - callback: createTaskCallback(completionHandler, options: options) + context: context, callback: createTaskCallback(completionHandler, options: options) ) - - startDownloadTask(downloadTask, url: url, request: request, options: options) + startDownloadTask(downloadTask, context: context) return downloadTask } @@ -425,3 +420,11 @@ extension ImageDownloader: AuthenticationChallengeResponsable {} // Use the default implementation from extension of `ImageDownloaderDelegate`. extension ImageDownloader: ImageDownloaderDelegate {} + +extension ImageDownloader { + struct DownloadingContext { + let url: URL + let request: URLRequest + let options: KingfisherParsedOptionsInfo + } +} From 442ebf3552629d7cd83806d62e594f92f0cb31c5 Mon Sep 17 00:00:00 2001 From: onevcat Date: Wed, 30 Dec 2020 14:40:12 +0900 Subject: [PATCH 4/8] Move method --- Sources/Networking/ImageDownloader.swift | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index 95627333c..561837910 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -235,11 +235,17 @@ open class ImageDownloader { return downloadTask } - private func startDownloadTask(_ downloadTask: DownloadTask, context: DownloadingContext) { + private func startDownloadTask( + context: DownloadingContext, + callback: SessionDataTask.TaskCallback + ) -> DownloadTask + { + + let downloadTask = addDownloadTask(context: context, callback: callback) let sessionTask = downloadTask.sessionTask guard !sessionTask.started else { - return + return downloadTask } sessionTask.onTaskDone.delegate(on: self) { (self, done) in @@ -295,6 +301,7 @@ open class ImageDownloader { } delegate?.imageDownloader(self, willDownloadImageForURL: context.url, with: context.request) sessionTask.resume() + return downloadTask } // MARK: Dowloading Task @@ -336,12 +343,10 @@ open class ImageDownloader { return nil } - let context = DownloadingContext(url: url, request: request, options: options) - // Ready to start download. Add it to session task manager (`sessionHandler`) - let downloadTask = addDownloadTask( - context: context, callback: createTaskCallback(completionHandler, options: options) + let downloadTask = startDownloadTask( + context: DownloadingContext(url: url, request: request, options: options), + callback: createTaskCallback(completionHandler, options: options) ) - startDownloadTask(downloadTask, context: context) return downloadTask } From 5b6083268399135f2f021991e29980f3a109a051 Mon Sep 17 00:00:00 2001 From: onevcat Date: Wed, 30 Dec 2020 15:05:07 +0900 Subject: [PATCH 5/8] Abstract async operation --- Sources/General/KingfisherOptionsInfo.swift | 4 +- Sources/Networking/ImageDownloader.swift | 77 +++++++++++++++------ Sources/Networking/RequestModifier.swift | 16 ++++- 3 files changed, 71 insertions(+), 26 deletions(-) diff --git a/Sources/General/KingfisherOptionsInfo.swift b/Sources/General/KingfisherOptionsInfo.swift index 9c19a17c0..13f5d5ed3 100644 --- a/Sources/General/KingfisherOptionsInfo.swift +++ b/Sources/General/KingfisherOptionsInfo.swift @@ -127,7 +127,7 @@ public enum KingfisherOptionsInfoItem { /// This is the last chance you can modify the image download request. You can modify the request for some /// customizing purpose, such as adding auth token to the header, do basic HTTP auth or something like url mapping. /// The original request will be sent without any modification by default. - case requestModifier(ImageDownloadRequestModifier) + case requestModifier(AsyncImageDownloadRequestModifier) /// The `ImageDownloadRedirectHandler` contained will be used to change the request before redirection. /// This is the possibility you can modify the image download request during redirect. You can modify the request for @@ -272,7 +272,7 @@ public struct KingfisherParsedOptionsInfo { public var preloadAllAnimationData = false public var callbackQueue: CallbackQueue = .mainCurrentOrAsync public var scaleFactor: CGFloat = 1.0 - public var requestModifier: ImageDownloadRequestModifier? = nil + public var requestModifier: AsyncImageDownloadRequestModifier? = nil public var redirectHandler: ImageDownloadRedirectHandler? = nil public var processor: ImageProcessor = DefaultImageProcessor.default public var imageModifier: ImageModifier? = nil diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index 561837910..fd25226b0 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -218,6 +218,42 @@ open class ImageDownloader { ) } + private func createDownloadContext( + with url: URL, + options: KingfisherParsedOptionsInfo, + done: ((Result) -> Void) + ) + { + func checkRequestAndDone(r: URLRequest) { + + // There is a possibility that request modifier changed the url to `nil` or empty. + // In this case, throw an error. + guard let url = r.url, !url.absoluteString.isEmpty else { + done(.failure(KingfisherError.requestError(reason: .invalidURL(request: r)))) + return + } + + done(.success(DownloadingContext(url: url, request: r, options: options))) + } + + // Creates default request. + var request = URLRequest(url: url, cachePolicy: .reloadIgnoringLocalCacheData, timeoutInterval: downloadTimeout) + request.httpShouldUsePipelining = requestsUsePipelining + + if let requestModifier = options.requestModifier { + // Modifies request before sending. + requestModifier.modified(for: request) { result in + guard let finalRequest = result else { + done(.failure(KingfisherError.requestError(reason: .emptyRequest))) + return + } + checkRequestAndDone(r: finalRequest) + } + } else { + checkRequestAndDone(r: request) + } + } + private func addDownloadTask( context: DownloadingContext, callback: SessionDataTask.TaskCallback @@ -319,34 +355,29 @@ open class ImageDownloader { options: KingfisherParsedOptionsInfo, completionHandler: ((Result) -> Void)? = nil) -> DownloadTask? { - // Creates default request. - var request = URLRequest(url: url, cachePolicy: .reloadIgnoringLocalCacheData, timeoutInterval: downloadTimeout) - request.httpShouldUsePipelining = requestsUsePipelining - - if let requestModifier = options.requestModifier { - // Modifies request before sending. - guard let r = requestModifier.modified(for: request) else { + var downloadTask: DownloadTask? + createDownloadContext(with: url, options: options) { result in + switch result { + case .success(let context): + // `downloadTask` will be set if the downloading started immediately. This is the case when no request + // modifier or a sync modifier (`ImageDownloadRequestModifier`) is used. Otherwise, when an + // `AsyncImageDownloadRequestModifier` is used the returned `downloadTask` of this method will be `nil` + // and the actual "delayed" task is given in `AsyncImageDownloadRequestModifier.onDownloadTaskStarted` + // callback. + downloadTask = startDownloadTask( + context: context, + callback: createTaskCallback(completionHandler, options: options) + ) + if let modifier = options.requestModifier { + modifier.onDownloadTaskStarted?(downloadTask) + } + case .failure(let error): options.callbackQueue.execute { - completionHandler?(.failure(KingfisherError.requestError(reason: .emptyRequest))) + completionHandler?(.failure(error)) } - return nil - } - request = r - } - - // There is a possibility that request modifier changed the url to `nil` or empty. - // In this case, throw an error. - guard let url = request.url, !url.absoluteString.isEmpty else { - options.callbackQueue.execute { - completionHandler?(.failure(KingfisherError.requestError(reason: .invalidURL(request: request)))) } - return nil } - let downloadTask = startDownloadTask( - context: DownloadingContext(url: url, request: request, options: options), - callback: createTaskCallback(completionHandler, options: options) - ) return downloadTask } diff --git a/Sources/Networking/RequestModifier.swift b/Sources/Networking/RequestModifier.swift index 06b062ab8..eaa1fd8c4 100644 --- a/Sources/Networking/RequestModifier.swift +++ b/Sources/Networking/RequestModifier.swift @@ -26,8 +26,13 @@ import Foundation +public protocol AsyncImageDownloadRequestModifier { + func modified(for request: URLRequest, modify: (URLRequest?) -> Void) + var onDownloadTaskStarted: ((DownloadTask?) -> Void)? { get } +} + /// Represents and wraps a method for modifying request before an image download request starts. -public protocol ImageDownloadRequestModifier { +public protocol ImageDownloadRequestModifier: AsyncImageDownloadRequestModifier { /// A method will be called just before the `request` being sent. /// This is the last chance you can modify the image download request. You can modify the request for some @@ -46,6 +51,15 @@ public protocol ImageDownloadRequestModifier { func modified(for request: URLRequest) -> URLRequest? } +extension ImageDownloadRequestModifier { + public func modified(for request: URLRequest, modify: (URLRequest?) -> Void) { + let request = modified(for: request) + modify(request) + } + + public var onDownloadTaskStarted: ((DownloadTask?) -> Void)? { return nil } +} + /// A wrapper for creating an `ImageDownloadRequestModifier` easier. /// This type conforms to `ImageDownloadRequestModifier` and wraps an image modify block. public struct AnyModifier: ImageDownloadRequestModifier { From 1b51871d5b0a9dbac25efcd41f0f22ce0bab7417 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 31 Dec 2020 00:20:08 +0900 Subject: [PATCH 6/8] Add escaping and tests for async test --- Sources/Networking/ImageDownloader.swift | 8 ++-- Sources/Networking/RequestModifier.swift | 6 +-- .../ImageDownloaderTests.swift | 43 ++++++++++++++++++- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index fd25226b0..f15607e09 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -221,7 +221,7 @@ open class ImageDownloader { private func createDownloadContext( with url: URL, options: KingfisherParsedOptionsInfo, - done: ((Result) -> Void) + done: @escaping ((Result) -> Void) ) { func checkRequestAndDone(r: URLRequest) { @@ -340,7 +340,7 @@ open class ImageDownloader { return downloadTask } - // MARK: Dowloading Task + // MARK: Downloading Task /// Downloads an image with a URL and option. Invoked internally by Kingfisher. Subclasses must invoke super. /// /// - Parameters: @@ -364,9 +364,9 @@ open class ImageDownloader { // `AsyncImageDownloadRequestModifier` is used the returned `downloadTask` of this method will be `nil` // and the actual "delayed" task is given in `AsyncImageDownloadRequestModifier.onDownloadTaskStarted` // callback. - downloadTask = startDownloadTask( + downloadTask = self.startDownloadTask( context: context, - callback: createTaskCallback(completionHandler, options: options) + callback: self.createTaskCallback(completionHandler, options: options) ) if let modifier = options.requestModifier { modifier.onDownloadTaskStarted?(downloadTask) diff --git a/Sources/Networking/RequestModifier.swift b/Sources/Networking/RequestModifier.swift index eaa1fd8c4..e4f9a5dfe 100644 --- a/Sources/Networking/RequestModifier.swift +++ b/Sources/Networking/RequestModifier.swift @@ -27,7 +27,7 @@ import Foundation public protocol AsyncImageDownloadRequestModifier { - func modified(for request: URLRequest, modify: (URLRequest?) -> Void) + func modified(for request: URLRequest, reportModified: @escaping (URLRequest?) -> Void) var onDownloadTaskStarted: ((DownloadTask?) -> Void)? { get } } @@ -52,9 +52,9 @@ public protocol ImageDownloadRequestModifier: AsyncImageDownloadRequestModifier } extension ImageDownloadRequestModifier { - public func modified(for request: URLRequest, modify: (URLRequest?) -> Void) { + public func modified(for request: URLRequest, reportModified: @escaping (URLRequest?) -> Void) { let request = modified(for: request) - modify(request) + reportModified(request) } public var onDownloadTaskStarted: ((DownloadTask?) -> Void)? { return nil } diff --git a/Tests/KingfisherTests/ImageDownloaderTests.swift b/Tests/KingfisherTests/ImageDownloaderTests.swift index 2d4c073a4..5361cc713 100644 --- a/Tests/KingfisherTests/ImageDownloaderTests.swift +++ b/Tests/KingfisherTests/ImageDownloaderTests.swift @@ -112,11 +112,40 @@ class ImageDownloaderTests: XCTestCase { modifier.url = url let someURL = URL(string: "some_strange_url")! - downloader.downloadImage(with: someURL, options: [.requestModifier(modifier)]) { result in + let task = downloader.downloadImage(with: someURL, options: [.requestModifier(modifier)]) { result in XCTAssertNotNil(result.value) XCTAssertEqual(result.value?.url, url) exp.fulfill() } + XCTAssertNotNil(task) + waitForExpectations(timeout: 3, handler: nil) + } + + func testDownloadWithAsyncModifyingRequest() { + let exp = expectation(description: #function) + + let url = testURLs[0] + stub(url, data: testImageData) + + var downloadTaskCalled = false + + let asyncModifier = AsyncURLModifier() + asyncModifier.url = url + asyncModifier.onDownloadTaskStarted = { task in + XCTAssertNotNil(task) + downloadTaskCalled = true + } + + + let someURL = URL(string: "some_strage_url")! + let task = downloader.downloadImage(with: someURL, options: [.requestModifier(asyncModifier)]) { result in + XCTAssertNotNil(result.value) + XCTAssertEqual(result.value?.url, url) + XCTAssertTrue(downloadTaskCalled) + exp.fulfill() + } + // The returned task is nil since the download is not starting immediately. + XCTAssertNil(task) waitForExpectations(timeout: 3, handler: nil) } @@ -535,3 +564,15 @@ class URLModifier: ImageDownloadRequestModifier { } } +class AsyncURLModifier: AsyncImageDownloadRequestModifier { + var url: URL? = nil + var onDownloadTaskStarted: ((DownloadTask?) -> Void)? + + func modified(for request: URLRequest, reportModified: @escaping (URLRequest?) -> Void) { + var r = request + r.url = url + DispatchQueue.main.async { + reportModified(r) + } + } +} From 1eb06770fba486cd63f3cd20e74437d4b5226170 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 31 Dec 2020 00:35:31 +0900 Subject: [PATCH 7/8] More refactor --- Sources/Networking/ImageDownloader.swift | 62 +++++++++++++++--------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/Sources/Networking/ImageDownloader.swift b/Sources/Networking/ImageDownloader.swift index f15607e09..c255533b2 100644 --- a/Sources/Networking/ImageDownloader.swift +++ b/Sources/Networking/ImageDownloader.swift @@ -271,6 +271,37 @@ open class ImageDownloader { return downloadTask } + + private func reportWillDownloadImage(url: URL, request: URLRequest) { + delegate?.imageDownloader(self, willDownloadImageForURL: url, with: request) + } + + private func reportDidDownloadImageData(result: Result<(Data, URLResponse?), KingfisherError>, url: URL) { + var response: URLResponse? + var err: Error? + do { + response = try result.get().1 + } catch { + err = error + } + self.delegate?.imageDownloader( + self, + didFinishDownloadingImageForURL: url, + with: response, + error: err + ) + } + + private func reportDidProcessImage( + result: Result, url: URL, response: URLResponse? + ) + { + if let image = try? result.get() { + self.delegate?.imageDownloader(self, didDownload: image, for: url, with: response) + } + + } + private func startDownloadTask( context: DownloadingContext, callback: SessionDataTask.TaskCallback @@ -290,37 +321,21 @@ open class ImageDownloader { let (result, callbacks) = done // Before processing the downloaded data. - do { - let value = try result.get() - self.delegate?.imageDownloader( - self, - didFinishDownloadingImageForURL: context.url, - with: value.1, - error: nil - ) - } catch { - self.delegate?.imageDownloader( - self, - didFinishDownloadingImageForURL: context.url, - with: nil, - error: error - ) - } + self.reportDidDownloadImageData(result: result, url: context.url) switch result { // Download finished. Now process the data to an image. case .success(let (data, response)): let processor = ImageDataProcessor( - data: data, callbacks: callbacks, processingQueue: context.options.processingQueue) - processor.onImageProcessed.delegate(on: self) { (self, result) in + data: data, callbacks: callbacks, processingQueue: context.options.processingQueue + ) + processor.onImageProcessed.delegate(on: self) { (self, done) in // `onImageProcessed` will be called for `callbacks.count` times, with each // `SessionDataTask.TaskCallback` as the input parameter. // result: Result, callback: SessionDataTask.TaskCallback - let (result, callback) = result + let (result, callback) = done - if let image = try? result.get() { - self.delegate?.imageDownloader(self, didDownload: image, for: context.url, with: response) - } + self.reportDidProcessImage(result: result, url: context.url, response: response) let imageResult = result.map { ImageLoadingResult(image: $0, url: context.url, originalData: data) } let queue = callback.options.callbackQueue @@ -335,7 +350,8 @@ open class ImageDownloader { } } } - delegate?.imageDownloader(self, willDownloadImageForURL: context.url, with: context.request) + + reportWillDownloadImage(url: context.url, request: context.request) sessionTask.resume() return downloadTask } From f1098e6b44c09cb2747989c9be66a9e7008b5e1d Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 31 Dec 2020 16:27:23 +0900 Subject: [PATCH 8/8] Add some doc --- Sources/Networking/RequestModifier.swift | 27 +++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/Sources/Networking/RequestModifier.swift b/Sources/Networking/RequestModifier.swift index e4f9a5dfe..a1d972df3 100644 --- a/Sources/Networking/RequestModifier.swift +++ b/Sources/Networking/RequestModifier.swift @@ -26,15 +26,38 @@ import Foundation +/// Represents and wraps a method for modifying request before an image download request starts in an asynchronous way. public protocol AsyncImageDownloadRequestModifier { + + /// This method will be called just before the `request` being sent. + /// This is the last chance you can modify the image download request. You can modify the request for some + /// customizing purpose, such as adding auth token to the header, do basic HTTP auth or something like url mapping. + /// When you have done with the modification, call the `reportModified` block with the modified request and the data + /// download will happen with this request. + /// + /// Usually, you pass an `AsyncImageDownloadRequestModifier` as the associated value of + /// `KingfisherOptionsInfoItem.requestModifier` and use it as the `options` parameter in related methods. + /// + /// If you do nothing with the input `request` and return it as is, a downloading process will start with it. + /// + /// - Parameters: + /// - request: The input request contains necessary information like `url`. This request is generated + /// according to your resource url as a GET request. + /// - reportModified: The callback block you need to call after the asynchronous modifying done. + /// func modified(for request: URLRequest, reportModified: @escaping (URLRequest?) -> Void) + + /// A block will be called when the download task started. + /// + /// If an `AsyncImageDownloadRequestModifier` and the asynchronous modification happens before the download, the + /// related download method will not return a valid `DownloadTask` value. Instead, you can get one from this method. var onDownloadTaskStarted: ((DownloadTask?) -> Void)? { get } } /// Represents and wraps a method for modifying request before an image download request starts. public protocol ImageDownloadRequestModifier: AsyncImageDownloadRequestModifier { - /// A method will be called just before the `request` being sent. + /// This method will be called just before the `request` being sent. /// This is the last chance you can modify the image download request. You can modify the request for some /// customizing purpose, such as adding auth token to the header, do basic HTTP auth or something like url mapping. /// @@ -57,6 +80,8 @@ extension ImageDownloadRequestModifier { reportModified(request) } + /// This is `nil` for a sync `ImageDownloadRequestModifier` by default. You can get the `DownloadTask` from the + /// return value of downloader method. public var onDownloadTaskStarted: ((DownloadTask?) -> Void)? { return nil } }