-
Notifications
You must be signed in to change notification settings - Fork 25
fix: Call UI not maximizing after account switch - WPB-20468 #3897
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
base: develop
Are you sure you want to change the base?
Conversation
Test Results3 166 tests 3 139 ✅ 3m 55s ⏱️ Results for commit 9e9282c. ♻️ This comment has been updated with latest results. |
| conversationVisibleObserver = NotificationCenter.default.addObserver( | ||
| forName: .conversationDidBecomeVisible, | ||
| object: nil, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| guard let self else { return } | ||
|
|
||
| if let observer = conversationVisibleObserver { | ||
| NotificationCenter.default.removeObserver(observer) | ||
| conversationVisibleObserver = nil | ||
| } | ||
| presentationDelegate?.updateActiveCallPresentationStateIfNeeded() | ||
| } |
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.
question: can we move this inside the user session? The session manager shouldn't be concerned about this so it'd be good to avoid it if possible.
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.
Well, I can share my thoughts on why I would prefer to keep it in SessionManager, but I'm open to your suggestions.
SessionManager owns presentationDelegate that already has API for UI communication in a context of account coordination/switching.
This is a use case specifically for switching between accounts and SessionManager orchestrating it.
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 also moved the code around and added more comments
| private func notifyConversationDidBecomeVisible() { | ||
| NotificationCenter.default.post( | ||
| name: .conversationDidBecomeVisible, | ||
| object: conversation |
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.
suggestion: it doesn't seem necessary to pass the conversation, perhaps you can remove it?
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.
Yes, removed
johnxnguyen
left a comment
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'm not a big fan of posting notifications like this but I understand an alternative solution must be tricky. Just so I understand, what's stopping us from awaiting the completion of showing the conversation directly? i.e something like
showConversation(completion: {
// do something
})
Issue
When receiving a call on an inactive account, the app switches to the correct account, but only shows the green top bar.
From the UI perspective the flow is
The calling UI is shown before the conversation navigation completes, and then the conversation appearing dismisses the calling UI.
updateActiveCallPresentationState()is called inside the loadSession flow (activateAccount ->select() -> loadSession ) before the completion executes to show the conversation. The conversation navigation then interferes with the already shown calling UI.Solution
Display the calling UI after the conversation is fully visible.
SessionManager.activateAccount()and triggersupdateActiveCallPresentationState()at the right time - after conversation is visibleTesting
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: