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

Notification hooks are not removed #13

Open
CodeWriter23 opened this issue Jul 14, 2017 · 1 comment
Open

Notification hooks are not removed #13

CodeWriter23 opened this issue Jul 14, 2017 · 1 comment
Assignees

Comments

@CodeWriter23
Copy link

CodeWriter23 commented Jul 14, 2017

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 dlo self-assigned this Jul 14, 2017
@dlo
Copy link
Member

dlo commented Jul 14, 2017

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 ;)

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

No branches or pull requests

2 participants