Skip to content
This repository was archived by the owner on Oct 31, 2019. It is now read-only.

Fixes #21 #22

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

Fixes #21 #22

wants to merge 1 commit into from

Conversation

benoit-pereira-da-silva

We disable on UIKeyboardWillShowNotification and re-enable on
UIKeyboardDidHideNotification if necessary to prevent from an IOS SDK internal bug : UICollectionViewUpdateItem missing selector crash.

We disable on UIKeyboardWillShowNotification and re-enable on
UIKeyboardDidHideNotification if necessary
@lukescott
Copy link
Owner

  • What is _observerForUIKeyboard* for? You don't seem to do anything with these.
  • You can avoid the weakSelf stuff if you add a method to the class.
  • The observers are not cleaned up in the dealloc. [[NSNotificationCenter defaultCenter] removeObserver:self] would be enough.
  • There is a usability issue. What if the keyboard is up for a cell outside the UICollectionVIew? In my app not being able to drag would be unexpected behavior as the keyboard is always up. This would cause frustration for the user.

@benoit-pereira-da-silva
Copy link
Author

  • _observerForUIKeyboard is a weak reference to prevent from retaining the observer in ARC context, and they should be removed by calling removeObserver:_observerForUIKeyboard... you are right !
  • "You can avoid the weakSelf stuff if you add a method ..." yes you can if you prefer not to use blocks.
  • pushing a new controller with a visible keyboard is not really a clean way.
  • may a diacritic sign could be set to provide feed back on "non draggability"
  • i think that : objc_setAssociatedObject(self, "LSCollectionViewHelper", helper, OBJC_ASSOCIATION_RETAIN_NONATOMIC); should be "released" (but i m not 100% sure)

Sorry for the approximative english ( i'm a french frog)

@lukescott
Copy link
Owner

I've actually never used addObserverForName:object:queue:usingBlock:. Didn't realize that it returned the observer. I prefer addObserver:selector:name:object: because you can simply do removeObserver:self to remove all observers in one call. It ensures all future observers are cleaned up. In this case I prefer not to use blocks.

The LSCollectionViewHelper is released when your UICollectionView is, by association.

I feel there are too many edge cases with this patch. It would be better if:

  • This was optional - perhaps a "diableDraggingWhileEditing" property
  • It only did this when the first responder was a child of the UIViewController.
  • enabled should not be changed. It can be changed after the keyboard appears, and then overwritten when the keyboard is dismissed.

To be honest I feel like this issue may be beyond the scope of this addon. One thing I can do is add a "will begin dragging" delegate call so you can dismiss the keyboard if the user begins dragging. If the text field takes up the entire cell, it may be better just to keep draggable disabled until you enable it with a button or something. The long press may be interfering with the text loupe anyway.

@benoit-pereira-da-silva
Copy link
Author

Thanks for taking the time to analyze. The will begin dragging idea is not bad.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants