remove addProgressCallback, add createDownloaderOperationWithUrl #2336

gukemanbu · 2018-05-24T12:30:20Z

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: ...

Pull Request Description

...

gukemanbu · 2018-05-24T12:30:27Z

I don't think the original code is good design. The first thing I saw was createCallback code, while the main logic is in addProgressCallback method, which not conducive to reading.

codecov-io · 2018-05-24T12:45:42Z

Codecov Report

Merging #2336 into master will decrease coverage by 0.12%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #2336      +/-   ##
==========================================
- Coverage   75.44%   75.32%   -0.13%     
==========================================
  Files          36       36              
  Lines        3837     3826      -11     
==========================================
- Hits         2895     2882      -13     
- Misses        942      944       +2

Impacted Files	Coverage Δ
Tests/Tests/SDWebImageDownloaderTests.m	`83.18% <100%> (+0.34%)`	⬆️
SDWebImage/SDWebImageDownloader.m	`76.75% <81.81%> (-1.7%)`	⬇️
SDWebImage/SDWebImageManager.m	`73.87% <0%> (-1.36%)`	⬇️
SDWebImage/SDImageCache.m	`64.02% <0%> (+0.42%)`	⬆️

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 e98a941...2338871. Read the comment docs.

bpoplauschi · 2018-07-11T10:09:09Z

I see your point @gukemanbu, but this change is breaking compatibility. We can talk about it, for a major release.

dreampiggy · 2018-07-11T17:26:49Z

This does not cause API breaking...Since the changes only happend on the implementation files.

But...Actually seems this change will not provide any benefit except for contributor readability, and at that time I have more things to do. So I just forget this because I guess maybe it's in a relative low priority.... 😅

bpoplauschi · 2018-08-02T07:59:45Z

@gukemanbu I didn't properly look at your PR the last time, sorry for that.
Thanks for your interest in our project and the contributions you made.

Regarding this PR here, I agree with you. The code design was not that great, I think your's is much cleaner. I will look at this one more time and try to merge it.

remove addProgressCallback, add createDownloaderOperationWithUrl

bpoplauschi · 2018-08-02T08:28:06Z

I did merge it - so thanks @gukemanbu

I did not move the cancel method so it's easier to see the actual diff
for the Downloader tests, I actually kept the original idea: check if passing a nil url will call the completion with all nils. Also added a check for the createDownloaderOperationWithUrl: non-nil result

dreampiggy · 2018-08-02T09:27:30Z

@gukemanbu This refactory, will not be available for 5.x branch. Now master and 5.x are separate and not mergable. If you wish to keep this design, please provide a new PR for 5.x branch as well.

bpoplauschi · 2018-08-02T09:34:11Z

@dreampiggy I think this change is not a big one. We will need to merge master into 5.x anyway before the final release.
But if @gukemanbu is so kind to make a PR for the 5.x branch, that would be appreciated.

dreampiggy · 2018-08-02T09:42:38Z

@bpoplauschi The diff is hard..I try to merge one, but however, this are many conflicts, nearlly 20+ files. 😅

I suggest to view the diff result, but using git diff and copy the code instead...Especially any changes into SDImageCoderXXX, because they've been renamed and git will recognize it as a new file.

bpoplauschi · 2018-08-02T09:53:31Z

Hmm, I see only 6 files with conflict. I see d5b60c6 commit was a merge from master to 5.x which you made (many thanks for that) on Jul 18th. So there are only a handful of commits since then.

bpoplauschi · 2018-08-02T09:53:43Z

I will look into this merge.

dreampiggy · 2018-08-02T10:04:48Z

@bpoplauschi You can try simulate to merge #2399 #2409 together...Because these PR change the SDImageCoder and SDWebImageDownloader...So that they will cause a full conflict(a new file marked as green and another file marked as red 😅 ) because of renaming...

Anyway, I don't mean impossible, but it's just need more time, maybe I can try provide a merge.

dreampiggy · 2018-08-02T10:21:57Z

Tests/Tests/SDWebImageDownloaderTests.m

-                                           completedBlock:(SDWebImageDownloaderCompletedBlock)completedBlock
-                                                   forURL:(nullable NSURL *)url
-                                           createCallback:(SDWebImageDownloaderOperation *(^)(void))createCallback;
+- (SDWebImageDownloaderOperation *)createDownloaderOperationWithUrl:(nullable NSURL *)url options:(SDWebImageDownloaderOptions)options;


This return value, in 4.x, should be NSOpration<SDWebImageDownloadOperationInterface>.

Maybe we can create another commit to fix it, leave it is OK as well.

Anyway, I fix this during 5.x merge.

It's fixed in the merge commit. Here we still see the diff with the original branch, without the conflicts fixed.

dreampiggy · 2018-08-02T10:23:03Z

Tests/Tests/SDWebImageDownloaderTests.m

-    } forURL:nil createCallback:nil];
-    [self waitForExpectationsWithTimeout:0.5 handler:nil];
+- (void)test07ThatCreateDownloaderOperationWithNilUrl {
+    [[SDWebImageDownloader sharedDownloader] createDownloaderOperationWithUrl:nil options:0];


I think, if we change the method to just createDownloaderOperationWithUrl. The url arg should be nonnull, because when the url is nil, the caller method downloadImageWithURL: already handle this case and earily return the response. So actually this should be removed.

I've also fixed this in the merge commit. Check that. 676a4b5

dreampiggy · 2018-08-02T10:46:11Z

@bpoplauschi I merge the lastest master to 5.x in 8b7e88d. Actually the changes is only about this PR (Another changes which apply to FLAnimatedImage, is already fixed in SDWebImageFLPlugin).

Actually, just some code movement, this PR :)

