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

Delegate objects are nullified in dealloc() method #163

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

dodikk
Copy link

@dodikk dodikk commented Apr 1, 2015

Since delegates are __unsafe_unretained they should be set to nil manually whenever a corresponding object is released.
I've added dealloc method to all classes where .delegate = self; statement is used.

  • NXOAuth2Account
  • NXOAuth2Request

The users have to do that as well, so this should be mentioned in the README, I guess.

Note : this pull request is based on from #155

#import "NXOAuth2Account.h"
#import <OAuth2Client/NXOAuth2Account.h>

@protocol NXApplication;
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep in line with the framework this should be called NXOAuth2Application

@toto
Copy link
Contributor

toto commented Apr 27, 2015

Generally we could convert the delegates to weak at some point. iOS 4 compatibility is really not that important anymore

@dodikk
Copy link
Author

dodikk commented Apr 28, 2015

Generally we could convert the delegates to weak at some point.

That would be a lot better. Still, I did not feel confident enough to introduce such major changes since I have not fully understood the instance ownership model of the library. Neither I do now. Meaning, there are

So I've made the changes as conservative and safe as I could.

@dodikk
Copy link
Author

dodikk commented Apr 28, 2015

P.S. Most of UIKit classes still use unsafe_unretained delegate properties for backward compatibility. UITableView, for example.

Moreover, it does not matter that much for this library since NSNotificationCenter is used for interactions with the client's code.

@toto
Copy link
Contributor

toto commented Apr 28, 2015

True. You can use NXOAuth2Client on it's own without the NXOAuth2AccountStore and friends (indeed that was how it was first built) and in that case it works with delegates. If you use the higher level API as is appropriate in most use cases it's indeed not very relevant.

@sirnacnud
Copy link
Contributor

I pushed up #204, making the delegates weak.

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