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

Fix reference counting #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

aspcartman
Copy link

I don't know how this could possibly work, but if you create a new application, add this lib and just try to BINDO with -observe: it won't work, because right after the creation of the binding it gets deallocated: BNDBinding is not retained. You had an NSMutableSet, where you add BNDBindings, but now it's gone and only BNDBindingKVOObserver are retained by association. But they have a weak reference to parent BNDBinding. That's a fail.

To fix this we must make BNDBindingKVOObservers to hold strong reference to parent BNDBinding.

I don't know how this could possibly work, but if you create a new application, add this lib and just try to BINDO with -observe: it won't work, because right after the creation of the binding it gets deallocated: `BNDBinding` is not retained. You had an `NSMutableSet`, where you add `BNDBinding`s, but now it's gone and only `BNDBindingKVOObserver` are retained by association. But they have a weak reference to parent `BNDBinding`. That's a fail.

To fix this we must make `BNDBindingKVOObserver`s to hold strong reference to parent `BNDBinding`.
Since BNDBinding.left/rightObserver are now weak, we can't just assign them.
@aspcartman
Copy link
Author

Ping, maaan

@markohlebar
Copy link
Owner

Hey @aspcartman sorry I'm super busy with other stuff. Had a brief look at it, and I think you might be right. The way we're using BINDO is always by keeping the reference to the BNDBinding that gets returned. I need more time to test your fix though, thanks for spotting this.

@aspcartman
Copy link
Author

@markohlebar thanks :)

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.

None yet

2 participants