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

VideoPlayer.tsx: Don't newline subtitles. (Help yomitan get the entire subtitle) #474

Merged
merged 6 commits into from
Aug 11, 2024

Conversation

Viterkim
Copy link
Contributor

Related to the issue i had, after still struggling and not being able to do anything with yomitan, i decided to just fix it here.
#459 (comment)

image
image

Works!

@Viterkim Viterkim changed the title VideoPlayer.tsx: Don't newline subtitles. (Let yomitan pick up the fu… VideoPlayer.tsx: Don't newline subtitles. (Help yomitan get the entire subtitle) Jul 25, 2024
@Viterkim
Copy link
Contributor Author

Viterkim commented Jul 25, 2024

Also this has been driving me crazy, trying to run it locally. (I did build and point nginx to it, everything works except the extension).

image
The extension doesn't seem to work/thinks its a different site.

image

the streaming video options also dissapears.

There's also hardcoded things, preventing you from simply running it on localhost, it has to be at localhost/asbplayer

    GET http://localhost/asbplayer/static/js/main.62a96a7c.js net::ERR_ABORTED 404 (Not Found)
script.js:1 Ignoring Event: localhost

Was thinking it would be convenient to run it/test everything + run it locally if internet goes down :)

@killergerbah
Copy link
Owner

Have you tried fixing this problem using the regex-based text replacement feature instead?
Under misc settings you can configure asbplayer to replace all \n characters with a space .

About the extension and locally-hosted asbplayer website - prod versions of the extension (like from Chrome web store) do not recognize localhost due to restrictions on the webstore. You can build a dev version of the extension instead if you want it to work with a locally-hosted asbplayer website.

@Viterkim
Copy link
Contributor Author

That makes it into 1 long line yea, but that's definitely worse to read. (I think basically all long subtitles are stacked). So not ideal :/
image

And i guess for making it work offline, it would be way easier to just add a serviceworker and add "progressive web app" support, that way the browser would cache the app even if you don't have internet.

@killergerbah
Copy link
Owner

Have you tried this setting on Yomitan?
image

I am hesitant to make changes just to workaround specific problems with other software.

@Viterkim
Copy link
Contributor Author

Viterkim commented Aug 3, 2024

Yes i tried that, it adds random text from other sentences to the line.
1

I'm not really sure what the bad thing about adding it would be :D
I have another friend who has the same issue but who doesn't use github. I think it's valid to be able to get the full sentence without using the extension? (And i think for most people, the only reason why they're using a browser media player, is for pop up dictonaries).

@killergerbah
Copy link
Owner

Okay I see your point.
I'll come back to this - I want to be reasonably sure this change is safe to make.

@Viterkim
Copy link
Contributor Author

Viterkim commented Aug 4, 2024

Okay I see your point. I'll come back to this - I want to be reasonably sure this change is safe to make.

Sure! Also don't want to pressure anything!
Maybe make it an option in the misc settings to be 100% sure? (So maybe default on, then Don't turn newlines into tags in subtitles or something

@@ -164,7 +164,9 @@ const showingSubtitleHtml = (
`;
}

return `<span style="${subtitleStyles}" class="${subtitleClasses}">${subtitle.text}</span>`;
const lines = subtitle.text.split('\n');
Copy link
Owner

Choose a reason for hiding this comment

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

This change should be repeated on line 198 to get the same behavior when DOM caching is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
image

@Viterkim Viterkim force-pushed the subtitle-help-yomitan branch 2 times, most recently from ef802f3 to 8116d67 Compare August 10, 2024 11:30
Copy link
Owner

@killergerbah killergerbah left a comment

Choose a reason for hiding this comment

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

It looks like there's a bug: multiple subtitle tracks are rendered with extra space between them now.

@Viterkim
Copy link
Contributor Author

Viterkim commented Aug 11, 2024

It looks like there's a bug: multiple subtitle tracks are rendered with extra space between them now.

Do you have a subtitle file i could try out and see?
EDIT: ah or is it just in the picture i posted?

@Viterkim
Copy link
Contributor Author

Here's my branch, and master. (With & without the dom option). (No difference)
image
image
image
image

@Viterkim
Copy link
Contributor Author

Or is it this? foun some subtitle online with dual, but it seems really weird
image
seems to only happen on my branch, with Pre-cache Subtitle DOM on
image

I'll try to fix it

…ith pre-cache subtitle dom + multiple sub track)
@Viterkim
Copy link
Contributor Author

Viterkim commented Aug 11, 2024

image

    line-height: normal;
    display: inline;

seems to do it
used this to test
(Hi10)Seishun_Buta_Yarou_wa_Bunny_Girl_Senpai_no_Yume_wo_Minai-13(1080p)(Vivid)(D7FBB8B6).ja-en.zip

edit: nvm it makes it into 1 long subtitle

@Viterkim
Copy link
Contributor Author

Viterkim commented Aug 11, 2024

image
ok should work now

i think setting line-height: inherit; looks better?
image

@Viterkim
Copy link
Contributor Author

image

@killergerbah
Copy link
Owner

Can you run Prettier on VideoPlayer.tsx? I can merge after that.

@killergerbah killergerbah merged commit 02662f0 into killergerbah:main Aug 11, 2024
1 check passed
@killergerbah
Copy link
Owner

Thanks!

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.

2 participants