[video_player] Fix VideoPlayer controller datasource datatype

[gabrielokura]

trafficstars

This PR updates VideoPlayerController network constructor to fix a bug with the datasource type. The solution follows the comments on the issue #121927.

Fixes #121927

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.
• [x] 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.
• [ ] I updated/added relevant documentation (doc comments with ///).
• [x] 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.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gabrielokura]

Hey @tarrinneal @stuartmorgan

As you can see CI is failing because of an analyzer issue in packages that depends on video_player package.

When I run the local analyzer this is the output:

Running for packages/camera/camera...
Running command: "flutter pub get" in /Users/gabrielokura/Desktop/Projects.nosync/packages/packages/camera/camera
Resolving dependencies...
  matcher 0.12.14 (0.12.15 available)
  material_color_utilities 0.2.0 (0.3.0 available)
  meta 1.9.0 (1.9.1 available)
  vm_service 11.2.1 (11.3.0 available)
Got dependencies!
Resolving dependencies in ./example...
Got dependencies in ./example.
`--[no-]analytics` is deprecated.  Use `--suppress-analytics` to disable analytics for one run instead.
Running command: "dart analyze --fatal-infos" in /Users/gabrielokura/Desktop/Projects.nosync/packages/packages/camera/camera
Analyzing camera...
No issues found!

But the output of CIRRUS error message is:

Warning: You are using these overridden dependencies:
! video_player 2.6.1 from path ../../video_player/video_player
Running command: "dart analyze --fatal-infos" in /tmp/cirrus-ci-build/packages/camera/camera
Analyzing camera...
   info - example/lib/main.dart:1004:11 - 'VideoPlayerController.VideoPlayerController.network' is deprecated and shouldn't be used. Use VideoPlayerController.networkUrl instead. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
1 issue found.

Resuming, Cirrus CI is failing because the dependency video_player is being overwriten in the pipeline. Could you help me with that?

video_player dependency should be using version 2.1.4 as you can check in `packages/camera/camera/example/pubspec.yaml':

dependencies:
  camera:
    # When depending on this package from a real application you should use:
    #   camera: ^x.y.z
    # See https://dart.dev/tools/pub/dependencies#version-constraints
    # The example app is bundled with the plugin so we use a path dependency on
    # the parent directory to use the current plugin's version.
    path: ../
  flutter:
    sdk: flutter
  path_provider: ^2.0.0
  video_player: ^2.1.4

Seems that cirrus ci has a cache and his dependencies are different than the examples, is this correct?

[stuartmorgan-g]

Seems that cirrus ci has a cache and his dependencies are different than the examples, is this correct?

It's not a cache, this is a test that uses in-tree dependencies to pre-detect issues that would otherwise only show up when publishing the package.

This will need to be a two-part PR. You can add an // ignore for the deprecation warning on the failing line in this PR, and then in a follow-up PR once this is published, update the call and remove the ignore.

Unrelated but:

This PR updates VideoPlayerController network constructor to fix a bug with the datasource type.

This is a confusing description for the PR; there's no bug being fixed here. It's just an improvement to the types used in the API.

[stuartmorgan-g]

This appears to be addressing the same issue as https://github.com/flutter/packages/pull/3510; I would encourage the two of you to collaborate on a single PR.

[gabrielokura]

Seems that cirrus ci has a cache and his dependencies are different than the examples, is this correct?

It's not a cache, this is a test that uses in-tree dependencies to pre-detect issues that would otherwise only show up when publishing the package.

This will need to be a two-part PR. You can add an // ignore for the deprecation warning on the failing line in this PR, and then in a follow-up PR once this is published, update the call and remove the ignore.

Unrelated but:

This PR updates VideoPlayerController network constructor to fix a bug with the datasource type.

This is a confusing description for the PR; there's no bug being fixed here. It's just an improvement to the types used in the API.

done :)

[gmackall]

Hey, it looks like all of your tests are passing but it is conflicting with tip of tree, can you please update your PR?

[stuartmorgan-g]

Hey, it looks like all of your tests are passing but it is conflicting with tip of tree, can you please update your PR?

Per the last FAQ here, we don't generally worry about changelog and pubspec version conflicts until everything is fully approved, since new conflicts will show up there regularly.

[stuartmorgan-g]

@gabrielokura Are you still planning on updating this based on the last feedback?

[gabrielokura]

@gabrielokura Are you still planning on updating this based on the last feedback?

Sure. Sorry for the delay, last weeks have been full.

I'll complete the PR in this weekend, using yours feedbacks. Btw, thanks for that :)

[gabrielokura]

The analysis failures in CI due to deprecation will need to be addressed.

Something like this? https://github.com/flutter/flutter/issues/127754

[stuartmorgan-g]

The analysis failures in CI due to deprecation will need to be addressed.

Something like this? flutter/flutter#127754

See the analyze section of https://github.com/flutter/flutter/wiki/Understanding-Packages-tests#specific-tests. The issue sounds like you are assuming the PR would land with analyze broken and then it would be fixed later; we don't land PRs that break our CI.

[gabrielokura]

@gabrielokura Are you still planning on updating this to address the analysis failures?

Yes, will do it until the next week. Sorry for the delay

[gabrielokura]

@stuartmorgan Now CI is failing only in the repo checks after adding the //ignore for the deprecation. Could you override this check?

[stuartmorgan-g]

Overriding version checks: We don't need to publish the examples with the ignores, since they should be updated to use the new method once this is published.

[stuartmorgan-g]

Overriding stale tree status.

[auto-submit[bot]]

auto label is removed for flutter/packages, pr: 3513, Mergeability of pull request flutter/packages/3513 could not be determined at time of merge..

[AlexV525]

AFAICT deprecating .network doesn't help with https://github.com/flutter/flutter/issues/121927 but causes a lot of cost for migrations. The dataSource will eventually be String, so what we can do better is to use Uri.parse(url).toString() for .network rather than inventing a new API to deprecate the widely used one. cc @gabrielokura @stuartmorgan

[stuartmorgan-g]

AFAICT deprecating .network doesn't help with flutter/flutter#121927

That issue is about someone passing an invalid URL to a method that requires a URL, which this transition makes literally impossible. I strongly disagree that making a class of bug impossible "doesn't help".

I also have direct evidence that it does help, because doing the same transition in the url_launcher API surface completely eliminated this entire category of issue report (e.g., issues of the form "url_launcher has a bug, because when I pass <some string that looks kind of like a URL, but isn't actually one> it doesn't work on iOS"), which used to be quite common.

but causes a lot of cost for migrations.

If someone is currently passing valid, constant URLs, or if the behavior they want is just hoping that parse can handle the arbitrary input and throwing if it can't, then the cost of migration should be trivial. If they are currently passing arbitrary untrusted content and just hoping for the best, and as a result of this change realize that they are actually responsible for deciding how to go from arbitrary input to a valid URL to meet the requirements of the API surface—requirements that have always been there, but were previously only enforced by comment— and do so in a better way that just throwing, then it might be costly, but that's the change working as intended.

The dataSource will eventually be String

This is an internal implementation detail, which is irrelevant to designing a good API.

so what we can do better is to use Uri.parse(url).toString() for .network

It is true that we could continue to enforce API requirements via comment rather than via the type system, deliberately allowing people to pass invalid data and then throwing an exception when they do, but I disagree that doing so is better. We could also have made the NNBD transition easier for clients by just marking every single parameter's type as nullable and continuing to assert when someone passed a null value, but we didn't, for exactly the same reason.