-
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
Use new functions exposed by Element Call about PiP #3334
Conversation
Also only use eventSink to communicate with the Presenter, instead of having public methods. Change WeakReference to an Activity to a listener and update tests.
📱 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 #3334 +/- ##
===========================================
- Coverage 82.75% 82.69% -0.07%
===========================================
Files 1680 1681 +1
Lines 39382 39415 +33
Branches 4777 4787 +10
===========================================
+ Hits 32591 32594 +3
- Misses 5117 5146 +29
- Partials 1674 1675 +1 ☔ View full report in Codecov by Sentry. |
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.
Some remarks, let me know what you think
|
||
package io.element.android.features.call.impl.utils | ||
|
||
interface WebPipApi { |
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 find the naming a bit weird, especially with the default implementation being WebViewWebPipApi
.
I'd have call it PipController
maybe?
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.
OK, done
|
||
package io.element.android.features.call.impl.pip | ||
|
||
interface PipActivity { |
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 really like the fact we leak the fact it's an Activity, can we just try to find another name?
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.
PipView
maybe?
@@ -66,6 +74,7 @@ class ElementCallActivity : AppCompatActivity(), CallScreenNavigator { | |||
private val webViewTarget = mutableStateOf<CallType?>(null) | |||
|
|||
private var eventSink: ((CallScreenEvents) -> Unit)? = null | |||
private var pipEventSink: ((PictureInPictureEvents) -> Unit)? = null |
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.
Instead of keeping a reference to the eventSink in a seperate value, I'd instead keep everything alive inside the content block :
setContent {
val pipState = pictureInPicturePresenter.present()
val pipEventSink by rememberUpdatedState(pipState.eventSink)
DisposableEffect(Unit) {
val onUserLeaveHintListener = Runnable {
pipEventSink(PictureInPictureEvents.EnterPictureInPicture)
}
val onPictureInPictureModeChangedListener = Consumer { value: PictureInPictureModeChangedInfo ->
pipEventSink(PictureInPictureEvents.OnPictureInPictureModeChanged(value.isInPictureInPictureMode))
}
addOnPictureInPictureModeChangedListener(onPictureInPictureModeChangedListener)
addOnUserLeaveHintListener(onUserLeaveHintListener)
onDispose {
removeOnPictureInPictureModeChangedListener(onPictureInPictureModeChangedListener)
removeOnUserLeaveHintListener(onUserLeaveHintListener)
}
}
[...]
}
Quality Gate passedIssues Measures |
Content
Motivation and context
Closes #3326
Screenshots / GIFs
Tests
Note: using regular (so not deployed) Element Call should not be impacted by this change.
Tested devices
Checklist