plugins icon indicating copy to clipboard operation
plugins copied to clipboard

[video_player] Passing http headers to file constructor

Open abdelaziz-mahdy opened this issue 2 years ago • 9 comments

when passing m3u8 file to videoplayer as videoplayercontroller.file links insided m3u8 file has to be used with http headers

so i passed it in the videoplayercontroller.file and changed all things related to it

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/plugins repo does use dart format.)
  • [ x] I signed the CLA.
  • [ x] The title of the PR starts with the name of the plugin 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.
  • [x ] 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.

abdelaziz-mahdy avatar Aug 06 '22 11:08 abdelaziz-mahdy

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.

flutter-dashboard[bot] avatar Aug 06 '22 11:08 flutter-dashboard[bot]

now all of the code will allow http headers to passed since it will use one DataSource

i dont know if it will cause problems since i dont have any tests to try it on

abdelaziz-mahdy avatar Aug 06 '22 13:08 abdelaziz-mahdy

Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the checklist reflects the state of the PR as posted please feel free to mark it as ready for review.

stuartmorgan avatar Aug 16 '22 15:08 stuartmorgan

Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the checklist reflects the state of the PR as posted please feel free to mark it as ready for review.

Which one is missing since I can't find it 😅

abdelaziz-mahdy avatar Aug 16 '22 17:08 abdelaziz-mahdy

You haven't added any tests, or run the auto-formatter.

stuartmorgan avatar Aug 16 '22 18:08 stuartmorgan

You haven't added any tests, or run the auto-formatter.

Tests can not be added since I don't know how to test these changes using testing code

Auto formatter I will do it for both java and dart code

abdelaziz-mahdy avatar Aug 16 '22 18:08 abdelaziz-mahdy

Have you read the documentation about different types of plugin tests linked from our contribution guide, and looked at other tests in the plugin?

If you have specific questions about tests we can try to answer them, but we can't accept PRs with no tests.

stuartmorgan avatar Aug 16 '22 19:08 stuartmorgan

Have you read the documentation about different types of plugin tests linked from our contribution guide, and looked at other tests in the plugin?

If you have specific questions about tests we can try to answer them, but we can't accept PRs with no tests.

i am sorry i didnt dive deep into testing guide

i will check it and implement the needed tests

thank you.

abdelaziz-mahdy avatar Aug 16 '22 21:08 abdelaziz-mahdy

I added the tests and made the pull request ready for review for a while now

Should I do something more or just wait?

abdelaziz-mahdy avatar Sep 17 '22 21:09 abdelaziz-mahdy

@zezo357 It seems like you still need to add a test. Let us know when you get around to that and we can move this PR along. Thanks!

GaryQian avatar Nov 10 '22 20:11 GaryQian

@zezo357 It seems like you still need to add a test. Let us know when you get around to that and we can move this PR along. Thanks!

I failed to add the test, since I don't have much knowledge about it

If anyone can help in that matter it will be much appreciated

abdelaziz-mahdy avatar Nov 10 '22 20:11 abdelaziz-mahdy

@zezo357 I can work on adding tests to this to help land the change!

camsim99 avatar Dec 07 '22 22:12 camsim99

@zezo357 I can work on adding tests to this to help land the change!

Do I need to give you permission? Or you can make a pull request on my fork?

Let me know if I need to do something to help you

abdelaziz-mahdy avatar Dec 08 '22 08:12 abdelaziz-mahdy

I was wondering if this problem has been solved yet

ShareZore avatar Dec 14 '22 09:12 ShareZore

I was wondering if this problem has been solved yet

The java side of tests are still not implemented, I failed to do it sadly and @camsim99 offered to do it but no progress yet I guess

abdelaziz-mahdy avatar Dec 14 '22 10:12 abdelaziz-mahdy

我想知道这个问题是否已经解决了

测试的java端仍然没有现实,我很遗弃不能做到并且@camsim99想去做,但我想还没有进展

private fun HlsMediaSource(dataSource: String?): HlsMediaSource { val contentUri = Uri.parse(dataSource) val mediaItem = MediaItem.fromUri(contentUri)

val defaultFactory = DefaultHttpDataSource
    .Factory()
    .setUserAgent(null)
    .setTransferListener(null)
    .setConnectTimeoutMs(30 * 1000)
    .setReadTimeoutMs(30 * 1000)
    .setAllowCrossProtocolRedirects(true)

return HlsMediaSource
    .Factory(defaultFactory)
    .setAllowChunklessPreparation(true)
    .createMediaSource(mediaItem)

}

ShareZore avatar Dec 14 '22 10:12 ShareZore

Android native can do that, but with flutter I don't know how to write it, right

ShareZore avatar Dec 14 '22 10:12 ShareZore

Triage checkin: what's the state of this PR? Is it just waiting for tests?

stuartmorgan avatar Jan 17 '23 19:01 stuartmorgan

Triage checkin: what's the state of this PR? Is it just waiting for tests?

Waiting on the java native tests since I couldn't implement those, but the dart tests I already implemented and they are passing

abdelaziz-mahdy avatar Jan 17 '23 19:01 abdelaziz-mahdy

@zezo357 Sorry for the delay on this. Just added tests and made some minor changes. Let me know what you think!

camsim99 avatar Jan 25 '23 22:01 camsim99

Will check it thoroughly tomorrow and test it

But for now I see it looks good

abdelaziz-mahdy avatar Jan 25 '23 22:01 abdelaziz-mahdy

looks good to me.

abdelaziz-mahdy avatar Jan 27 '23 13:01 abdelaziz-mahdy

@zezo357 Can you fix the failing Dart test? Then we'll need a second review.

camsim99 avatar Jan 27 '23 17:01 camsim99

@zezo357 Can you fix the failing Dart test? Then we'll need a second review.

will check them

abdelaziz-mahdy avatar Jan 27 '23 17:01 abdelaziz-mahdy

I fixed the dart test related to my pull request there is another one that fails file with special characters with error

Invalid argument(s): Illegal character in path
dart:core                                      new _Uri.file
package:video_player/video_player.dart 254:26  new VideoPlayerController.file
test\video_player_test.dart 352:35             main.<fn>.<fn>.<fn>

abdelaziz-mahdy avatar Jan 27 '23 18:01 abdelaziz-mahdy

@zezo357 I don't see that failure, but there is a formatting error, but you can run the command suggested to fix it https://github.com/flutter/plugins/pull/6213/checks?check_run_id=10935748930

camsim99 avatar Jan 27 '23 18:01 camsim99

@zezo357 I don't see that failure, but there is a formatting error, but you can run the command suggested to fix it https://github.com/flutter/plugins/pull/6213/checks?check_run_id=10935748930

fixed formatting

abdelaziz-mahdy avatar Jan 27 '23 18:01 abdelaziz-mahdy

  • [x] I listed at least one issue that this PR fixes in the description above.

Please add the issue link to the PR description.

Is this problem actually Android-specific, or does it apply to iOS as well? It's fine to fix it only for Android for now if you don't have iOS experience, but if it's broken there as well we should update the relevant issue accordingly.

i didnt test on ios, but the issue was for me on android. and i did describe the issue on the pull request should i open an issue?

abdelaziz-mahdy avatar Jan 27 '23 19:01 abdelaziz-mahdy

Yes, that checklist item is about having an issue filed clearly describing the problem, including reproduction steps, so there's a place to reference details about the issue being fixed.

stuartmorgan avatar Jan 27 '23 20:01 stuartmorgan

Yes, that checklist item is about having an issue filed clearly describing the problem, including reproduction steps, so there's a place to reference details about the issue being fixed.

ok i will open one and try to make the steps to reproduce it.

abdelaziz-mahdy avatar Jan 27 '23 20:01 abdelaziz-mahdy