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

Allow begin observing the keyboard notifications without the view being loaded #199

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Aug 24, 2020

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.

@kyleve kyleve requested a review from kylebshr August 25, 2020 00:33
@kylebshr
Copy link
Collaborator

Any reason not to use a module initializer (like the second answer here) and set this up automatically?

@kyleve
Copy link
Collaborator Author

kyleve commented Aug 25, 2020

@kylebshr: Do you mean the static void __attribute__ ((constructor)) Initer() bit?

@kylebshr
Copy link
Collaborator

Yep - won't that get called when the module is loaded? (disclaimer I have no idea what I'm talking about)

@kyleve
Copy link
Collaborator Author

kyleve commented Aug 25, 2020

@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.

@kyleve kyleve force-pushed the kve/begin-observing-kb branch from fd8a7ca to 96e463a Compare August 25, 2020 19:56
@kyleve
Copy link
Collaborator Author

kyleve commented Aug 26, 2020

@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
Copy link
Collaborator

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

Copy link
Collaborator Author

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!

https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Copy link
Collaborator Author

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() {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable 👍

@kylebshr
Copy link
Collaborator

@kyleve totally looked last night and forgot to approve, had some questions but no need to block

@kyleve kyleve force-pushed the kve/begin-observing-kb branch from 96e463a to c0aa982 Compare August 26, 2020 18:27
@kyleve kyleve merged commit 09e1eff into main Aug 26, 2020
@kyleve kyleve deleted the kve/begin-observing-kb branch August 26, 2020 19:25
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.

2 participants