Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[video_player] Updates minimum supported SDK #7498

Merged
merged 8 commits into from
Aug 26, 2024

Conversation

abdelaziz-mahdy
Copy link
Contributor

@abdelaziz-mahdy abdelaziz-mahdy commented Aug 26, 2024

fixes flutter/flutter#154050 and flutter/flutter#154054 by setting the min sdk version

Pre-launch Checklist

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

@matanlurey
Copy link
Contributor

Thanks. This won't work as you'd expect due to how versioning is resolved, but now that #7497 is merged (the revert) if you send me a roll-forward with the SDK constraints set properly I'd love to LGTM it 😁

@abdelaziz-mahdy
Copy link
Contributor Author

is this the correct way?

@matanlurey
Copy link
Contributor

is this the correct way?

Looks good except as per the failing check the minimum Dart bounds needs to be bumped as well, then we can merge!

@abdelaziz-mahdy
Copy link
Contributor Author

is this the correct way?

Looks good except as per the failing check the minimum Dart bounds needs to be bumped as well, then we can merge!

done and updated changelog to reflect it too

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2024
Copy link
Contributor

auto-submit bot commented Aug 26, 2024

auto label is removed for flutter/packages/7498, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2024
Copy link
Contributor

auto-submit bot commented Aug 26, 2024

auto label is removed for flutter/packages/7498, due to - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label.

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2024
@auto-submit auto-submit bot merged commit a0501f2 into flutter:main Aug 26, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 26, 2024
@stuartmorgan
Copy link
Contributor

Reviewers: In the future, please include the PR title and description in your review. As someone who uses git log and git blame frequently in this repository, having PRs landed with extremely misleading descriptions makes life much more difficult. The primary effect of this PR is not to change the minimum SDK, it is to change the rendering pipeline.


* Updates minimum supported SDK version to Flutter 3.24/Dart 3.5.

* Re-adds Impeller support.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future please make sure that the list under a given version is one list with one or more entries, not multiple separate lists of one item each, which renders differently on pub.dev.

Copy link
Contributor Author

@abdelaziz-mahdy abdelaziz-mahdy Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I am sorry for that want me to issue a pr fixing that?

If yes, what should be the updated version number

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, CHANGELOG formatting isn't something that we would do another release for unless it were extremely broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thank you for letting me know I will make sure it's correct next time.

@abdelaziz-mahdy
Copy link
Contributor Author

Reviewers: In the future, please include the PR title and description in your review. As someone who uses git log and git blame frequently in this repository, having PRs landed with extremely misleading descriptions makes life much more difficult. The primary effect of this PR is not to change the minimum SDK, it is to change the rendering pipeline.

Well the pr started as updating the minimum sdk, so I am sorry for forgetting to update the title to the new changes

@nate-thegrate
Copy link
Member

@stuartmorgan thanks for letting us know.

In the future, I'll be a bit more judicious about submitting a "rubber-stamp LGTM" 😄

@stuartmorgan
Copy link
Contributor

Well the pr started as updating the minimum sdk

I understand that, but in 6 months when someone is trying to understand the origin of this code by looking at blame or log, what matters is what the PR as landed does. The PR title and description become part of the permanent repository history and are frequently used to understand changes, so should be reviewed just like other documentation in a PR (e.g., code comments).

@abdelaziz-mahdy
Copy link
Contributor Author

Well the pr started as updating the minimum sdk

I understand that, but in 6 months when someone is trying to understand the origin of this code by looking at blame or log, what matters is what the PR as landed does. The PR title and description become part of the permanent repository history and are frequently used to understand changes, so should be reviewed just like other documentation in a PR (e.g., code comments).

I understand now, and will keep it in mind next time I open a pr to make sure it's correct,

Thank you for explaining the problem

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 29, 2024
flutter/packages@d862279...2a0f254

