-
Notifications
You must be signed in to change notification settings - Fork 510
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
Set priority of download data tasks at the right time #490
Conversation
🚫 CI failed with log |
…e the lib should do the right thing out of the box
@@ -18,7 +18,7 @@ typedef void (^PINRemoteImageDownloadCompletion)(NSURLResponse * _Nullable respo | |||
|
|||
@interface PINRemoteImageDownloadQueue : NSObject | |||
|
|||
@property (nonatomic, assign) NSUInteger maxNumberOfConcurrentDownloads; | |||
@property (atomic, assign) NSUInteger maxNumberOfConcurrentDownloads; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is actually thread-safe so I thought it'd be a good idea to let clients know that.
🚫 CI failed with log |
NSURLSessionDataTask *dataTask = self->_progressImage.dataTask; | ||
if (dataTask) { | ||
dataTask.priority = dataTaskPriorityWithImageManagerPriority(priority); | ||
[self.manager.urlSessionTaskQueue setQueuePriority:priority forTask:dataTask]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No behavior change here, just cache the data task in a variable for better readability.
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 🤞
priority:priority | ||
completionHandler:^(NSURLSessionTask *task, NSError *error) { | ||
completionHandler(task.response, error); | ||
[self lock]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the indentation change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new priority param made this line too long so I break it into multiple lines hence the new indentation.
CI passed with build 625. |
Thanks for reviewing, @garrettmoon and @maicki. |
Currently the priority is set after a task has been scheduled and potentially sent off. It means the priority may not be picked up at all. This diff adds a new behavior that sets the priority right after the task is created.
Also expose an API that allows the priority to be set by Texture.