packages icon indicating copy to clipboard operation
packages copied to clipboard

[video_player] #60048 ios picture in picture

Open vanlooverenkoen opened this issue 2 years ago • 58 comments

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

demo_pip_iphone

List which issues are fixed by this PR. You must list at least one issue.

This feature is only available on iOS because android has a different implementation. simple_pip_mode can be used for Android

Fixes https://github.com/flutter/flutter/issues/60048

Migration from flutter/plugin to flutter/packages is done with this pr

old pr: https://github.com/flutter/plugins/pull/6284

No migration needed this is an addon

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.
  • [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.

vanlooverenkoen avatar Mar 20 '23 20:03 vanlooverenkoen

@hellohuanlin & @stuartmorgan I did the migration from https://github.com/flutter/plugins/pull/6284 to here

vanlooverenkoen avatar Mar 20 '23 20:03 vanlooverenkoen

@tarrinneal can I assist you with the review?

vanlooverenkoen avatar Apr 03 '23 10:04 vanlooverenkoen

@tarrinneal I updated my pull request fully retested it to make sure it does what I expect.

vanlooverenkoen avatar Apr 17 '23 16:04 vanlooverenkoen

@tarrinneal I updated my pull request fully retested it to make sure it does what I expect.

Seems like you still have a few broken tests. I should be able to get this re-reviewed this week, if you can git them sorted out.

tarrinneal avatar May 10 '23 08:05 tarrinneal

@tarrinneal the tests are failing on camera because of the dependency overrides. How should I handle this? Because the dependency override script doesn't update it in the camera package.

I will fix the merge conflicts today

vanlooverenkoen avatar May 10 '23 08:05 vanlooverenkoen

@tarrinneal the tests are failing on camera because of the dependency overrides. How should I handle this? Because the dependency override script doesn't update it in the camera package.

I will fix the merge conflicts today

That's right. Sorry, I got this pr mixed up with another one. I'll take a look!

tarrinneal avatar May 10 '23 08:05 tarrinneal

Thanks for your extended review! I will go through it this weekend! And process all your feedback

vanlooverenkoen avatar Jun 08 '23 07:06 vanlooverenkoen

@stuartmorgan can you give me another review/feedback

vanlooverenkoen avatar Jun 23 '23 17:06 vanlooverenkoen

Android changes look good to me!

Any other issues I should fix? Not sure with android changes you are referring to.

vanlooverenkoen avatar Jul 06 '23 18:07 vanlooverenkoen

Android changes look good to me!

Any other issues I should fix? Not sure with android changes you are referring to.

Not on my part. Just the changes to the example look fine

camsim99 avatar Jul 06 '23 18:07 camsim99

Hey @vanlooverenkoen are you still working on this pr? I think it's about ready to be merged.

tarrinneal avatar Jul 24 '23 17:07 tarrinneal

Yes I am, i will be back from holiday tomorrow, i will do a final cleanup

vanlooverenkoen avatar Jul 24 '23 17:07 vanlooverenkoen

@tarrinneal fixed the pr

vanlooverenkoen avatar Jul 25 '23 07:07 vanlooverenkoen

I think this is ready to be broken up into the federated pr's. Here's a link on how to do that. https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins

tarrinneal avatar Jul 25 '23 10:07 tarrinneal

@stuartmorgan can you do another review?

vanlooverenkoen avatar Jul 31 '23 13:07 vanlooverenkoen

@vanlooverenkoen Hi, are you still planning on updating this?

hellohuanlin avatar Oct 09 '23 19:10 hellohuanlin

Hi, yes of course. I changed jobs and forgot to add my GitHub email to my phone and PC. Give me some time so I can fix this, but yes I will still fix all the comments and discussions

vanlooverenkoen avatar Oct 12 '23 20:10 vanlooverenkoen

Status check from triage: It looks like there were some commits, but a lot of CI is still failing. Is this intended to be reviewed again now, or still waiting for more fixes?

stuartmorgan-g avatar Dec 14 '23 15:12 stuartmorgan-g

I thought I had fixed the latest comments, but again there are merge conflicts. Quite hard to keep track of what still should be fixed.

vanlooverenkoen avatar Dec 14 '23 16:12 vanlooverenkoen

I thought I had fixed the latest comments, but again there are merge conflicts. Quite hard to keep track of what still should be fixed.

Merge conflicts aren't an issue during review; I was referring to the many CI failures in the last run. A few of those may have been unrelated flake, but most in my spot-checks were actual failures in the PR, which need to be resolved.

stuartmorgan-g avatar Dec 14 '23 16:12 stuartmorgan-g

I will double check when all ci/checks are passed/failed again. I indeed saw some linux, windows & macos ci checks fail. But because I did not touch any of these integrations I thought it was something that is broken on main.

vanlooverenkoen avatar Dec 14 '23 16:12 vanlooverenkoen

I indeed saw some linux, windows & macos ci checks fail.

The first word in the name of a CI check is the OS of the host machine running the tests; every check will be one of those three.

But because I did not touch any of these integrations I thought it was something that is broken on main.

Breakage on main is relatively rare; unless you've checked the hash you've merged to and it was red in our postsubmit, it's best to assume it's any failures are caused by the PR unless you've looked at the details and found that they can't be related.

stuartmorgan-g avatar Dec 14 '23 16:12 stuartmorgan-g

Some ci's fail because it seems that the error are related to camera which depends on video_player but video_player are currently stil using overrides? How do I fix that?

vanlooverenkoen avatar Dec 14 '23 16:12 vanlooverenkoen

For this combo PR you can just add dependency overrides to the camera example so that it uses the platform interface from path. (It won't be a problem in the final landed PR because by then the platform interface will be published.)

stuartmorgan-g avatar Dec 14 '23 17:12 stuartmorgan-g

@vanlooverenkoen Are you still planning on updating this PR?

stuartmorgan-g avatar Jan 23 '24 20:01 stuartmorgan-g

I missed your comment. I will check this PR again this weekend.

vanlooverenkoen avatar Jan 23 '24 21:01 vanlooverenkoen

From triage: @vanlooverenkoen Are you still planning on completing this PR?

stuartmorgan-g avatar Mar 05 '24 20:03 stuartmorgan-g

@stuartmorgan yes working on it right now

vanlooverenkoen avatar Mar 07 '24 14:03 vanlooverenkoen

I have taken the week of from work to work on side projects, And this PR is one of them :)

vanlooverenkoen avatar Mar 07 '24 14:03 vanlooverenkoen

@stuartmorgan any idea why this is happening?

 test/test_api.g.dart:368:20: Error: Method not found: 'wrapResponse'.
             return wrapResponse(error: e);

vanlooverenkoen avatar Mar 07 '24 15:03 vanlooverenkoen