search

nsomar / OAStackView

Porting UIStackView to iOS 7+
MIT License
2.14k stars 191 forks source link

Cleanup KVO when view is deallocating #94

Open krizpoon opened 8 years ago

krizpoon commented 8 years ago

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

samsymons commented 7 years ago

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

nsomar commented 7 years ago

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

samsymons commented 7 years ago

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

samsymons commented 7 years ago

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?