Skip to content

Conversation

@Friendlygamemaker
Copy link

@Friendlygamemaker Friendlygamemaker commented Aug 10, 2025

better lyric view using timestamps from lrc file
reads from SRT files and does karaoke style lyrics
can choose how many lines appear on screen
can choose what size the lines should be
font size scales down to fit on screen
feel free to reach out with feedback or suggestions

@digimezzo
Copy link
Owner

@Friendlygamemaker Thank you for this pull request! I have reviewed the code and have a few questions:

In lyricsService.getLyricsAsync, a check on this.showRichLyrics defines which type of lyrics and in which order they are processed. The priority of reading embedded and file lyrics is different. Why is that?

In LrcLyricsGetter.getLyricsAsync, won't the following code crash if there is no timestamp in the line? I'd expect lineParts[1] to be undefined in that case.

const lineParts = lines[i].replace('[', '').split(']');
const lineWithoutTimestamp: string = lineParts[1];

@Friendlygamemaker
Copy link
Author

In lyricsService.getLyricsAsync, a check on this.showRichLyrics defines which type of lyrics and in which order they are processed. The priority of reading embedded and file lyrics is different. Why is that?

We change the priority of the lyrics read so that we can look specifically for lyrics with timestamps first.

In LrcLyricsGetter.getLyricsAsync, won't the following code crash if there is no timestamp in the line? I'd expect lineParts[1] to be undefined in that case.

you're absolutely right I may need to add safe guards to make sure the program throws properly and doesn't crash the program. However, a valid LRC file should have timestamps.

Sorry for the late response. Thanks for looking at my pull request.

@Friendlygamemaker
Copy link
Author

This should prevent the program from crashing.
should catch any error then move onto the next way to get lyrics (embedded, cached, or online).

try {
    lyrics = await this.lrcLyricsGetter.getLyricsAsync(track);
} catch (e) {
    this.logger.error(e, 'Could not get LRC lyrics', 'LyricsService', 'getLyricsAsync');
}

@digimezzo digimezzo added this to the Dopamine 3.0.2 milestone Nov 17, 2025
@digimezzo
Copy link
Owner

@Friendlygamemaker Sorry for the delay in processing your PR. I'm planning it for version 3.0.2. I'm currently finishing up the work for 3.0.1.

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