Add missing PlayerView lifecycle handling (PiP #1) #85
Conversation
zigavehovec
changed the title
zigavehovec
changed the title
| private val activityLifecycle = | ||
| activity.let { it as? FlutterActivity ?: it as? FlutterFragmentActivity } | ||
| ?.lifecycle | ||
| ?: error( | ||
| "Trying to create an instance of ${this::class.simpleName}" + | ||
| " while not attached to a FlutterActivity or FlutterFragmentActivity", | ||
| ) |
There was a problem hiding this comment.
The section Alternatives to FlutterActivity here indicate that a Flutter view can be part of different types of activities. While this is probably not the common use case we could just use
(activity as? LifecycleOwner)
instead to be more resilient.
There was a problem hiding this comment.
I read through the docs and I don't expect any problems because of this. But I also see no issue in just using the LifecycleOwner observer cast, so let's do that just to be safe.
|
|
||
| fun Context.requireActivity(): Activity = | ||
| when (this) { | ||
| is Activity -> this | ||
| is ContextWrapper -> baseContext.requireActivity() | ||
| else -> error("No activity found for context $this") | ||
| } |
There was a problem hiding this comment.
NIT: I am not a big fan of a single big util file. As it is - for now - only used once, can we just have it private in the FlutterPlayerView.kt? If you already have another use case for this in mind, please create a more specific file like e.g. ContextExtension.kt.
There was a problem hiding this comment.
Maybe we can also have a slightly more verbose error message in case we don't have an activity. 🙂
There was a problem hiding this comment.
You're right, big utils class aren't pretty. Not sure yet if we'll need this again so I made it private in the same file.
d315606
| @@ -58,7 +85,7 @@ class FlutterPlayerView( | |||
| ) = when (call.method) { | |||
| Methods.ENTER_FULLSCREEN -> playerView.enterFullscreen() | |||
| Methods.EXIT_FULLSCREEN -> playerView.exitFullscreen() | |||
| Methods.DESTROY_PLAYER_VIEW -> { /* no-op */ } | |||
| Methods.DESTROY_PLAYER_VIEW -> Unit // no-op | |||
There was a problem hiding this comment.
Nice, I like this style more 👍
| ?: error("Trying to create an instance of ${this::class.simpleName} while not attached to an Activity") | ||
|
|
||
| private val activityLifecycle = | ||
| (activity as? LifecycleOwner) |
There was a problem hiding this comment.
The activity should always be null or a LifecycleOwner, right?
| (activity as? LifecycleOwner) | |
| (activity as LifecycleOwner?) |
There was a problem hiding this comment.
The activity should never be null at this point, but it might not extend LifecycleOwner.
| when (this) { | ||
| is Activity -> this | ||
| is ContextWrapper -> baseContext.getActivity() | ||
| else -> null |
There was a problem hiding this comment.
throw instead of returning null? this could even be inlined.
There was a problem hiding this comment.
I went for nullable approach to be able to provide a more descriptive error message where it's used (FlutterPlayerView.kt:46).
Since we might also need it somewhere else I decided to just leave it generic, so that it can just be extracted in that case.
Description
Similar to RN wrapper before this PR the
PlayerView's lifecycle handling is missing and so the player doesn't pause when app goes to the background.This PR is also prework for PiP implementation, as we will need activity reference to implement it.
Changes
Context.requireActivity()to get activity reference from the ContextTests
Checklist (for PR submitters and reviewers)
CHANGELOG.mdentry for new/changed features, bug fixes or important code changes