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

Adds typewriter support to subtitles #3

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

GGshor
Copy link

@GGshor GGshor commented Feb 4, 2024

Adds typewriter support to subtitles, suggestion originally came from: discord

Typewriter is by default off, but can be toggled by enabling it:
image

Changes:

  • Added typewriter module
  • Added typewriter option
  • Typewriter is based on duration and message length

@GGshor GGshor added the enhancement New feature or request label Feb 4, 2024
@GGshor GGshor self-assigned this Feb 4, 2024
Copy link
Member

@TheNexusAvenger TheNexusAvenger left a comment

Choose a reason for hiding this comment

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

Got a couple formatting things, but 2 behavior questions with rich text as well. I haven't tested the functionality though.

I'd also like this new option to be presented in the README. Right now, this feature is undocumented.

src/EventTransform.lua Outdated Show resolved Hide resolved
src/LocalAudioSubtitlesTypes.lua Outdated Show resolved Hide resolved
@@ -120,6 +123,9 @@ function SubtitleEntry:UpdateMultipleText(): ()
TextTransparency = 0,
})
self.Window:UpdateSize()
if self.TypeWriterEnabled then
task.spawn(TypeWriter, self.TextLabel, Message, (self.Duration/Message:len()))
Copy link
Member

Choose a reason for hiding this comment

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

I think we use spaces around /, so self.Duration / Message:len()).
I think we also use string.len(String) instead of String:len(), so I'd think self.Duration / string.len(Message)) would be more consistent even though it is longer.

Also, does this mean the typewriter option can't handle rich text? That would need to either be handled or documented.

Copy link
Author

@GGshor GGshor Feb 7, 2024

Choose a reason for hiding this comment

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

Will get back to you about the rich text, as of now I've had no issues with rich text while using typewriter.
Marking this PR as a draft until I can get the proof it.

Comment on lines 5 to 6
displayText = displayText:gsub("<br%s*/>", "\n")
displayText:gsub("<[^<>]->", "")
Copy link
Member

Choose a reason for hiding this comment

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

Do we use String:gsub(...) in other parts of the project as opposed to string.gsub(String, ...)?
Behavior-wise, I'm not sure this properly handled mismatched tags (<a>thing</b> -> thing, when it shouldn't). Handling that may be tricky. It wasn't easy to get right in Nexus Admin for string escaping (https://github.com/TheNexusAvenger/Nexus-Admin/blob/master/src/Common/Filter.lua#L29-L42).

Copy link
Author

Choose a reason for hiding this comment

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

Wil look into handling mismatched tags.

src/Util/TypeWriter.lua Outdated Show resolved Hide resolved
@GGshor GGshor marked this pull request as draft February 7, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants