-
Notifications
You must be signed in to change notification settings - Fork 28
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
Allow begin observing the keyboard notifications without the view being loaded #199
Conversation
Any reason not to use a module initializer (like the second answer here) and set this up automatically? |
@kylebshr: Do you mean the |
Yep - won't that get called when the module is loaded? (disclaimer I have no idea what I'm talking about) |
@kylebshr So the C-style constructors should generally be avoided in this kind of case; since this is calling across ObjC types and across frameworks. If you're doing purely local, C-only setup, the constructor call would work, but in this case, we need to wait until the runtime gives us a chance to set things up. |
Listable/Sources/Internal/KeyboardObserver/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
Listable/Sources/Internal/KeyboardObserver/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
Listable/Sources/Internal/KeyboardObserver/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
fd8a7ca
to
96e463a
Compare
@kylebshr: Mind giving this a look today? I'd like to merge today to unblock Jason |
/// it's possible (but unlikely) that changing the keyboard visibility on a list could cause | ||
/// _another_ list to be allocated, which would mutate the `self.delegates` array | ||
/// while we are mutating it, violating Swift's exclusive access clause. | ||
let delegates = self.delegates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know a ton about exclusive access, but isn't this a non-issue because all these changes would happen on a main thread? I don't see how there are conflicting accesses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can happen due to nested accesses as well, eg:
self.things.forEach { thing in
self.things.append(newThing)
}
Though after testing it, this no longer happens for arrays; I think they're smart about arrays now, likely since they're COW internally (but this was an issue with the original exclusive access implementation). Going to remove this bit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I ran into this last week with the VisibleContent
struct, which is why I did this here, but yeah, seems like arrays are somehow immune, as are im guessing all the stdlib collections.)
|
||
/// Called by `ListView` on setup, to warn developers | ||
/// if something has gone wrong with keyboard setup. | ||
static func logKeyboardSetupWarningIfNeeded() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think the logging is still needed now that set up is automatic? Is there a case where this wouldn't get set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I'm doing something that's a little clever to get the registration working, I wanted to add an obvious log statement if it stops working for whatever reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable 👍
@kyleve totally looked last night and forgot to approve, had some questions but no need to block |
96e463a
to
c0aa982
Compare
So this is kinda gross, but, no other great solution.
Problem: We receive keyboard frames based on system NSNotifcations. If the keyboard is already visible when a list is created, we previously had no way of knowing what the current keyboard frame was, and thus, we had no way of knowing how to adjust the contentInset of the list view to avoid keyboard overlap.
Solution: On app startup, register for keyboard notifications, and use a single shared observer.