Skip to content
This repository was 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
fluttergithubbot merged 23 commits into
flutter-team-archive:masterfrom
vanlooverenkoen:bugfix/#72999-caption-still-showing
Sep 16, 2021
Merged

[video_player] bugfix caption still showing when text is empty#3374
fluttergithubbot merged 23 commits into
flutter-team-archive:masterfrom
vanlooverenkoen:bugfix/#72999-caption-still-showing

Conversation

@vanlooverenkoen

@vanlooverenkoen vanlooverenkoen commented Dec 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 reviewed Jan 8, 2021
View reviewed changes
Comment thread packages/video_player/video_player/lib/video_player.dart Outdated
Salakar
Salakar reviewed Jan 8, 2021
View reviewed changes
Comment thread packages/video_player/video_player/lib/video_player.dart Outdated
rrousselGit
rrousselGit reviewed Jan 8, 2021
View reviewed changes
Comment thread packages/video_player/video_player/lib/video_player.dart Outdated
Salakar
Salakar reviewed Jan 8, 2021
View reviewed changes
Comment thread packages/video_player/video_player/lib/video_player.dart Outdated
rrousselGit
rrousselGit reviewed Jan 8, 2021
View reviewed changes
Comment thread packages/video_player/video_player/lib/video_player.dart Outdated
Salakar
Salakar reviewed Jan 8, 2021
View reviewed changes
Comment thread packages/video_player/video_player/lib/video_player.dart Outdated
Salakar
Salakar approved these changes Jan 8, 2021
View reviewed changes

@Salakar Salakar left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@russellwheatley russellwheatley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

stuartmorgan-g
stuartmorgan-g suggested changes Feb 9, 2021
View reviewed changes

@stuartmorgan-g stuartmorgan-g left a comment

Copy link
Copy Markdown
Contributor

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

vanlooverenkoen commented Jul 24, 2021

Copy link
Copy Markdown
Contributor Author

build_all_plugins_ipa timedout. How can I fix that?

@vanlooverenkoen

vanlooverenkoen commented Jul 24, 2021

Copy link
Copy Markdown
Contributor Author

@stuartmorgan the tests are added for this regression issue.

stuartmorgan-g
stuartmorgan-g approved these changes Sep 14, 2021
View reviewed changes

@stuartmorgan-g stuartmorgan-g left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread packages/video_player/video_player/lib/video_player.dart
/// [VideoPlayerValue.caption].
///
/// If [text] is null, nothing will be displayed.
/// If [text] is null or empty, nothing will be displayed.

@stuartmorgan-g stuartmorgan-g Sep 14, 2021

Copy link
Copy Markdown
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-g

stuartmorgan-g commented Sep 14, 2021

Copy link
Copy Markdown
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-g stuartmorgan-g 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-team-archive: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-team-archive/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-team-archive#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-team-archive#3374)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cyanglaz cyanglaz Awaiting requested review from cyanglaz
@iskakaushik iskakaushik Awaiting requested review from iskakaushik
4 more reviewers
@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes
@Salakar Salakar Salakar approved these changes
@russellwheatley russellwheatley russellwheatley approved these changes
@rrousselGit rrousselGit rrousselGit approved these changes
Reviewers whose approvals may not affect merge requirements

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-g @Salakar @russellwheatley @rrousselGit @ditman @cyanglaz @fluttergithubbot