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

Add cancel method for PINRemoteImageManager #509

Merged
merged 4 commits into from
Jul 11, 2019

Conversation

zhongwuzw
Copy link
Contributor

Add cancel all tasks for PINRemoteImageManager, it's the convenience method for user to call all tasks one shot, don't need to maintain uuids by themself.

@ghost
Copy link

ghost commented Jul 8, 2019

🚫 CI failed with log

@bolsinga
Copy link
Contributor

bolsinga commented Jul 9, 2019

Please add tests.

@garrettmoon
Copy link
Collaborator

Hmm, I'm not sure this will work. If the UUIDs are stored in a weak hash table, PINRemoteImageManager will 'lose' references to requests that the caller doesn't strongly retain the UUID to…

@zhongwuzw
Copy link
Contributor Author

@garrettmoon 🤔 Emm, PINRemoteImageTask would retain UUID by _callbackBlocks.

@zhongwuzw
Copy link
Contributor Author

@garrettmoon And I may have a thought to optimize _locked_taskForUUID:key: of PINRemoteManager by add a weak to weak NSMapTable(uuid to task) and exposure key of PINRemoteImageTask, what do you think? 🤔

@garrettmoon
Copy link
Collaborator

@garrettmoon 🤔 Emm, PINRemoteImageTask would retain UUID by _callbackBlocks.

You're right! Would you mind adding a comment to the code saying as much?

@garrettmoon
Copy link
Collaborator

@garrettmoon And I may have a thought to optimize _locked_taskForUUID:key: of PINRemoteManager by add a weak to weak NSMapTable(uuid to task) and exposure key of PINRemoteImageTask, what do you think? 🤔

Seems like a good optimization because it will also make the code clearer I think!

@zhongwuzw
Copy link
Contributor Author

@garrettmoon And I may have a thought to optimize _locked_taskForUUID:key: of PINRemoteManager by add a weak to weak NSMapTable(uuid to task) and exposure key of PINRemoteImageTask, what do you think? 🤔

Seems like a good optimization because it will also make the code clearer I think!

👌 I'll make a PR later~

Copy link
Collaborator

@garrettmoon garrettmoon left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution here!

@@ -695,6 +697,8 @@ - (NSUUID *)downloadImageWithURL:(NSURL *)url
}
[task addCallbacksWithCompletionBlock:completion progressImageBlock:progressImage progressDownloadBlock:progressDownload withUUID:UUID];
[self.tasks setObject:task forKey:key];
// Relax :), task retain the UUID for us, it's ok to have a weak reference to UUID here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great comment :)

@garrettmoon garrettmoon merged commit a5e7ae5 into pinterest:master Jul 11, 2019
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.

3 participants