Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented data download callback #773

Merged
merged 2 commits into from
Sep 21, 2017
Merged

Implemented data download callback #773

merged 2 commits into from
Sep 21, 2017

Conversation

Ashok28
Copy link
Contributor

@Ashok28 Ashok28 commented Sep 20, 2017

This adds possibility to further manipulate downloaded raw image data. Can be used i.e. for decrypting images.

@@ -141,6 +153,9 @@ extension ImageDownloaderDelegate {
public func isValidStatusCode(_ code: Int, for downloader: ImageDownloader) -> Bool {
return (200..<400).contains(code)
}
public func imageDownloader(_ downloader: ImageDownloader, didDownload data: Data, for url: URL) -> Data? {
return nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just return the original data instead of nil? Any reason to return a nil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, there's no particular reason, to return nil. I've pushed updated version.

@onevcat
Copy link
Owner

onevcat commented Sep 20, 2017

@Ashok28 Thanks for the commit. It could be useful and please take a look at the review comment above. Thank you!

@onevcat
Copy link
Owner

onevcat commented Sep 21, 2017

And I guess it would be better to interrupt the process and call completion handler with an error if nil is returned in this delegate method. By this, the user could have more control on what should happen according. (Say, not showing an image and getting an error when decrypting is failing).

@Ashok28 How do you think about that?

Anyway, I will merge this for now and do some improvement based on it. Thanks!

@onevcat onevcat merged commit 037c3ec into onevcat:master Sep 21, 2017
@Ashok28
Copy link
Contributor Author

Ashok28 commented Sep 21, 2017

Definitely, this should have acted just as basic implementation, which I used for one specific use case, wasn't sure, if it's going to get merged. I can further improve it.

petarlazarov pushed a commit to dowjones-mobile/Kingfisher that referenced this pull request Oct 6, 2021
Implemented data download callback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants