Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fix issue #2739 #2741

Merged
merged 5 commits into from May 26, 2015
Merged

Fix issue #2739 #2741

merged 5 commits into from May 26, 2015

Conversation

bnickel
Copy link
Contributor

@bnickel bnickel commented May 20, 2015

This introduces associated objects so notifications can be cleared in dealloc without clobbering the class's dealloc.

Added associated activity view animator so notifications can be unsubscribed from without clobbering the default `dealloc`.
@kcharwood
Copy link
Contributor

Should we just swizzle out dealloc in the category? I'm always hesitant to swizzle unless there is a better option, but it seems like the cleaner route here.

@bnickel
Copy link
Contributor Author

bnickel commented May 20, 2015

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.

@kcharwood
Copy link
Contributor

I'll dig in a bit more. I can revert #2685 in the near term.

@kcharwood
Copy link
Contributor

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.

@kcharwood kcharwood added the bug label May 20, 2015
@kcharwood kcharwood added this to the 2.5.5 milestone May 20, 2015
@bnickel
Copy link
Contributor Author

bnickel commented May 20, 2015

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 dealloc is itself a problem so I don't think we need to expand that.

@kcharwood
Copy link
Contributor

Good info. Thanks

@bnickel
Copy link
Contributor Author

bnickel commented May 20, 2015

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"
Copy link
Member

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?

Copy link
Contributor Author

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.

@cnoon
Copy link
Member

cnoon commented May 22, 2015

Could you fix the -(void)dealloc spacing on both the animator classes? I know you didn't make the change, but we should fix it before I forget!

@cnoon
Copy link
Member

cnoon commented May 22, 2015

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!

@kcharwood
Copy link
Contributor

@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 AFRefreshControlNotificationObserver and AFActivityIndicatorViewNotificationObserver?

@kcharwood
Copy link
Contributor

Thanks for your work here @bnickel

I'm going to merge this in now to squash this crash.

🍻

kcharwood added a commit that referenced this pull request May 26, 2015
@kcharwood kcharwood merged commit cf78b03 into AFNetworking:master May 26, 2015
@kcharwood kcharwood modified the milestones: 2.5.5, 2.6.0 Jun 26, 2015
@bnickel bnickel deleted the issue2739 branch July 27, 2015 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants