[video_player] Add optional web options

[defuncart]

• Fixes https://github.com/flutter/flutter/issues/62192
• Fixes first part of https://github.com/flutter/flutter/issues/88151 (ability to show controls)
• Fixes https://github.com/flutter/flutter/issues/64397

Can be used as follows:

_controller = VideoPlayerController.asset(
  'assets/Butterfly-209.mp4',
  videoPlayerOptions: const VideoPlayerOptions(
    webOptions: VideoPlayerWebOptions(
        controls: VideoPlayerWebOptionsControls.enabled(
          allowDownload: false,
          allowFullscreen: false,
          allowPlaybackRate: false,
          allowPictureInPicture: false,
        ),
        allowContextMenu: false,
        allowRemotePlayback: false,
     ),
  ),
);

Pre-launch Checklist

• [X] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [X] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [X] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [X] I signed the CLA.
• [X] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [X] I listed at least one issue that this PR fixes in the description above.
• [ ] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• [x] I updated/added relevant documentation (doc comments with ///).
• [ ] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard[bot]]

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.

[defuncart]

@ditman @stuartmorgan The original PR was opened 3 months ago, would be nice to get some feedback on the api. I think this is a feature that can benefit flutter web users.

[stuartmorgan]

Bringing over my API note from the other PR for convenience (/cc @ditman)

I think the main question is whether we want to expose things through an options mechanism like this, or do something more direct like exposing the element for direct manipulation. How many more things like this might we want to add later? If we end up duplicating all of a large API surface that would be problematic, but if it's a fairly small set of options this (or the alternate pattern we have in some other plugins of using platform-specific option subclasses) could be reasonable.

[stuartmorgan]

@ditman Do you have an opinion on the API question above?

[stuartmorgan]

Update from triage: waiting for @ditman's input on the high-level API question.

[defuncart]

@ditman @stuartmorgan Thanks for the review.

As more options may come in the future (i.e. mute, autoplay), I'd split the control options out to their own model VideoPlayerWebOptionsControls for a clearer api with named constructors.

I agree that setWebOptions(options) should suffice for web users and it doesn't need to be a property of VideoPlayerOptions. However, the benefit of being part of VideoPlayerOptions is that when initialize is called, the optional web options are initialized. If setWebOptions(options) is a method on VideoPlayerController, then initialize will need to be invoked before setWebOptions.

[stuartmorgan]

@ditman Do you want to weigh in again on the structure we want for exposing <video> attributes?

[defuncart]

@ditman Added allowPictureInPicture and allowRemotePlayback options. As picture-in-picture is displayed in controls list, I've added the option to VideoPlayerWebOptionsControls. If you are happy with the API, then I can add tests etc.

[ditman]

If you are happy with the API, then I can add tests etc.

I'm happy with the API @defuncart, thanks! The only unaddressed concern is using dynamic as a return value of the _onContextMenu function definition, which should be more precise (at the very least it should be: Object?)

[defuncart]

@ditman Update function definition to be correct. Does 'override: no versioning needed' need to be added so that repo_checks can succeed?

[stuartmorgan]

Does 'override: no versioning needed' need to be added so that repo_checks can succeed?

Which of the version change exemptions (see link in PR checklist) do you believe this PR falls under?

[ditman]

@defuncart if we don't add a version to the platform_interface package, it won't get published to pub -> people won't be able to use the new methods that you're defining there. You'll have to split this PR into a couple of PRs so you can land the packages in logical order:

1. platform_interface (defining the new methods and types), then
2. all _platform implementations (defining the per-platform behavior), and finally
3. core plugin package (using/giving access to new features to all programmers)

See "Changing Federated Plugins" on the wiki for more info (we're currently at step 2 of the process)

[stuartmorgan]

It looks like this is ready to split out the next PR, which is the web implementation. You'll just need to add in updating the version constraint for the interface package (and add a version bump).

[stuartmorgan]

Update from triage: the blocking PR is in the queue to land now.

[flutter-dashboard[bot]]

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

[ditman]

Only the changes to the core package are remaining now, right?

[wreppun]

I just wrote up a whole comment on my confusion regarding this feature, and how I wasn't sure if the options were getting picked up and used by the controller... and then I:

1. Realized that I was looking at only a subset of the PR.
2. Found this parent PR.
3. Saw that it hadn't merged yet.

Thanks for the work @defuncart , this will be super helpful!

[stuartmorgan]

@defuncart Are you still planning on updating this to add tests, per the comment above?

[defuncart]

@stuartmorgan @ditman What type of tests are expected here? How can kIsWeb be mocked?

[stuartmorgan]

What type of tests are expected here?

See my earlier comment. To put it another way: a test that would fail if the code you are adding in this PR were accidentally removed or broken by someone later.

How can kIsWeb be mocked?

You shouldn't need to mock it; we run Dart unit tests in both web and vm mode, so a test that is skipped on non-web will still run in CI.

[defuncart]

What type of tests are expected here?

See my earlier comment. To put it another way: a test that would fail if the code you are adding in this PR were accidentally removed or broken by someone later.

Thanks for the info, due to holidays, I won't be able to look into this for the next month. @wreppun if you'd like, feel free to take over adding the 🧪.

[hellohuanlin]

I can only see 1 file change in this PR. Is this ready for review?

[stuartmorgan]

Converting to a Draft; please feel free to mark this as ready for review once the tests have been added.

[Thioby]

any chance to merge this? This literally makes this lib unusable.

[stuartmorgan]

@Thioby We cannot merge an uncompleted draft PR; anyone interested in accelerating the process of getting to a landable PR is welcome to contribute tests as discussed above.

[goderbauer]

(triage): @defuncart Do you still have plans to come back to this PR and address the feedback given above?

[goderbauer]

(triage): I am going to close this one since there hasn't been any follow-up. If you find the time to address the feedback given above please reopen this. Thank you.

[ditman]

Assigning to myself to land this. Needs tests.