[video_player] Added fullscreen functionality (for web). #3577
Conversation
rsegecin
requested review from
hellohuanlin,
camsim99,
ditman and
tarrinneal
as code owners
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
rsegecin
changed the title
packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart
Show resolved
Hide resolved
rsegecin
changed the title
| await tester.pumpAndSettle(_playDuration); | ||
|
|
||
| expect(controller.value.isFullScreen, true); | ||
| expect(controller.value.position, |
There was a problem hiding this comment.
How is this expectation related to the functionality you are testing?
| await tester.pumpAndSettle(_playDuration); | ||
|
|
||
| expect(controller.value.isFullScreen, false); | ||
| expect(controller.value.position, |
There was a problem hiding this comment.
Same question here.
|
|
||
| await controller.play(); | ||
| await controller.toggleFullScreen(); | ||
| await tester.pumpAndSettle(_playDuration); |
There was a problem hiding this comment.
Why do you have to play a second of video for full screen changes to take effect?
| expect(controller.value.position, | ||
| (Duration position) => position > Duration.zero); | ||
| }, | ||
| skip: !kIsWeb, |
There was a problem hiding this comment.
Rather than making this platform-gated, there should be a supportsFullScreen API at both the app-facing and platform interface levels so that it's runtime-checkable.
| Future<void> toggleFullScreen() async { | ||
| if (kIsWeb) { | ||
| if (!value.isFullScreen) { | ||
| await _applyFullScreen(); |
There was a problem hiding this comment.
Since these are only used in one place, they don't need to be separated out into helpers.
| value = value.copyWith(isFullScreen: true); | ||
| } else { | ||
| await _applyExitFullScreen(); | ||
| value = value.copyWith(isFullScreen: false); |
There was a problem hiding this comment.
Can't the user exit full screen directly? It seems like we would need a state listener for this, rather than managing the state from Dart.
| @@ -125,6 +125,16 @@ void main() { | |||
| expect(VideoPlayerPlatform.instance.pause(await textureId), completes); | |||
| }); | |||
|
|
|||
| testWidgets('can enter fullscreen', (WidgetTester tester) async { | |||
| expect(VideoPlayerPlatform.instance.enterFullScreen(await textureId), | |||
| completes); | |||
There was a problem hiding this comment.
This is only testing that the future completes, not that it does anything. Wouldn't at empty implementation of enterFullScreen pass this test?
|
@rsegecin Are you still planning on updating this PR based on the feedback above? |
Yes, I do want to continue but I think it'll take some time for me to understand, learn and make the appropriate corrections you mentioned. I've been busy with other projects and I don't know when I'll work in this PR so if you rather close it I'll understand may be we can reopen in the future right? Btw thank you very much for your review @stuartmorgan. |
|
I'll mark it as a draft for now so that it doesn't show up in our regular PR triage; please feel free to mark it as ready for review again once you've updated it. |
|
This PR is adding support to enable the native full-screen support on web, without needing to add a controller-owned method: (The difference that I see is that apps cannot know/trigger the full-screen mode, it needs to be user-triggered from the native UI) @rsegecin what do you think about 3259, would it fulfill your needs? |
Yes, definitely, thank you for notifying me @ditman. Kudos to @defuncart coming up with this nice way to set web options for video_player. |
Adds a feature on the video_player package to enable a video to go fullscreen on web platform.
flutter/flutter#64397
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto 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.