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

clean up gesture recognizers #80

Merged
merged 2 commits into from Feb 13, 2015
Merged

clean up gesture recognizers #80

merged 2 commits into from Feb 13, 2015

Conversation

myeyesareblind
Copy link

I got a weird crash when I changed collectionViewFlowLayout dynamically.
Current version leaves gestures alive, so when the collectionView is tapped, gestureRecognizer will send message to zombie.

@bartoszhernas
Copy link

I have the same issue, but your fix is not helping. I am setting this layout in Storyboard and then changing it dynamically and with your fix its still crashing.

@lxcid
Copy link
Owner

lxcid commented Feb 12, 2015

Do you have a demo project that demonstrate this? If you have I can review it.

@bartoszhernas
Copy link

If you add to your example something like

- (void)viewDidAppear:(BOOL)animated {
    [super viewDidAppear:animated];
    self.collectionView.collectionViewLayout = [LXReorderableCollectionViewFlowLayout new];
    self.collectionView.collectionViewLayout = [LXReorderableCollectionViewFlowLayout new];
}

It will crash then

@lxcid
Copy link
Owner

lxcid commented Feb 12, 2015

Noted. Thanks. Will give it a try and get back to you guys.

@myeyesareblind
Copy link
Author

@BartG , thank you, my first commit didn't fix the issue.
The problem is that in dealloc, collection view is already nil, so the gestures still exists.

@myeyesareblind
Copy link
Author

I won't do that, feel free to contribute.

@lxcid
Copy link
Owner

lxcid commented Feb 13, 2015

@BartG That was what I was thinking as well.

I was thinking of the following changes but some of it I'm currently not confident enough about them. I'm tagging LOW, MED, HIGH as an indication of my confidence level of each point.

  • [LOW] properties holding gesture recognisers should be change to weak, from strong.
    The reason for this is that the ownership of the gesture recognisers should not be shared between the layout and the collection view. It is technically owned by the collection view and reference from the layout. But the code written in place do not expect these properties to be weak in nature, and the change might cause unexpected behaviour. So I'm letting it slip.
  • [MED] Introduces -[LXReorderableCollectionViewFlowLayout tearDownCollectionView] which wraps @BartG solution but with some if guards. (I prefer to be extra caution) It should be the opposite of -[LXReorderableCollectionViewFlowLayout setupCollectionView].
  • [MED] Have -[LXReorderableCollectionViewFlowLayout tearDownCollectionView] be called in both dealloc and KVO of collectionView properties.

@myeyesareblind
Copy link
Author

@lxcid
I like 2-3, this way you are making it obvious that there is some important code to be run on dealloc.

lxcid added a commit that referenced this pull request Feb 13, 2015
lxcid added a commit that referenced this pull request Feb 13, 2015
@lxcid
Copy link
Owner

lxcid commented Feb 13, 2015

Thanks @myeyesareblind, didn't realise its your changes. I have pull from your repro and work on top of your changes. :)

337711b

Could you help me review if this look good? My initial test seems to work. Let me know if you find any quirkiness. If all is well I'll merge to master and release a tag.

/cc @BartG

@myeyesareblind
Copy link
Author

It's good

@lxcid lxcid mentioned this pull request Feb 13, 2015
@lxcid lxcid merged commit 9197b44 into lxcid:master Feb 13, 2015
@lxcid
Copy link
Owner

lxcid commented Feb 13, 2015

Thanks for the help! Its merged!

gshaviv added a commit to gshaviv/LXReorderableCollectionViewFlowLayout that referenced this pull request Mar 27, 2016
* commit '58b772eea6a38009cdfb8004398a864cae6cb231':
  [BUGFIX lxcid#80] Clean up gestures when collectionView property is nil'ed or layout is deallocated.
  really clean-up gesture recognizers
  clean up gesture recognizers
  Uses `- [CADisplayLink duration]` to calculate the distance to scroll.
  [Issue lxcid#49] Replaces `LX_rasterizedImage` with `LX_snapshotView` which attempts to use the iOS 7 snapshot function whenever possible.
  Fixed warnings in the example project when compiling for 64-bit architectures. Also, update a bit of the convention in the example project to be more modern and to my taste. :P
  Fixed bug of multiple cells on rapid tapping.

# Conflicts:
#	LXReorderableCollectionViewFlowLayout/LXReorderableCollectionViewFlowLayout.m
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

4 participants