Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] bugfix caption still showing when text is empty #3374

Merged
merged 23 commits into from
Sep 16, 2021
Merged

[video_player] bugfix caption still showing when text is empty #3374

merged 23 commits into from
Sep 16, 2021

Conversation

vanlooverenkoen
Copy link
Contributor

@vanlooverenkoen vanlooverenkoen commented Dec 27, 2020 •
edited
Loading

Description

Caption widget will still show the dark background if the text is empty. If null it would already be hidden.

Related Issues

#Fixed flutter/flutter#72999

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@google-cla google-cla bot added the cla: yes label Dec 27, 2020
@vanlooverenkoen vanlooverenkoen mentioned this pull request Dec 28, 2020
Closed
Salakar
Salakar approved these changes Jan 8, 2021
View reviewed changes
Copy link
Member

@Salakar Salakar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks again @vanlooverenkoen - LGTM

CI failure appears to be unrelated - failed to clone flutter git repository.

cc @cyanglaz - this one is good to go

russellwheatley
russellwheatley approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

LGTM

stuartmorgan
stuartmorgan suggested changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

This looks good, but needs a test. Could you add a widget test covering this case?

@vanlooverenkoen
Merge pull request #5 from flutter/master
246a042 
Master merge
@vanlooverenkoen
Merge branch 'master' into bugfix/#72999-caption-still-showing
a9d6fc4 
# Conflicts:
#	packages/video_player/video_player/CHANGELOG.md
#	packages/video_player/video_player/pubspec.yaml
@vanlooverenkoen
Merge pull request #6 from flutter/master
7570933 
Master
@vanlooverenkoen
Merge branch 'master' into bugfix/#72999-caption-still-showing
bbdc044 
# Conflicts:
#	packages/video_player/video_player/CHANGELOG.md
#	packages/video_player/video_player/pubspec.yaml
@vanlooverenkoen
Added an extra test for empty caption text
60ca485
@vanlooverenkoen
Copy link
Contributor Author

build_all_plugins_ipa timedout. How can I fix that?

@vanlooverenkoen
Copy link
Contributor Author

@stuartmorgan the tests are added for this regression issue.

stuartmorgan
stuartmorgan approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Sorry for the delay getting back to this review.

packages/video_player/video_player/lib/video_player.dart
@@ -914,11 +914,12 @@ class ClosedCaption extends StatelessWidget {
/// Creates a a new closed caption, designed to be used with
/// [VideoPlayerValue.caption].
///
/// If [text] is null, nothing will be displayed.
/// If [text] is null or empty, nothing will be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally text should be non-nullable with a default to '' so that we don't have two ways of expressing the same thing, but unfortunately that would be a breaking change at this point.

@stuartmorgan
Copy link
Contributor

I've resolved the version bump conflicts; I expect web tests to fail here due to an infra issue we're having at the moment, in which case merging in master again tomorrow should fix it.

@stuartmorgan stuartmorgan added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 15, 2021
@fluttergithubbot fluttergithubbot merged commit 5d64092 into flutter:master Sep 16, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 18, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2021
@engine-flutter-autoroll
5d64092 [video_player] bugfix caption still showing when text is empty (
0df2799 
flutter/plugins#3374)
@vanlooverenkoen vanlooverenkoen deleted the bugfix/#72999-caption-still-showing branch September 21, 2021 07:51
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
@vanlooverenkoen @amantoux
[video_player] bugfix caption still showing when text is empty (flutt…
cfe19ba 
…er#3374)
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
@vanlooverenkoen @KyleFin
[video_player] bugfix caption still showing when text is empty (flutt…
88ff4b1 
…er#3374)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rrousselGit rrousselGit rrousselGit approved these changes

@russellwheatley russellwheatley russellwheatley approved these changes

@Salakar Salakar Salakar approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

Assignees

@cyanglaz cyanglaz

Labels
cla: yes p: video_player waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[video_player] Caption widget is still showing even if the caption is empty.
8 participants
@vanlooverenkoen @stuartmorgan @Salakar @russellwheatley @rrousselGit @ditman @cyanglaz @fluttergithubbot