Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
/pluginsPublic archive

[video_player] VTT Support#2878

Merged
fluttergithubbot merged 76 commits intoflutter:masterfrom
ferrazrx:ferrazrx/web_vtt
Sep 22, 2021
Merged

[video_player] VTT Support#2878
fluttergithubbot merged 76 commits intoflutter:masterfrom
ferrazrx:ferrazrx/web_vtt

Conversation

@ferrazrx
Copy link
Contributor

@ferrazrxferrazrx commentedJul 15, 2020•
edited
Loading

Description

Class WebVTTCaptionFile created.

Related Issues

flutter/flutter#50595

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 theContributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests forall changed/updated/fixed behaviors (SeeContributor 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 theFlutter 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 thepub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed theCLA.
  • 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 isnot a breaking change.

alexodus reacted with thumbs up emoji
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝Please visithttps://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with@googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️Googlers:Go here for more info.

@ferrazrxferrazrx changed the titleFerrazrx/web vtt[video_player] VTT SupportJul 15, 2020
Copy link
Contributor

@johnsonmhjohnsonmh left a comment

Choose a reason for hiding this comment

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

This PR looks awesome! Thank you so much for the contribution.

I just left a few comments for suggestions/nits here and there.

@matthew-carrollmatthew-carroll removed their request for reviewJuly 15, 2020 20:46
Copy link
Contributor

@johnsonmhjohnsonmh left a comment

Choose a reason for hiding this comment

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

Thanks for adding thorough tests!

Copy link
Contributor

@cyanglazcyanglaz left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, I have left some comments. Also we need to update the CHANGELOG and the version number in thepubspec.yaml

@@ -0,0 +1,200 @@
// Copyright 2020 The Chromium Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be "Copyright 2013 The Flutter Authors." in both new files, which is whyformat is unhappy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

final Duration start;
///
/// When the value is null, the caption object is invalid.
final Duration? start;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change, because this whole file is exported.

Why do we need the ability to make invalidCaption objects in the first place? That seems like a strange thing to allow. If this is just for:

    final Caption newCaption = Caption(      number: captionNumber,      start: startAndEnd.start,      end: startAndEnd.end,      text: textWithoutFormat,    );    if (newCaption.start != null && newCaption.end != null) {      captions.add(newCaption);      captionNumber++;    }

the caption is being thrown away anyway, so we can just not make it in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call.
Updated to makestart andend non-null

: _captions = _parseCaptionsFromWebVttString(fileContents);

/// The entire body of the Vtt file.
final String fileContents;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this public?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was probably a copy paste from SubRipCaptionFile in sub_rip.dart. RemovedfileContents completely. Also added TODO insub_rip.dart. It's going to be a breaking change to update it in sub_rip.dart and I think it would be better to update it when there's a meaningful breaking change for this plugin.

I'm not sure how to keep track of this tho. We might forgot about this when doing breaking changes in the plugin.

final List<Caption> captions = <Caption>[];

// Ignore metadata
List<String> metadata = ['HEADER', 'NOTE', 'REGION', 'WEBVTT'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:Set

cyanglaz reacted with thumbs up emoji
// [00:00.000 --> 01:24.000 align:center]
// ['Introduction']
// ]
// if caption has just header or time, but no text, captionLines.length will be 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the comments to be capitalized and end with a period, per normal style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

}

final List<String> dotSections = timestampString.split('.');
final List<String> hoursMinutesSeconds = dotSections[0].split(':');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:timeComponents. There's no guarantee that this has all of these.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

import 'closed_caption_file.dart';
import 'package:html/parser.dart' as html_parser;

/// Represents a [ClosedCaptionFile], parsed from the WebVtt file format.
Copy link
Contributor

Choose a reason for hiding this comment

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

WebVTT

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated so all the appearance of WebVtt are WebVTT now.

import 'package:html/parser.dart' as html_parser;

/// Represents a [ClosedCaptionFile], parsed from the WebVtt file format.
/// See: https://en.wikipedia.org/wiki/WebVtt
Copy link
Contributor

Choose a reason for hiding this comment

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

This link is broken; it'shttps://en.wikipedia.org/wiki/WebVTT

Copy link
Contributor

Choose a reason for hiding this comment

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

done

minutes = int.parse(hoursMinutesSeconds[1]);
seconds = int.parse(hoursMinutesSeconds[2]);
} else if (int.parse(hoursMinutesSeconds[0]) > 59) {
// Timestamp takes the form of [hours]:[minutes].[milliseconds]
Copy link
Contributor

Choose a reason for hiding this comment

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

This seemed deeply strange to me so I checkedthe spec, and this isn't a valid timestamp. We shouldn't handle invalid values unless there's substantial real-world evidence that lots of people have written this (which seems unlikely, because even if other parsers allow this it is incapable of expressing any time within the first59 hours of a video)

// Timestamp takes the form of [minutes]:[seconds].[milliseconds]
minutes = int.parse(hoursMinutesSeconds[0]);
seconds = int.parse(hoursMinutesSeconds[1]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the above is removed, this can all condense to:

int hours = 0;if (hoursMinutesSeconds.length == 3) {  hours = int.parse(timeComponents.removeAt(0));}final int minutes = int.parse(timeComponents.removeAt(0));final int seconds = int.parse(timeComponents.removeAt(0));

It would be good to add a safety check that the length is either 2 or 3 and returns null first, as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@stuartmorgan-gstuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM with various nits.

(If for whatever reason you feel like doing the bigger change, I can re-review that part, otherwise it's good to land without another review pass.)


List<String> milisecondsStyles = dotSections[1].split(" ");

// TODO(cyanglaz): Handle caption styles.
Copy link
Contributor

@stuartmorgan-gstuartmorgan-gSep 21, 2021•
edited
Loading

Choose a reason for hiding this comment

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

As I'm reading this again: it's weird that _parseWebVTTTimestamp is where captions would be handled. I think the caller—which is already doing a regex match for timestamps anyway—should just use match groups (the regex would need to be adjusted slightly since right now it's making groups for all the sub-pieces of the timestamp, which isn't actually used) to extract just the timestamps, and then pass those here, removing the need for themilisecondsStyles = dotSections[1].split(" "). Then the caption-handling TODO would go in the calling function, not here.

But I'm fine with that being something that's cleaned up if/when this functionality is added since I don't want to keep piling changes onto a PR you adopted in the first place :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I'll put a note in the code and link this comment.

for (final caption in _closedCaptionFile!.captions) {
if (caption.start <= position && caption.end >= position) {
final Duration? start = caption.start;
final Duration? end = caption.end;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is no longer needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@cyanglazcyanglaz 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. labelSep 22, 2021
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • This pull request has changes requested by@cyanglaz. Please resolve those before re-applying the label.

@fluttergithubbotfluttergithubbot removed 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. labelSep 22, 2021
Copy link
Contributor

@cyanglazcyanglaz left a comment

Choose a reason for hiding this comment

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

Approving to fix CI

@cyanglazcyanglaz 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. labelSep 22, 2021
@fluttergithubbotfluttergithubbot merged commited2e7a7 intoflutter:masterSep 22, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull requestSep 22, 2021
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull requestSep 22, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull requestSep 27, 2021
NickalasB added a commit to NickalasB/plugins that referenced this pull requestSep 27, 2021
* master: (51 commits)  [webview_flutter] Update version number app_facing package (flutter#4375)  [webview_flutter] Adjust integration test domains (flutter#4383)  Remove some trivial custom analysis options files (flutter#4379)  [google_maps_flutter_platfomr_interface] Add Marker drag events (flutter#2653)  [flutter_plugin_tools] Improve version check error handling (flutter#4376)  [flutter_plugin_tools] Allow overriding breaking change check (flutter#4369)  [url_launcher] Error handling when URL cannot be parsed with Uri.parse (flutter#4365)  [webview_flutter] Migrate main package to fully federated architecture. (flutter#4366)  [google_sign_in] Bump minimum Flutter version and iOS deployment target (flutter#4334)  Add false secret lists, and enforce ordering (flutter#4372)  [camera_web] Update usage documentation (flutter#4371)  [video_player] VTT Support (flutter#2878)  Require authors file (flutter#4367)  [flutter_plugin_tools] Fix federated safety check (flutter#4368)  [webview_flutter] Extract WKWebView implementation into a separate package (flutter#4345)  [webview_flutter] Extract Android implementation into a separate package (flutter#4343)  [in_app_purchase] Ensure the `introductoryPriceMicros` field is populated correctly. (flutter#4364)  [flutter_plugin_tools] Add a federated PR safety check (flutter#4329)  [camera] Add web support (flutter#4240)  [webview_flutter] Bump minimum Flutter version and iOS deployment target (flutter#4361)  ...# Conflicts:#packages/webview_flutter/webview_flutter/lib/platform_interface.dart#packages/webview_flutter/webview_flutter/lib/src/webview_method_channel.dart#packages/webview_flutter/webview_flutter/lib/webview_flutter.dart
clocksmith pushed a commit to clocksmith/flutter that referenced this pull requestOct 29, 2021
KyleFin pushed a commit to KyleFin/plugins that referenced this pull requestDec 21, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

3 more reviewers

@stuartmorgan-gstuartmorgan-gstuartmorgan-g approved these changes

@cyanglazcyanglazcyanglaz approved these changes

@johnsonmhjohnsonmhjohnsonmh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cyanglazcyanglaz

Labels

cla: yesp: video_playerwaiting 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.

8 participants

@ferrazrx@googlebot@cyanglaz@ayush221b@vanlooverenkoen@stuartmorgan-g@fluttergithubbot@johnsonmh

Comments

[8]ページ先頭
©2009-2026 Movatter.jp