-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add missing PlayerView lifecycle handling (PiP #1) #85
Changes from 5 commits
19a98d9
0b037ec
57f5a2b
3febaf0
4cf69bc
bd37436
d315606
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ package com.bitmovin.player.flutter | |
|
||
import android.content.Context | ||
import android.view.View | ||
import androidx.lifecycle.DefaultLifecycleObserver | ||
import androidx.lifecycle.LifecycleOwner | ||
import com.bitmovin.player.PlayerView | ||
import com.bitmovin.player.api.Player | ||
import com.bitmovin.player.flutter.json.JPlayerViewCreateArgs | ||
import io.flutter.embedding.android.FlutterActivity | ||
import io.flutter.embedding.android.FlutterFragmentActivity | ||
import io.flutter.plugin.common.BinaryMessenger | ||
import io.flutter.plugin.common.EventChannel | ||
import io.flutter.plugin.common.MethodCall | ||
|
@@ -22,7 +25,6 @@ class FlutterPlayerView( | |
id: Int, | ||
args: Any?, | ||
) : MethodChannel.MethodCallHandler, EventChannel.StreamHandler, PlatformView, EventListener() { | ||
private val playerView: PlayerView | ||
private val methodChannel: MethodChannel = | ||
MethodChannel( | ||
messenger, | ||
|
@@ -35,10 +37,33 @@ class FlutterPlayerView( | |
messenger, | ||
) | ||
|
||
private val activity = context.requireActivity() | ||
private val playerView: PlayerView = PlayerView(context, player = null) | ||
|
||
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", | ||
) | ||
|
||
private val activityLifecycleObserver = | ||
object : DefaultLifecycleObserver { | ||
override fun onStart(owner: LifecycleOwner) = playerView.onStart() | ||
|
||
override fun onResume(owner: LifecycleOwner) = playerView.onResume() | ||
|
||
override fun onPause(owner: LifecycleOwner) = playerView.onPause() | ||
|
||
override fun onStop(owner: LifecycleOwner) = playerView.onStop() | ||
|
||
override fun onDestroy(owner: LifecycleOwner) = dispose() | ||
} | ||
|
||
init { | ||
val playerViewCreateArgs = JPlayerViewCreateArgs(args as Map<*, *>) | ||
|
||
playerView = PlayerView(context, null as Player?) | ||
PlayerManager.onPlayerCreated(playerViewCreateArgs.playerId) { player -> | ||
playerView.player = player | ||
if (playerViewCreateArgs.hasFullscreenHandler) { | ||
|
@@ -50,6 +75,8 @@ class FlutterPlayerView( | |
) | ||
} | ||
} | ||
|
||
activityLifecycle.addObserver(activityLifecycleObserver) | ||
} | ||
|
||
override fun onMethodCall( | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I like this style more 👍 |
||
else -> throw NotImplementedError() | ||
} | ||
|
||
|
@@ -69,6 +96,7 @@ class FlutterPlayerView( | |
playerView.onDestroy() | ||
methodChannel.setMethodCallHandler(null) | ||
eventChannel.setStreamHandler(null) | ||
activityLifecycle.removeObserver(activityLifecycleObserver) | ||
} | ||
|
||
override fun onListen( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
package com.bitmovin.player.flutter | ||
|
||
import android.app.Activity | ||
import android.content.Context | ||
import android.content.ContextWrapper | ||
import android.os.Handler | ||
import android.os.Looper | ||
|
||
|
@@ -25,3 +28,10 @@ inline fun postToMainThread(crossinline block: () -> Unit?) { | |
block() | ||
} | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
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.
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 useinstead to be more resilient.
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 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.bd37436