bpoplauschi · 2018-08-02T11:36:29Z

Yeap. Thanks @dreampiggy for doing the merge. I was doing it myself, but that's ok.

gukemanbu · 2018-08-10T02:33:36Z

Thank you all!

bpoplauschi · 2018-08-10T14:23:05Z

@gukemanbu Thanks a lot for showing interest and contributing to SDWebImage!
We would like to invite you to become a maintainer – no pressure to accept! At this point we are only a handful of people doing this in our spare time.
I am working on writing down the expectations from contributors, but basically

we want contributors to provide ideas, keep the ship shipping and to take some of the load from others. It is non-obligatory; we’re here to get things done in an enjoyable way.

Pitch in with what seems comfortable: comment on open issues/PRs, triage, improve documentation, write your own PRs.
Let us know.

Quote from Moya Contributing guide.

bpoplauschi · 2018-10-24T08:39:18Z

@gukemanbu did you see my invitation addressed to you above?

gukemanbu · 2018-10-25T10:03:21Z

@gukemanbu did you see my invitation addressed to you above?

@bpoplauschi I'm too busy these days to see it in time, Sorry for the late reply, I'd love to become a maintainer.

bpoplauschi · 2018-10-25T14:50:10Z

Awesome @gukemanbu. I sent you an invitation to the organization.

gukemanbu · 2018-10-26T01:33:41Z

@bpoplauschi Thank you very much，I have accepted the invitation。

xucg added 2 commits May 24, 2018 16:50

remove addProgressCallback, add createDownloaderOperationWithUrl

6ab801b

add unit test

91a8227

dreampiggy added the refactoring label May 30, 2018

Merge branch 'master' into xucg

2338871

bpoplauschi merged commit 2338871 into SDWebImage:master Aug 2, 2018

bpoplauschi added a commit that referenced this pull request Aug 2, 2018

Merge pull request #2336 from gukemanbu/xucg

676a4b5

remove addProgressCallback, add createDownloaderOperationWithUrl

bpoplauschi added this to the 4.4.3 milestone Aug 2, 2018

dreampiggy reviewed Aug 2, 2018

View reviewed changes

gukemanbu deleted the xucg branch October 25, 2018 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove addProgressCallback, add createDownloaderOperationWithUrl #2336

remove addProgressCallback, add createDownloaderOperationWithUrl #2336

gukemanbu commented May 24, 2018 •

edited

gukemanbu commented May 24, 2018 •

edited

codecov-io commented May 24, 2018 •

edited

bpoplauschi commented Jul 11, 2018

dreampiggy commented Jul 11, 2018

bpoplauschi commented Aug 2, 2018

bpoplauschi commented Aug 2, 2018

dreampiggy commented Aug 2, 2018 •

edited

bpoplauschi commented Aug 2, 2018

dreampiggy commented Aug 2, 2018 •

edited

bpoplauschi commented Aug 2, 2018

bpoplauschi commented Aug 2, 2018

dreampiggy commented Aug 2, 2018 •

edited

dreampiggy Aug 2, 2018

bpoplauschi Aug 2, 2018

dreampiggy Aug 2, 2018 •

edited

bpoplauschi Aug 2, 2018

dreampiggy commented Aug 2, 2018 •

edited

bpoplauschi commented Aug 2, 2018

gukemanbu commented Aug 10, 2018

bpoplauschi commented Aug 10, 2018

bpoplauschi commented Oct 24, 2018

gukemanbu commented Oct 25, 2018 •

edited

bpoplauschi commented Oct 25, 2018

gukemanbu commented Oct 26, 2018

remove addProgressCallback, add createDownloaderOperationWithUrl #2336

remove addProgressCallback, add createDownloaderOperationWithUrl #2336

Conversation

gukemanbu commented May 24, 2018 • edited

New Pull Request Checklist

Pull Request Description

gukemanbu commented May 24, 2018 • edited

codecov-io commented May 24, 2018 • edited

Codecov Report

bpoplauschi commented Jul 11, 2018

dreampiggy commented Jul 11, 2018

bpoplauschi commented Aug 2, 2018

bpoplauschi commented Aug 2, 2018

dreampiggy commented Aug 2, 2018 • edited

bpoplauschi commented Aug 2, 2018

dreampiggy commented Aug 2, 2018 • edited

bpoplauschi commented Aug 2, 2018

bpoplauschi commented Aug 2, 2018

dreampiggy commented Aug 2, 2018 • edited

dreampiggy Aug 2, 2018

Choose a reason for hiding this comment

bpoplauschi Aug 2, 2018

Choose a reason for hiding this comment

dreampiggy Aug 2, 2018 • edited

Choose a reason for hiding this comment

bpoplauschi Aug 2, 2018

Choose a reason for hiding this comment

dreampiggy commented Aug 2, 2018 • edited

bpoplauschi commented Aug 2, 2018

gukemanbu commented Aug 10, 2018

bpoplauschi commented Aug 10, 2018

bpoplauschi commented Oct 24, 2018

gukemanbu commented Oct 25, 2018 • edited

bpoplauschi commented Oct 25, 2018

gukemanbu commented Oct 26, 2018

gukemanbu commented May 24, 2018 •

edited

gukemanbu commented May 24, 2018 •

edited

codecov-io commented May 24, 2018 •

edited

dreampiggy commented Aug 2, 2018 •

edited

dreampiggy commented Aug 2, 2018 •

edited

dreampiggy commented Aug 2, 2018 •

edited

dreampiggy Aug 2, 2018 •

edited

dreampiggy commented Aug 2, 2018 •

edited

gukemanbu commented Oct 25, 2018 •

edited