[video_player] Optimize caption retrieval with binary search in VideoPlayerController

[abdelaziz-mahdy]

Just found the todo in the code and it was from 3 years ago, so i wanted to implement it since it may help people using captions to have more optimized code

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• 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.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[tarrinneal]

@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.

[stuartmorgan]

@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.

The fact that it's a more efficient search doesn't inherently need a test; that's essentially just a refactoring. However, this seems like a good opportunity to make sure we have reasonably robust coverage of caption correctness. E.g., is there a unit test that would fail if the initial sort were commented out?

[abdelaziz-mahdy]

@stuartmorgan does this change need a test? It shouldn't be changing any behaviors, I'm not sure how a test would work honestly.

The fact that it's a more efficient search doesn't inherently need a test; that's essentially just a refactoring. However, this seems like a good opportunity to make sure we have reasonably robust coverage of caption correctness. E.g., is there a unit test that would fail if the initial sort were commented out?

I can add that , so in general more caption edge cases

[abdelaziz-mahdy]

I just checked the test for sorting, and it shows that the captions are not sorted. After rechecking the code, I realized that the captions are implemented as a getter, which means it’s a read-only operation.

I have two solutions in mind:

1- Modify the public-facing controller by adding a getter to retrieve the sorted captions. However, based on my observations, it’s not recommended to change the interface, so this solution might not be valid.

2- Ensure that all ClosedCaptionFile classes return sorted captions. I will implement this solution for now, and if the public-facing approach turns out to be better, we can always revert to it.

Edit: fix number 2 is not clean since it doesnt enforce the sorting so i am going with the first fix

[abdelaziz-mahdy]

tests implemented and improved the logic for the binary search since it was incorrect in some cases

for this test test('captions are sorted correctly on initialization'
this test can be ignored and if thats the case we can remove the new public member to the class (my main issue is that i was not able to expose a private member only for testing) if someone can point me to the right direction that will help a lot

[stuartmorgan]

Why is new public API needed? I don't see any implementation code using this.

[abdelaziz-mahdy]

To be able to access that list in testing, as I mentioned in the last comment I couldn't make it private and available for testing,

If the test is not needed I can remove it with the test

[abdelaziz-mahdy]

i removed it and the test.

[stuartmorgan]

If something needs to be visible only for testing, it should be annotated @visibleForTesting.

It's not clear to me why even that would be necessary though; API to get the current caption is already public, and I would expect that it should be straightforward to construct a case where getting the caption for some time would be incorrect unless the captions are sorted internally.

[abdelaziz-mahdy]

abstract class ClosedCaptionFile {
  /// The full list of captions from a given file.
  ///
  /// The [captions] will be in the order that they appear in the given file.
  List<Caption> get captions;
}

its only a getter so calling sort on it doesnt sort the original list, this is why i copied the list and sorted it and the public method was for the testing part only, if the test is not needed the public method is not needed

but _sortedCaptions is needed

[stuartmorgan]

this is why i copied the list and sorted it
[...]
but _sortedCaptions is needed

I'm not sure what part of my comments led you to believe I don't understand _sortedCaptions. My comment is about the new public API.

the public method was for the testing part only, if the test is not needed the public method is not needed

I still don't understand why you are presenting a binary choice between adding new public API and having testing. Whether or not the correct caption is fetched for a given time is already an observable property with existing public API. You should be able to construct a case where, with binary search, the wrong caption is reported without the internal sorting step, and thus validate that it is being handled correctly without directly inspecting internal state via new public accessors.

Testing internal implementation details is an anti-pattern, and should only be done when the important properties cannot be tested via the actual client APIs. I have not seen any argument for why that would not be the case here. What scenario are you unable to test without adding a new public API?

[abdelaziz-mahdy]

i didnt mean all the testing of the testing is ignored, just this test captions are sorted correctly on initialization since it requires the expose of the public interface so i can check if they are sorted correctly

List<Caption>? get sortedCaptions without it i cant get the sorted list and without the sorted list i cant make that test

but i did add test cases for not sorted captions and tested retrieving them which works correctly and if the sorting is removed they wont pass

so i think those are enough, and no need to introduce the public interface since all of the cases are covered as you mentioned

and i did remove the public method.

edit: i am sorry if my english is not clear enough since its not my first language,

but in the end the public method is not needed since the other tests already covered the sorting checks