2024-08-28 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds internal wrapper for remaining methods of the Android native `AdsManager`  (flutter/packages#7437)
2024-08-28 brackenavaron@gmail.com [shared_preferences] Fix typo in changelog (flutter/packages#7523)
2024-08-27 matanlurey@users.noreply.github.com Remove matan from codeowners (flutter/packages#7511)
2024-08-27 tarrinneal@gmail.com [shared_preferences] Add test to enforce mutable lists (flutter/packages#7369)
2024-08-27 magder@google.com [google_sign_in_ios] Fix "callee requires a non-null parameter" analyzer warning (flutter/packages#7513)
2024-08-27 louisehsu@google.com [in_app_purchase_storekit] Allows 'localizedDescription' to be nullable to handle occasional nulls in Storekit objects (flutter/packages#7515)
2024-08-27 31859944+LongCatIsLooong@users.noreply.github.com Cupertino icons golden tests (flutter/packages#7421)
2024-08-26 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.10 to 2.0.20 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7500)
2024-08-26 zezohassam@gmail.com [video_player] Updates minimum supported SDK (flutter/packages#7498)
2024-08-26 paulberry@google.com [url_launcher] Ignore new `unreachable_switch_default` warning. (flutter/packages#7487)
2024-08-26 linxunfeng@yeah.net Revert "[video_player] Relands #6456: Uses SurfaceProducer, this time with setCallback for suspend/resume lifecycles" (flutter/packages#7497)
2024-08-24 matanlurey@users.noreply.github.com [video_player] Relands #6456: Uses `SurfaceProducer`, this time with `setCallback` for suspend/resume lifecycles. (flutter/packages#6989)
2024-08-23 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Convert `PlatformTileOverlay` to Pigeon (flutter/packages#7467)
2024-08-23 47866232+chunhtai@users.noreply.github.com [go_router] Fixes issue so that the parseRouteInformationWithContext � (flutter/packages#7337)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
flutter/packages@d862279...2a0f254

2024-08-28 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds internal wrapper for remaining methods of the Android native `AdsManager`  (flutter/packages#7437)
2024-08-28 brackenavaron@gmail.com [shared_preferences] Fix typo in changelog (flutter/packages#7523)
2024-08-27 matanlurey@users.noreply.github.com Remove matan from codeowners (flutter/packages#7511)
2024-08-27 tarrinneal@gmail.com [shared_preferences] Add test to enforce mutable lists (flutter/packages#7369)
2024-08-27 magder@google.com [google_sign_in_ios] Fix "callee requires a non-null parameter" analyzer warning (flutter/packages#7513)
2024-08-27 louisehsu@google.com [in_app_purchase_storekit] Allows 'localizedDescription' to be nullable to handle occasional nulls in Storekit objects (flutter/packages#7515)
2024-08-27 31859944+LongCatIsLooong@users.noreply.github.com Cupertino icons golden tests (flutter/packages#7421)
2024-08-26 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.10 to 2.0.20 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7500)
2024-08-26 zezohassam@gmail.com [video_player] Updates minimum supported SDK (flutter/packages#7498)
2024-08-26 paulberry@google.com [url_launcher] Ignore new `unreachable_switch_default` warning. (flutter/packages#7487)
2024-08-26 linxunfeng@yeah.net Revert "[video_player] Relands flutter#6456: Uses SurfaceProducer, this time with setCallback for suspend/resume lifecycles" (flutter/packages#7497)
2024-08-24 matanlurey@users.noreply.github.com [video_player] Relands flutter#6456: Uses `SurfaceProducer`, this time with `setCallback` for suspend/resume lifecycles. (flutter/packages#6989)
2024-08-23 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Convert `PlatformTileOverlay` to Pigeon (flutter/packages#7467)
2024-08-23 47866232+chunhtai@users.noreply.github.com [go_router] Fixes issue so that the parseRouteInformationWithContext � (flutter/packages#7337)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: video_player platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[video_player] cannot find symbol final class VideoPlayer implements TextureRegistry.SurfaceProducer.Callback
4 participants