-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[video_player] synchronize isPlaying state #3261
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
Conversation
Feel free to request review again when my comments in the original PR is addressed. |
I've addressed your comments but removing those two
|
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.
iOS part looks good
packages/video_player/video_player_android/example/pubspec.yaml
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/lib/mini_controller.dart
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/pubspec.yaml
Outdated
Show resolved
Hide resolved
My understanding is that what you changed is the standard approach in 1p plugins. IIRC I have seen similar practice in other plugins. Without that overrides, the dependency would be the previously published version |
I stamped the iOS part. you probably wanna add back the reviewers for android/web. |
How can add back the reviewers? |
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 web bits look good to me, this is a big update for the player, thanks for doing it!
packages/video_player/video_player_web/example/integration_test/video_player_web_test.dart
Show resolved
Hide resolved
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 Android parts look good to me!
This PR made me realize the lack of test coverage on the Android native parts, so you could add a test for this change, but definitely not blocking for this PR. More-so a note to self :)
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.
LGTM
It looks like this also already has approval for, iOS, web, and Android. So, I'll take a look at #3361.
This will help https://www.youtube.com/watch?v=IMQdSTlTXjA |
In my opinion this PR is ready
|
Is this PR missing anything in order to be able to merge? |
Seems ready to me. |
[video_player] synchronize isPlaying state
This was the original PR: flutter/plugins#7198
Resolves flutter/flutter#49081.
Resolves flutter/flutter#120872.
This PR synchronizes
isPlaying
state between native and dart code, for example if a headset is unplugged or a phone call interrupts the video.Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.