-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix Element Call closing automatically on API 34 #3402
Conversation
It seems like registering a user leave hint listener way too early was causing the activity to try to enter PiP erroneously and that led to the activity closing instead.
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3402 +/- ##
===========================================
- Coverage 82.62% 82.62% -0.01%
===========================================
Files 1694 1694
Lines 39684 39685 +1
Branches 4831 4831
===========================================
Hits 32788 32788
- Misses 5204 5205 +1
Partials 1692 1692 ☔ View full report in Codecov by Sentry. |
val onUserLeaveHintListener = Runnable { | ||
pipEventSink(PictureInPictureEvents.EnterPictureInPicture) | ||
var onUserLeaveHintListener by remember { mutableStateOf<Runnable?>(null) } | ||
DisposableEffect(lifecycleState.isAtLeast(Lifecycle.State.STARTED)) { |
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 understand the DisposableEffect(lifecycleState.isAtLeast(Lifecycle.State.STARTED))
, this will still launch the effect with value false
, and then with value true
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 just realised I had added the wrong check to the if
above (copy&paste issue), so now it should:
- Launch the side effect with
false
, but do nothing. - Launch the side effect again with
true
, this time register the listener. - If we go out of the call and then back again, the listener won't be registered again.
- When the activity is finished the listener will also be removed.
I tried with simpler versions using LaunchedEffect
and a lifecycle observer, but neither of them removed the listener when the activity was closed. Although maybe as the activity is being closed, it's not needed at all.
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.
Actually, I was able to simplify it in b791c54. It seems like using lifecycleScope.launch
is enough to make it wait until the UI is ready.
2750bd4
to
b791c54
Compare
Quality Gate passedIssues Measures |
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.
My test is now passing.
FTR, starting a call on API 34 was failing, since the Activity was finished just after being started.
Content
The leave hint listener for PiP in the Call screen is now registered once the UI is ready.
Motivation and context
It seems like registering a user leave hint listener way too early was causing the activity to try to enter PiP erroneously and that led to the activity closing instead.
Tests
Tested devices
Checklist