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

Feature addtional http headers and response #2166

Closed

Conversation

dreampiggy
Copy link
Contributor

@dreampiggy dreampiggy commented Jan 11, 2018

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: #2020

Pull Request Description

This PR introduce the feature to provide custom HTTP addtional headers for each UIView request, and grab the NSURLResponse from the download operation to the top entry of our lib. It based on the context arg.

@dreampiggy dreampiggy mentioned this pull request Jan 11, 2018
8 tasks
@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch 2 times, most recently from 4b50eb4 to 5e89565 Compare January 11, 2018 13:02
@dreampiggy dreampiggy added this to the 5.0.0 milestone Jan 12, 2018
@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch from 1c609b4 to dc9bcb0 Compare January 12, 2018 09:05
@dreampiggy
Copy link
Contributor Author

dreampiggy commented Jan 12, 2018

Well. After some consideration, I think we should break the API and change the return type. This change should at lease in 5.x. The reason why to change this is shown here :

Our previous API -[SDWebImageManager loadImageWithURL:options:progress:completed:] describe that

@return Returns an NSObject conforming to SDWebImageOperation. Should be an instance of SDWebImageDownloaderOperation

So this is adoptable and may not break current usage(Just the API). And I think, return a simple SDWebImageOperation protocol instance is really useless. User can not do anything except cancel the operation. We can make it open to the user to provide more custom advanced usage(Including that grab the NSURLResponse, check HTTP headers and more).

Compared to Kingfisher's same API KingfisherManager.retrieveImage, I think this design is not a bad design and this is not hard to maintain in the future. We should basically add two property, one is NSOperation *cacheOperation, the other is SDWebImageDownloadToken *downloadToken. Some advanced feature can be implementation and make it more user-customizable.

All the other properties should be hidden because it rely on the complex internal logic. I think now user can do something awesome feature with it :)

@BlueVirusX
Copy link

Can't wait to see this in version 5.x! :)

I think PR #2020 can be closed if this is the way to go.

@dreampiggy dreampiggy changed the base branch from master to 5.x January 12, 2018 09:32
@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch 2 times, most recently from a93b5ed to 63f7110 Compare January 12, 2018 11:48
@codecov-io
Copy link

codecov-io commented Jan 12, 2018

Codecov Report

Merging #2166 into 5.x will increase coverage by 5.03%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##              5.x    #2166      +/-   ##
==========================================
+ Coverage   75.51%   80.55%   +5.03%     
==========================================
  Files          42       37       -5     
  Lines        4121     3625     -496     
  Branches      353      328      -25     
==========================================
- Hits         3112     2920     -192     
+ Misses        987      683     -304     
  Partials       22       22
Impacted Files Coverage Δ
Tests/Tests/SDWebImageDownloaderTests.m 97.07% <ø> (+1.2%) ⬆️
SDWebImage/UIView+WebCacheOperation.m 59.45% <0%> (-11.51%) ⬇️
SDWebImage/SDWebImageManager.m 85.2% <100%> (+2.12%) ⬆️
SDWebImage/SDWebImageDownloader.m 83.25% <100%> (+1.08%) ⬆️
SDWebImage/UIImage+GIF.m 0% <0%> (-100%) ⬇️
SDWebImage/UIImage+WebP.m 0% <0%> (-100%) ⬇️
SDWebImage/UIImage+MultiFormat.m 0% <0%> (-100%) ⬇️
SDWebImage/UIImage+WebCache.m 81.81% <0%> (-18.19%) ⬇️
SDWebImage/NSData+ImageContentType.m 50% <0%> (-11.67%) ⬇️
SDWebImage/SDWebImageCoderHelper.m 61.42% <0%> (-6.55%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fa6e88...de63c77. Read the comment docs.

@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch from b008f33 to 2564a9a Compare January 13, 2018 10:42
@dreampiggy
Copy link
Contributor Author

dreampiggy commented Jan 13, 2018

Current break change (Need change code)

SDWebImageManager.h

  • -[SDWebImageManager loadImageWithURL:options:progress:completed:] return type id<SDWebImageOperation> CHANGED TO SDWebImageCombinedOperation * (Actually this do not need change code because SDWebImageCombinedOperation conform to SDWebImageOperation and the document also mean the return type is SDWebImageCombinedOperation)

SDWebImageDownloadToken

  • url CHANGE TO readonly (Previous design is wrong, this value should not be modified outisde)
  • downloadOperationCancelToken CHANGE TO readonly (Previous design is wrong, this value should not be modified outisde)

@skyline75489
Copy link
Member

I like this one. But I'm not sure whether we should expose SDWebImageCombinedOperation. It seems very internal to me. The id <SDWebImageOperation> is obviously more flexible.

@dreampiggy
Copy link
Contributor Author

dreampiggy commented Jan 22, 2018

@skyline75489 You can use a id<SDWebImageOperation>. But if we add more and more property already internal on this. For example, we need downloadToken now to allow user grab the download NSURLResponse, which means we should add this to the abstact protocol again and again so maybe this is really tied to that actual class. And this is not a protocol to let user provide unlike SDWebImageDownloadOperationInterface, it just like a public interface but not which allow user to implement

For the download which bridge the cache and downloader together, I don't think this design will be totally change in the future (that will be a really huge change). So a request to load image must contains two process: cache request and download request. The operation for cache request is a NSOperation and the download request is SDWebImageDownloadToken. I think these two is useful for use which need know the context but not just have a ability to cancel

This design is adopted from Kingfisher too. I think this design is not a bad design.

@dreampiggy
Copy link
Contributor Author

dreampiggy commented Jan 23, 2018

@skyline75489 I seperate the context arg part to another PR #2189. For whether we should introduce this addtional headers feature and expose the SDWebImageCombinedOperation, we can make further discussion.

But that context is usefull for the #2140 because I need to provide a SDWebImageContextAnimatedImageClass with to allow the cache and downloader to create a UIImage<SDAnimatedImage> instance instead of normal UIImage instance. Because our SDAnimatedImage is a protocol which can let user implement their own (Such as YYImage, which can conform to SDAnimatedImage :)). So we can not use a simple SDWebImageOptions enum to solve this problem.

And also, some user ask to allow custom scale factor because of that our SDImageScaleForKey is really tricky and hard-coded. They can provide their desired scale with a CGFloat. This can not be solved by SDWebImageOptions either. And maybe more issue about this. So we need that context arg and merged now.

@dreampiggy dreampiggy changed the title Pass the context arg from the top level to the bottom level to allow additional headers or other logic in the future Feature addtional http headers and response Jan 23, 2018
@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch from 1f5d09b to bfd8189 Compare January 23, 2018 13:39
@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch from bfd8189 to b83f8ac Compare February 7, 2018 11:12
@dreampiggy dreampiggy force-pushed the feature_additional_http_headers branch from b83f8ac to 84f6cae Compare February 7, 2018 12:05
@dreampiggy
Copy link
Contributor Author

Maybe this feature we need more time to allow extensibility. Currently it can only apply the HTTP Headers. However, for NSURLRequest, there maybe be more than just HTTP Headers, for example, the timeoutInterval, HTTPMethod and more.

We can introduc a more common usage like request modifier or something. We send the original request for download to the user, user can use mutableCopy and modify to return their own request.

@dreampiggy
Copy link
Contributor Author

@BlueVirusX @skyline75489 This PR was replaced by #2261 . Which provide a more common use case and abstract implementation to keep extensibility

@dreampiggy dreampiggy closed this Mar 23, 2018
@bpoplauschi bpoplauschi deleted the feature_additional_http_headers branch April 2, 2018 13:23
@dreampiggy dreampiggy removed this from the 5.0.0 milestone Apr 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants