Skip to content

Subtitles#188

Closed
kirill09 wants to merge 7 commits intofluttercommunity:masterfrom
kirill09:subtitles
Closed

Subtitles#188
kirill09 wants to merge 7 commits intofluttercommunity:masterfrom
kirill09:subtitles

Conversation

@kirill09
Copy link
Copy Markdown

subtitle display

@Reached
Copy link
Copy Markdown

Reached commented Nov 1, 2019

@kirill09 is this working for you? :)

@kirill09
Copy link
Copy Markdown
Author

kirill09 commented Nov 1, 2019

@kirill09 is this working for you? :)

yes

@Ahmadre
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

/// Defines a custom RoutePageBuilder for the fullscreen
final ChewieRoutePageBuilder routePageBuilder;

Subtitles subtitle;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

children: <Widget>[
_buildTopBar(backgroundColor, iconColor, barHeight, buttonPadding),
_buildHitArea(),
_buildSubtitles(chewieController.subtitle),
Copy link
Copy Markdown
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
Copy Markdown

Is this PR ready to merged?

@Ahmadre
Copy link
Copy Markdown
Collaborator

Ahmadre commented Nov 16, 2020

Is this PR ready to merged?

Nope, I am working on it 🧑🏻‍💻

@Ahmadre
Copy link
Copy Markdown
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
Copy Markdown
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