-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Closed Captioning] Create SubRip file parser and dart closed caption data object #2473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
packages/video_player/closed_caption_file/lib/closed_caption_file.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! The approach is good, but there's a few things that I think should be changed before landing.
fourthCaption.text, | ||
'- [ Machinery Beeping ]\n- I\'m not sure what that was,', | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Some test cases for what happens with invalid input would be great, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/video_player/closed_caption_file/lib/closed_caption_file.dart
Outdated
Show resolved
Hide resolved
packages/video_player/closed_caption_file/lib/closed_caption_file.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit test is failing in CI, but it's not obvious to me why:
00:07 +12 -1: /tmp/cirrus-ci-build/packages/video_player/video_player/test/sub_rip_file_test.dart: Parses SubRip file [E]
Unsupported operation: _Namespace
package:dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 196:49 throw_
package:dart-sdk/lib/_internal/js_dev_runtime/patch/io_patch.dart 202:5 get _namespace
package:dart-sdk/lib/io/file_impl.dart 488:31 openSync
package:dart-sdk/lib/io/file_impl.dart 549:18 readAsBytesSync
package:dart-sdk/lib/io/file_impl.dart 594:18 readAsStringSync
sub_rip_file_test.dart 15:31 <fn>
package:test_api <fn>
Just double checking, it passes locally? the js_dev_runtime
part looks suspicious to me, maybe this has something to do with the federated web implementation?
video_player's minor version should be bumped in pubspec.yaml and CHANGELOG.md now since it's been moved into that package.
Other than that the implementation here looks solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix!
… data object (flutter#2473) This PR specifies a dart object that represents a "Closed Caption". This will be useful in a follow up PR, where I will add closed caption support the the `VideoPlayerController`.
… data object (flutter#2473) This PR specifies a dart object that represents a "Closed Caption". This will be useful in a follow up PR, where I will add closed caption support the the `VideoPlayerController`.
… data object (flutter#2473) This PR specifies a dart object that represents a "Closed Caption". This will be useful in a follow up PR, where I will add closed caption support the the `VideoPlayerController`.
… data object (flutter#2473) This PR specifies a dart object that represents a "Closed Caption". This will be useful in a follow up PR, where I will add closed caption support the the `VideoPlayerController`.
Description
This PR specifies a dart object that represents a "Closed Caption". This will be useful in a follow up PR, where I will add closed caption support the the
VideoPlayerController
.Related Issues
flutter/flutter#25388
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.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?