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

Cleanup KVO when view is deallocating #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krizpoon
Copy link

May cause crash when OAStackView was deallocated but still observing subview's hidden property.

@samsymons
Copy link

Is there anything I can do to help get this PR merged?

@nsomar
Copy link
Owner

nsomar commented Feb 1, 2017

@samsymons Hey, thanks for the interest.
Do you think you can get the travis build green again :) that would be super helpful!

@samsymons
Copy link

@oarrabi Absolutely, I'll take a look into that — thanks! 😄

@samsymons
Copy link

I have not yet had time to fix up the CI issues with this PR but hope to look into that this week; sorry about that!

I also found an edge case with this fix, where views added multiple times to the stack view will not get properly deallocated after this fix in dealloc:

[self removeObserverForViews:self.subviews];

In this case, I had a label being added to the stack view multiple times, but that line above will only remove the observer for subviews. I fixed this crash in our project by updating the fix to remove the observer for all arranged subviews, not just self.subviews.

It seems that addArrangedSubview: is assuming that it is always getting a new item, so you can have a situation where the same view is added multiple times. Does it makes sense to have addArrangedSubview: check whether the newly added subview already exists in the arranged subviews array before adding it?

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