Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subtitles #188

Closed
wants to merge 7 commits into from
Closed

Subtitles #188

wants to merge 7 commits into from

Conversation

kirill09
Copy link

subtitle display

@Reached
Copy link

Reached commented Nov 1, 2019

@kirill09 is this working for you? :)

@kirill09
Copy link
Author

kirill09 commented Nov 1, 2019

@kirill09 is this working for you? :)

yes

@Ahmadre
Copy link
Collaborator

Ahmadre commented Mar 16, 2020

@kirill09 thank you sooo much for this PR :) whats about the conflicts? could you check them?

@Ahmadre
Copy link
Collaborator

Ahmadre commented Nov 9, 2020

@kirill09 I think these changes from you are very important and give chewie a big plus. I would appreciate it, if you have a second look here into this PR and the merge conflicts, so we can have subtitles in chewie :)

@Ahmadre
Copy link
Collaborator

Ahmadre commented Nov 12, 2020

I fixed the conflicts. I will check everything now. @nstrelow if you got time you can also have a quick look. Maybe I am too fast 😂

The cool thing here is the builder:
Bildschirmfoto 2020-11-12 um 19 04 08

@Ahmadre
Copy link
Collaborator

Ahmadre commented Nov 12, 2020

Ok, tested everything, here are my results:
I don't understand why we're using such dirty algorithm to pass:

  • index
  • start duration
  • end duration
  • text
    by this:
subtitle: Subtitles([
    Subtitle('0\n00:00:00 --> 00:00:10\nHello from Subtitles :)'),
]),
subtitleBuilder: (context, subtitle) {
    return Text(subtitle);
},

The result in the UI is:
Bildschirmfoto 2020-11-12 um 20 29 14

Honestly this is not really user or developer friendly. It's very easy in Dart to export a simple model for this. Yes, model is existing but factory constructor is horrible ^^ sorry to say that.

Also: the parameter: subtitle was existing as a parameter but was not passed into the constructor, so I had to fix that.

My recommendation: I'll refactor the Subtitle model to pass user-friendly parameters rather then a string like above.

/cc @nstrelow

Edit: I would also prefer that subtitles appear inside the Video and not in the chewie container.

@@ -269,6 +270,8 @@ class ChewieController extends ChangeNotifier {
/// Defines a custom RoutePageBuilder for the fullscreen
final ChewieRoutePageBuilder routePageBuilder;

Subtitles subtitle;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This wasn't mention in the constructor. Already fixed.


static Duration stringToDuration(String value) {
final component = value.split(':');
return Duration(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Added check to also pass: 00:00:00 if milliseconds not needed. Already fixed.

}

class Subtitle {
factory Subtitle(String value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

factory constructor will be refactored so it's more user friendly to use.

@@ -75,6 +79,7 @@ class _CupertinoControlsState extends State<CupertinoControls> {
children: <Widget>[
_buildTopBar(backgroundColor, iconColor, barHeight, buttonPadding),
_buildHitArea(),
_buildSubtitles(chewieController.subtitle),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't align relatively to the video aspect ratio. Will be fixed.

@Ahmadre Ahmadre self-assigned this Nov 12, 2020
@kennylbj
Copy link

Is this PR ready to merged?

@Ahmadre
Copy link
Collaborator

Ahmadre commented Nov 16, 2020

Is this PR ready to merged?

Nope, I am working on it 🧑🏻‍💻

@Ahmadre
Copy link
Collaborator

Ahmadre commented Jan 21, 2021

Hey guys 👋

Sorry for the delay. I moved many times within Germany and now I can continue my work on this important feature.

I already got it working, but it's hidden by the controls and I want to make the subtitles aware of the controls.

Be prepared 💪🏼

@Ahmadre Ahmadre mentioned this pull request May 24, 2021
@Ahmadre
Copy link
Collaborator

Ahmadre commented May 24, 2021

Since @kirill09 's repo and branch are somehow damaged/deleted, I've moves this PR to: #475

please follow that PR. It's ready to release, we just have to review it!

@Ahmadre Ahmadre closed this May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants