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

Add subtitle mode (videoplayer-like displaying), fix parsing SRT and refactor #1

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

FeniXb3
Copy link

@FeniXb3 FeniXb3 commented Mar 25, 2024

I found this repository while looking for subtitles plugin for my accessibility workshop. I liked that you use SRT format, as I was looking for something that could be used in cutscenes. However, I realized that it's not using the data as videoplayers do and instead of creating my own version from scratch I decided do build on top of your project and give my changes back.

It started small, but in the end git diff will not be useful to finding out what the changes are. So, to sum up in few points:

  1. Code was refactored - I've seen some redundant parts and started to limit repetitions whenever I knew what to do.
  2. Static types were added in most places - it made it easier for me to know if everything was working well, as I had more code completion.
  3. Some issues were fixed - SRT might have multiple lines in each segment and it was not working correctly. Regex used can be tested here - https://regex101.com/r/MCt4cQ/1 It's based on this SO answer - https://stackoverflow.com/a/71585341/1816426
  4. "Subtitles" mode was added - that's the only thing I needed and I believe more people might be interested in it.
  5. Getting parsed captions/subtitles only was separated - in my opinion it's better than having the "TimeOnly" parameter.
  6. Return types of functions were unified - some functions were either returning Animation or bool or Dictionary.

Feel free to take whatever you feel fitting. I know it's a lot for a single PR, but I didn't want to create multiple PRs with different changes, as most parts were interconnected due to refactor. You can check separate commits I did to see how it progressed.

There are some parts I would change even more but I have to move on with the workshop and leave reafactoring for now.

@FeniXb3
Copy link
Author

FeniXb3 commented Mar 25, 2024

Oh, and I forgot I wanted to attach a video, showing the "Subtitles" style:

2024-03-25.16-45-40.mp4

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.

None yet

1 participant