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

Fix chromecast subtitle position #379

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

brianardiles
Copy link

No description provided.

@vankasteelj
Copy link
Member

What does your fork changes exactly? I think you should rather open an issue/PR on main module, for it to remain maintained and updated.

@brianardiles
Copy link
Author

@vankasteelj My fork change this:
VTT_BODY.push(module.exports.formatTime(caption.startTimeMicro) + ' --> ' + module.exports.formatTime(caption.endTimeMicro));

to

VTT_BODY.push(module.exports.formatTime(caption.startTimeMicro) + ' --> ' + module.exports.formatTime(caption.endTimeMicro) + ' line:90%');

This edit the subs.vtt and add margin from bottom with this change reading is much easier

img_20160424_123928

@sulaimanob
Copy link

can you give me .exe for your fix

@vankasteelj
Copy link
Member

I'm uncomfortable with this. I know it breaks DLNA on LG, but that probably can be fixed. My main concern here is the fork of node-captions without a PR being sent back upstream.

@Breaking1
Copy link

Breaking1 commented May 1, 2016

This was fixed when i got an update on my Chromecast, as you can see here i think it is a Chromecast firmware issue and an update was provided to fix this.

https://productforums.google.com/forum/?hl=da#!topic/chromecast/Qe76EGhzlII;context-place=forum/chromecast

@brianardiles
Copy link
Author

@Breaking1 No, editing the vtt subtitles file you can put the text in any position on the screen.

@vankasteelj I can think of another way to modify the script in pt

@sulaimanob go to C:\Users\YOURUSERNAMME\AppData\Local\Popcorn-Time\node_modules\node-captions\lib
open vtt.js file search this line "VTT_BODY.push(module.exports.formatTime(caption.startTimeMicro) + ' --> ' + module.exports.formatTime(caption.endTimeMicro));"

and replace with VTT_BODY.push(module.exports.formatTime(caption.startTimeMicro) + ' --> ' + module.exports.formatTime(caption.endTimeMicro) + ' line:90%');

now restart pt

@brianardiles
Copy link
Author

@vankasteelj check the last commit in node-captions 11 Aug 2015

@vankasteelj
Copy link
Member

vankasteelj commented May 24, 2016

Okay, I'm voting for this, apparently node-captions is dead (maintainer not online for a year) @xaiki you ok merging this too?

(not merging like it's now, it has leftovers from a merge that broke dlna, only keep the actual changes from brai4u: "node-captions": "git+https://github.com/brai4u/node-captions.git",)

@xaiki
Copy link
Member

xaiki commented May 24, 2016

why do we have all those commits ? didn't you revert those in the firstplace ?

@xaiki
Copy link
Member

xaiki commented May 24, 2016

changes look ok, but i have no tv to test on, a few nitpicks:

  • @brai4u you should own node-captions and publish it on NPM
  • @brai4u you should split out your changes and work with @vankasteelj to get these changes not to break his TV... (or any others fwiw)

@brianardiles
Copy link
Author

Search on google "problem chromecast subtitles popcorn time" and should find pics like this

img

I used the version with my commit and works like this

f0e7ef5e-0a19-11e6-8d84-26769eef9117

@xaiki
Copy link
Member

xaiki commented May 31, 2016

can we solve the nitpicks and merge this ?

1 similar comment
@xaiki
Copy link
Member

xaiki commented Jun 1, 2016

can we solve the nitpicks and merge this ?

@z11h
Copy link

z11h commented Jun 1, 2016

It would be great if we can merge this!

@xaiki
Copy link
Member

xaiki commented Jun 1, 2016

@zakariaridouh do you want to do the cleanup @vankasteelj asked for ? go for it =D

@vankasteelj
Copy link
Member

@brai4u you think you can also merge the PRs that are made on the main repo, and publish your version to NPM (I don't like the idea of depending on you not deleting your repo^^) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants