lionheart / LHSKeyboardAdjusting

An easy-to-use Objective-C protocol that automatically resizes / adjusts views when a keyboard appears on iOS.
http://lionheartsw.com
Apache License 2.0
100 stars 10 forks source link

Notification hooks are not removed #13

Open CodeWriter23 opened 7 years ago

CodeWriter23 commented 7 years ago

Hi,

I needed a different design with keyboard handling, so I ended up writing my own. Thanks for this great example, I learned a lot by reading it.

I did want to point out, LHSKeyboardAdjusting uses the block-oriented methods for adding and removing observers for notifications, and it conflates the calling conventions with the target:selector variety of these methods. As a result, when a view controller disappears and calls the lhs_deactivateKeyboardAdjustment method to unhook, the notification observers are not actually removed.

The block-oriented API docs for addObserverForName:object:queue:usingBlock:

Return Value An opaque object to act as the observer.

and

To unregister observations, you pass the object returned by this method to removeObserver:

https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc

- (void)lhs_deactivateKeyboardAdjustment {
  [[NSNotificationCenter defaultCenter] removeObserver:**self** name:UIKeyboardWillHideNotification object:nil];
  [[NSNotificationCenter defaultCenter] removeObserver:**self** name:UIKeyboardDidShowNotification object:nil];
}

"self" is not the observer you are looking for :)

This is what I did in my code. The properties (i.e., .willShowObserver, etc.) are implemented with getter/setters that use ObjC Associated Objects for storage. I apologize in advance if that offends your sense of style. For me, anything is better than coding by punching bytes in 8 switches at a time via front panel.

- (void)activateKeyboardShow:(void(^)(BOOL beforeAfter, NSDictionary *userInfo))showBlock hide:(void(^)(BOOL beforeAfter, NSDictionary *userInfo))hideBlock {
    NSNotificationCenter *center = [NSNotificationCenter defaultCenter];
    UIViewController *__weak weakSelf = self;

    self.willShowObserver = [center addObserverForName: UIKeyboardWillShowNotification
                                                object: nil
                                                 queue: [NSOperationQueue mainQueue]
    usingBlock:^(NSNotification * _Nonnull note) {
        [weakSelf keyboardWillShow: note block: showBlock];
    }];

    self.willHideObserver = [center addObserverForName: UIKeyboardWillHideNotification
                                                object: nil
                                                 queue: [NSOperationQueue mainQueue]
    usingBlock:^(NSNotification * _Nonnull note) {
        [weakSelf keyboardWillHide: note block: hideBlock];
    }];
}

- (void)deactivateKeyboardEvents {
    NSNotificationCenter *center = [NSNotificationCenter defaultCenter];

    [center removeObserver: self.willShowObserver];
    [center removeObserver: self.willHideObserver];
}

Additionally, the notification blocks have a retain cycle in them as they reference self instead of __weak reference to self.

[[NSNotificationCenter defaultCenter] addObserverForName:UIKeyboardDidShowNotification
  | object:nil
  | queue:[NSOperationQueue currentQueue]
  | usingBlock:^(NSNotification * _Nonnull note) {
  | if (show) {
  | show();
  | }
  |  
  | [**self** lhs_keyboardDidShow:note];
  | }];

I've gone in my own direction with this code; I apologize in advance that my goodwill is limited to this bug report and examples from my own code instead of an actual pull request. Major deadlines loom for me, so it's the best I can do.

Thanks again for your code, and for unknowingly being my teacher.

dlo commented 7 years ago

This is great, thanks for the heads up! Those block-based methods always have a gotcha. I'll fix this up, and I'm humbled to have been an inspiration for your implementation ;)