[video_player] Add setClosedCaptionFile method to VideoPlayerController by ibrahimcetin · Pull Request #5105 · flutter/plugins

ibrahimcetin · 2022-03-24T19:42:29Z

This PR adds setClosedCaptionFile method to VideoPlayerController to add, change or remove closedCaptionFile.

Fixes flutter/flutter#100302

Pre-launch Checklist

stuartmorgan-g

Thanks for the submission!

packages/video_player/video_player/CHANGELOG.md

packages/video_player/video_player/test/video_player_test.dart

packages/video_player/video_player/lib/video_player.dart

stuartmorgan-g

Looks good other than the version issue.

@bparrishMines Could you do the secondary review here?

stuartmorgan-g · 2022-04-12T19:00:02Z

packages/video_player/video_player/CHANGELOG.md

@@ -1,6 +1,7 @@
-## NEXT
+## 2.3.1


Sorry, I missed this before: this version change should be 2.4.0 rather than 2.3.1, since this is a new feature rather than a bugfix.

Ok, I updated

stuartmorgan-g · 2022-04-12T20:40:53Z

packages/video_player/video_player/pubspec.yaml

 repository: https://github.com/flutter/plugins/tree/main/packages/video_player/video_player
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
-version: 2.3.0
+version: 2.3.1


This needs to match.

stuartmorgan-g · 2022-04-12T20:41:10Z

packages/video_player/video_player/CHANGELOG.md

+## 2.4.0

 * Updates minimum Flutter version to 2.10.
+* Adds `setClosedCaptionFile` method to `VideoPlayerController`


Nit: missing period at the end (missed this in the previous review).

This hasn't been resolved.

Still not resolved.

bparrishMines · 2022-04-13T17:45:45Z

packages/video_player/video_player/lib/video_player.dart

+  ///
+  /// If [closedCaptionFile] is null, closed caption file will be removed.
+  Future<void> setClosedCaptionFile(
+    Future<ClosedCaptionFile>? closedCaptionFile,


I think this would be better as FutureOr<ClosedCaptionFile?> closedCaptionFile so a user wouldn't have to use Future.value() to pass a value synchronously.

Or better yet, I think just making it synchronous would be best if possible: ClosedCaptionFile? closedCaptionFile.

Actually, I made it synchronous but then I turned it to async because closedCaptionFile parameter in initialize method is async. I don't know why closedCaptionFile parameter is Future but I thought if they made it Future, there was a logic behind it. However, my main thought is to make it synchronous as I mentioned in the issue (In short, removing closedCaptionFile parameter and adding a sync setter and getter for closedCaptionFile). But this would be a breaking change, so I decided to add just a setter method. I discussed this with stuartmorgan but I want to ask that question to you too because I'm wondering your thoughts, Can we make this breaking change? I think synchronous would be better in general.

I missed in my review that you didn't change final Future<ClosedCaptionFile>? closedCaptionFile; to a getter, which was part of what we discussed. This change will leave the public accessor returning the wrong file.

(That should also be added to the tests, since it was missed.)

Can we make this breaking change

We are not making a breaking change in video_player for this; that's a disruptive change, and this is a niche use case that can easily be accommodated by a non-breaking change.

@bparrishMines I forgot the previous discussion was in Discord rather than here, so you wouldn't have context, sorry about that. I suggested the Future<> version for consistency; I could see an argument for making it take a file directly and then convert to a Future internally for compatibility with the existing public getter API; I don't have a strong opinion on that.

Ah, I hadn't realized this was already discussed. No worries. This solution sounds good to me! (Assuming @stuartmorgan will wait for the accessor change.)

A getter does not allow assigning a value (nor is the final keyword applicable to it)

Yes, I know.

To be honest, I don't know how to change final Future<ClosedCaptionFile>? closedCaptionFile; to a getter. I turned it to Future<ClosedCaptionFile>? closedCaptionFile; but it became assignable. That's what i'm trying to say. So, how can I change it to a getter?

I believe @stuartmorgan was thinking something like:

VideoPlayerController.asset(/*other params*/, {Future<closedCaptionFile>? closedCaptionFile, /*otherparams*/}) : _closedCaptionFile = closedCaptionFile; Future<ClosedCaptionFile>? _closedCaptionFile; Future<ClosedCaptionFile>? get closedCaptionFile { return _closedCaptureFile; }

Exactly, with the caveat that the member variable will need to be called something like _closedCaptionFileFuture since there's already a _closedCaptionFile.

To be honest, I don't know how to change final Future<ClosedCaptionFile>? closedCaptionFile; to a getter. I turned it to Future<ClosedCaptionFile>? closedCaptionFile; but it became assignable. That's what i'm trying to say.

In the future, if you aren't sure how to implement a suggestion it is much more effective to simply ask, rather than doing something completely different without explaining what you did and then ask questions about that instead. I had absolutely no context for the question you were asking since I didn't know what the code you'd actually written was (or that it was different from my suggestion).

Exactly, with the caveat that the member variable will need to be called something like _closedCaptionFileFuture since there's already a _closedCaptionFile.

Thanks for the answer. I implemented this and everything works fine except the document. Now, the closedCaptionFile parameter shows the constructors' document. (I'm not talking about the getter, its document is ok)

In the future, if you aren't sure how to implement a suggestion it is much more effective to simply ask, rather than doing something completely different without explaining what you did and then ask questions about that instead. I had absolutely no context for the question you were asking since I didn't know what the code you'd actually written was (or that it was different from my suggestion).

Thanks for your feedback. I'll do.

I committed the changes

stuartmorgan-g · 2022-04-13T19:23:07Z

packages/video_player/video_player/lib/video_player.dart

+  ///
+  /// If [closedCaptionFile] is null, closed caption file will be removed.
+  Future<void> setClosedCaptionFile(
+    Future<ClosedCaptionFile>? closedCaptionFile,


I missed in my review that you didn't change final Future<ClosedCaptionFile>? closedCaptionFile; to a getter, which was part of what we discussed. This change will leave the public accessor returning the wrong file.

(That should also be added to the tests, since it was missed.)

Can we make this breaking change

We are not making a breaking change in video_player for this; that's a disruptive change, and this is a niche use case that can easily be accommodated by a non-breaking change.

@bparrishMines I forgot the previous discussion was in Discord rather than here, so you wouldn't have context, sorry about that. I suggested the Future<> version for consistency; I could see an argument for making it take a file directly and then convert to a Future internally for compatibility with the existing public getter API; I don't have a strong opinion on that.

bparrishMines

LGTM

stuartmorgan-g · 2022-04-15T16:14:56Z

packages/video_player/video_player/CHANGELOG.md

+## 2.4.0

 * Updates minimum Flutter version to 2.10.
+* Adds `setClosedCaptionFile` method to `VideoPlayerController`


This hasn't been resolved.

stuartmorgan-g · 2022-04-15T16:16:39Z

packages/video_player/video_player/lib/video_player.dart

  /// This future will be awaited and the file will be loaded when
  /// [initialize()] is called.
-  final Future<ClosedCaptionFile>? closedCaptionFile;
+  Future<ClosedCaptionFile>? get closedCaptionFile {


Please put this after the field declarations, not in the middle of them. Right before setClosedCaptionFile would be the best place.

stuartmorgan-g · 2022-04-15T16:20:37Z

packages/video_player/video_player/lib/video_player.dart

-      _closedCaptionFile ??= await closedCaptionFile;
-      value = value.copyWith(caption: _getCaptionAt(value.position));
+    if (_closedCaptionFileFuture != null) {
+      await setClosedCaptionFile(_closedCaptionFileFuture);


Calling the setter to set the same value that the field is already set to in order to benefit from side-effects in the setter is an anti-pattern; it makes the code less understandable, and is subject to subtle breakage if someone later optimizes the setter to be a no-op if the new and old values are the same.

The way to share code here would be to make a new _updateClosedCaptionWithFuture(...) method and put the logic there, then implement both initialize and setClosedCaptionFile using that.

I guess everything is okay now.

stuartmorgan-g · 2022-04-18T19:31:15Z

packages/video_player/video_player/CHANGELOG.md

+## 2.4.0

 * Updates minimum Flutter version to 2.10.
+* Adds `setClosedCaptionFile` method to `VideoPlayerController`


Still not resolved.

stuartmorgan-g · 2022-04-18T19:33:34Z

packages/video_player/video_player/lib/video_player.dart

    return Caption.none;
  }

+  /// Optional field to specify a file containing the closed


This sentence is no longer correct, since it's not a field now.

/// Returns the file containing closed captions for the video, if any.

stuartmorgan-g · 2022-04-18T19:33:56Z

packages/video_player/video_player/lib/video_player.dart

+
+  /// Sets a closed caption file.
+  ///
+  /// If [closedCaptionFile] is null, closed caption file will be removed.


s/closed caption file/closed captions/

I didn't understand what you mean

Sorry, I shouldn't use that shorthand in reviews.

Please replace "closed caption file" with "closed captions" in this comment.

Just the second.

ibrahimcetin · 2022-04-19T15:44:54Z

Hope everything is fine now. And by the way, I want to thank you for your help @stuartmorgan.

stuartmorgan-g · 2022-04-19T15:49:04Z

The CHANGELOG issue that I've commented on three times is still unaddressed. We do require that PRs follow our style guides (which is why it's part of the checklist).

ibrahimcetin · 2022-04-19T15:56:15Z

Sorry, my bad

stuartmorgan-g

LGTM, thanks!

…erController (flutter/plugins#5105)

…er (flutter#5105)

ibrahimcetin added 3 commits March 24, 2022 22:21

Add setClosedCaptionFile method

e27d680

Update version

99066da

Update Changelog

aaef516

github-actions bot added the p: video_player label Mar 24, 2022

stuartmorgan-g suggested changes Mar 24, 2022

View reviewed changes

packages/video_player/video_player/CHANGELOG.md Outdated Show resolved Hide resolved

packages/video_player/video_player/test/video_player_test.dart Show resolved Hide resolved

packages/video_player/video_player/lib/video_player.dart Show resolved Hide resolved

ibrahimcetin added 2 commits March 24, 2022 23:53

Describe null in comments

60ab81a

Update Changelog

f067739

ibrahimcetin requested a review from stuartmorgan-g March 24, 2022 21:37

stuartmorgan-g suggested changes Apr 12, 2022

View reviewed changes

stuartmorgan-g requested a review from bparrishMines April 12, 2022 19:00

Update Changelog

ccd534a

stuartmorgan-g suggested changes Apr 12, 2022

View reviewed changes

Update pubspec version

acb5337

bparrishMines suggested changes Apr 13, 2022

View reviewed changes

stuartmorgan-g suggested changes Apr 13, 2022

View reviewed changes

bparrishMines approved these changes Apr 13, 2022

View reviewed changes

Change closedCaptionFile to a getter

da436c3

ibrahimcetin requested a review from stuartmorgan-g April 14, 2022 19:01

stuartmorgan-g suggested changes Apr 15, 2022

View reviewed changes

ibrahimcetin added 4 commits April 15, 2022 20:37

Some Improvements

fb2d11c

Update changelog

1292af3

Update

7cf0353

Small change

d5d9353

stuartmorgan-g suggested changes Apr 18, 2022

View reviewed changes

ibrahimcetin added 3 commits April 18, 2022 23:41

Merge branch 'main' into set-closed-caption-file

0dd87a5

Fix document

d306f3e

Update document

2ab0b4d

Add period at the end

3848181

ibrahimcetin requested a review from stuartmorgan-g April 19, 2022 17:33

stuartmorgan-g approved these changes Apr 21, 2022

View reviewed changes

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 Apr 21, 2022

fluttergithubbot merged commit f2dd3d9 into flutter:main Apr 21, 2022

engine-flutter-autoroll mentioned this pull request Apr 22, 2022

Roll Plugins from 5c3c1b8aac06 to ca63d964d9fd (10 revisions) flutter/flutter#102379

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 22, 2022

f2dd3d98a [video_player] Add setClosedCaptionFile method to VideoPlay…

12d4d18

…erController (flutter/plugins#5105)

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[video_player] Add setClosedCaptionFile method to VideoPlayerControll…

1332954

…er (flutter#5105)

Conversation

ibrahimcetin commented Mar 24, 2022 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibrahimcetin Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibrahimcetin commented Apr 19, 2022

Uh oh!

stuartmorgan-g commented Apr 19, 2022

Uh oh!

ibrahimcetin commented Apr 19, 2022

ibrahimcetin commented Mar 24, 2022 •

edited by stuartmorgan-g

Loading

ibrahimcetin Apr 13, 2022 •

edited

Loading