-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Conversation
Added associated activity view animator so notifications can be unsubscribed from without clobbering the default `dealloc`.
Should we just swizzle out |
Some quick googling of "swizzling dealloc" suggests it is not a good idea for production code. Associated objects are a reliable way to get notified of an object's deallocation. |
I'll dig in a bit more. I can revert #2685 in the near term. |
First time I have it done this way, and it's actually not as bad as I thought it would be. I might want to change a few names, and can you add some test coverage to the refresh control as well? Also, drop a comment in those tests with this issue number so we can have a bread crumb trail pointing back here. |
I tried testing the refresh control but I couldn't trigger a crash. As far as I can tell it doesn't have any notifications so the only thing a test could prove is wrong would be hypothetical memory leaks. We know clobbering |
Good info. Thanks |
Added a comment with the issue number. |
|
||
#pragma clang diagnostic push | ||
#pragma clang diagnostic ignored "-Wreceiver-is-weak" | ||
#pragma clang diagnostic ignored "-Warc-repeated-use-of-weak" |
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.
How about pulling the activityIndicatorView
into a local strong ref here to avoid the clang pragmas? It would be cleaner, safer and is more of a general practice. It shouldn't result in a retain cycle since it is only a temporary retain.
- (void)setAnimatingWithStateOfTask:(NSURLSessionTask *)task {
UIActivityIndicatorView *activityIndicatorView = self.activityIndicatorView;
NSNotificationCenter *notificationCenter = [NSNotificationCenter defaultCenter];
[notificationCenter removeObserver:self name:AFNetworkingTaskDidResumeNotification object:nil];
[notificationCenter removeObserver:self name:AFNetworkingTaskDidSuspendNotification object:nil];
[notificationCenter removeObserver:self name:AFNetworkingTaskDidCompleteNotification object:nil];
if (task) {
if (task.state != NSURLSessionTaskStateCompleted) {
if (task.state == NSURLSessionTaskStateRunning) {
[activityIndicatorView startAnimating];
} else {
[activityIndicatorView stopAnimating];
}
[notificationCenter addObserver:self selector:@selector(af_startAnimating) name:AFNetworkingTaskDidResumeNotification object:task];
[notificationCenter addObserver:self selector:@selector(af_stopAnimating) name:AFNetworkingTaskDidCompleteNotification object:task];
[notificationCenter addObserver:self selector:@selector(af_stopAnimating) name:AFNetworkingTaskDidSuspendNotification object:task];
}
}
}
If we decide to go this route, it should be applied to all changes in this PR in both the UIActivityIndicatorView
and the UIRefreshControl
.
Thoughts?
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 reason I did it this way is simply that adding a strong variable adds no real value other than its indirect silencing of warnings and it seems like a pattern of this project to directly silence warnings on a one-off basis rather than adapt the code or build flags.
The warning "Weak property may be unpredictably set to nil" is still technically valid unless we do if (activityIndicatorView)
. This is unnecessary in our case because the only time it could not be there is in the async dispatches and sendmsg will handle nil
. arc-repeated-use-of-weak
is really a compiler issue since its only ever used in once per branch.
Whatever y'all decide I'm fine with.
Could you fix the |
Overall this is a great set of changes from my POV. I only have the couple of things and I'm good with these changes @kcharwood. Thanks for this @bnickel! |
@bnickel I think I'd like to see the class names changed as well to better describe what they are actually doing. I'm thinking |
Thanks for your work here @bnickel I'm going to merge this in now to squash this crash. 🍻 |
This introduces associated objects so notifications can be cleared in dealloc without clobbering the class's dealloc.