From 8928378a579dde893a2f39e5b73017a39d56667b Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 19 Jun 2026 17:18:08 -0400 Subject: [PATCH] refactor(lyrics): colocate and unexport cue-normalization helpers Move the cue-normalization machinery out of lyrics.go into a dedicated lyrics_normalize.go (with lyrics_normalize_test.go), leaving lyrics.go to hold just the shared lyric types and their methods. lyrics.go was mixing the domain type/contract definitions with format-agnostic post-processing. Unexport normalizeLyrics, normalizeCueLines, and normalizeLineTiming: they have no callers outside the model package, so they should not be part of its public API. NormalizeCueEnds stays exported because the Subsonic enhanced-lyrics serializer (server/subsonic/lyrics.go) resolves cue ends per agent group while building the response; that is the only legitimate cross-package caller. Also includes a small no-op robustness tweak in parseLRC: len(times) == 0 instead of times == nil (equivalent here, more idiomatic). No behavior change. --- model/lyrics.go | 126 ------------------------------- model/lyrics_lrc.go | 4 +- model/lyrics_lyricsfile.go | 6 +- model/lyrics_normalize.go | 134 +++++++++++++++++++++++++++++++++ model/lyrics_normalize_test.go | 33 ++++++++ model/lyrics_srt.go | 2 +- model/lyrics_test.go | 27 ------- model/lyrics_ttml.go | 8 +- 8 files changed, 177 insertions(+), 163 deletions(-) create mode 100644 model/lyrics_normalize.go create mode 100644 model/lyrics_normalize_test.go diff --git a/model/lyrics.go b/model/lyrics.go index 33c26e2cb..111b1c2a9 100644 --- a/model/lyrics.go +++ b/model/lyrics.go @@ -2,10 +2,7 @@ package model import ( "encoding/json" - "slices" "strings" - - "github.com/navidrome/navidrome/utils/gg" ) type Cue struct { @@ -94,126 +91,3 @@ func (ll LyricList) Main() (Lyrics, bool) { } return ll[0], true } - -func NormalizeLyrics(lyrics Lyrics) Lyrics { - lyrics.Line = NormalizeCueLines(lyrics.Line) - if len(lyrics.Agents) == 0 { - lyrics.Agents = nil - } - return lyrics -} - -func NormalizeCueLines(lines []Line) []Line { - if len(lines) == 0 { - return lines - } - - normalized := make([]Line, len(lines)) - copy(normalized, lines) - - for i := range normalized { - if len(normalized[i].Cue) > 0 { - normalized[i].Cue = slices.Clone(normalized[i].Cue) - } - - var fallbackEnd *int64 - if normalized[i].End != nil { - v := *normalized[i].End - fallbackEnd = &v - } else if i+1 < len(normalized) && normalized[i+1].Start != nil { - v := *normalized[i+1].Start - fallbackEnd = &v - } - - normalized[i] = normalizeCueLine(normalized[i], fallbackEnd) - } - - return normalized -} - -func NormalizeLineTiming(line Line) Line { - if len(line.Cue) == 0 { - return line - } - - var earliestStart *int64 - var latestEnd *int64 - for i := range line.Cue { - token := line.Cue[i] - if token.Start != nil { - if earliestStart == nil || *token.Start < *earliestStart { - v := *token.Start - earliestStart = &v - } - } - - candidateEnd := token.End - if candidateEnd == nil { - candidateEnd = token.Start - } - if candidateEnd != nil { - if latestEnd == nil || *candidateEnd > *latestEnd { - v := *candidateEnd - latestEnd = &v - } - } - } - - if line.Start == nil && earliestStart != nil { - v := *earliestStart - line.Start = &v - } - if line.End == nil && latestEnd != nil { - v := *latestEnd - line.End = &v - } - return line -} - -func normalizeCueLine(line Line, fallbackEnd *int64) Line { - if len(line.Cue) == 0 { - return line - } - line.Cue = NormalizeCueEnds(line.Cue, fallbackEnd) - return NormalizeLineTiming(line) -} - -// NormalizeCueEnds resolves missing cue end times within a single ordered cue -// group: each end is filled from the next cue's start, then from fallbackEnd, -// and is clamped so it never precedes the cue's own start nor overruns the next -// cue. End times are all-or-none — if any cue still lacks an end afterwards, all -// ends in the group are cleared. The input slice is never mutated. -func NormalizeCueEnds(cues []Cue, fallbackEnd *int64) []Cue { - if len(cues) == 0 { - return cues - } - - out := slices.Clone(cues) - for i := range out { - end := out[i].End - if end == nil { - if i+1 < len(out) && out[i+1].Start != nil { - end = out[i+1].Start - } else { - end = fallbackEnd - } - } - if end != nil && i+1 < len(out) && out[i+1].Start != nil && *end > *out[i+1].Start { - end = out[i+1].Start - } - if end != nil && out[i].Start != nil && *end < *out[i].Start { - end = out[i].Start - } - out[i].End = gg.Clone(end) - } - - for i := range out { - if out[i].End == nil { - for j := range out { - out[j].End = nil - } - break - } - } - return out -} diff --git a/model/lyrics_lrc.go b/model/lyrics_lrc.go index f8e75fb49..6fde0c8f5 100644 --- a/model/lyrics_lrc.go +++ b/model/lyrics_lrc.go @@ -84,7 +84,7 @@ func parseLRC(language, text string) (*Lyrics, error) { // The second condition is for when there is a timestamp in the middle of // a line (after any text) - if times == nil || times[0][0] != 0 { + if len(times) == 0 || times[0][0] != 0 { if validLine { priorLine += "\n" + line } @@ -166,7 +166,7 @@ func parseLRC(language, text string) (*Lyrics, error) { DisplayArtist: artist, DisplayTitle: title, Lang: language, - Line: NormalizeCueLines(structuredLines), + Line: normalizeCueLines(structuredLines), Offset: offset, Synced: synced, } diff --git a/model/lyrics_lyricsfile.go b/model/lyrics_lyricsfile.go index 1346da634..91e24947e 100644 --- a/model/lyrics_lyricsfile.go +++ b/model/lyrics_lyricsfile.go @@ -44,7 +44,7 @@ func parseLyricsfile(_ string, contents []byte) (LyricList, error) { } if doc.Metadata.Instrumental { - return LyricList{NormalizeLyrics(lyrics)}, nil + return LyricList{normalizeLyrics(lyrics)}, nil } if len(doc.Lines) == 0 { @@ -53,14 +53,14 @@ func parseLyricsfile(_ string, contents []byte) (LyricList, error) { return nil, nil } lyrics.Line = lines - return LyricList{NormalizeLyrics(lyrics)}, nil + return LyricList{normalizeLyrics(lyrics)}, nil } lines, agents := buildLyricsfileLines(doc.Lines) lyrics.Line = lines lyrics.Agents = agents lyrics.Synced = true - return LyricList{NormalizeLyrics(lyrics)}, nil + return LyricList{normalizeLyrics(lyrics)}, nil } const lyricsfileVersion = "1.0" diff --git a/model/lyrics_normalize.go b/model/lyrics_normalize.go new file mode 100644 index 000000000..276aa4da3 --- /dev/null +++ b/model/lyrics_normalize.go @@ -0,0 +1,134 @@ +package model + +import ( + "slices" + + "github.com/navidrome/navidrome/utils/gg" +) + +func normalizeLyrics(lyrics Lyrics) Lyrics { + lyrics.Line = normalizeCueLines(lyrics.Line) + if len(lyrics.Agents) == 0 { + lyrics.Agents = nil + } + return lyrics +} + +func normalizeCueLines(lines []Line) []Line { + if len(lines) == 0 { + return lines + } + + normalized := make([]Line, len(lines)) + copy(normalized, lines) + + for i := range normalized { + if len(normalized[i].Cue) > 0 { + normalized[i].Cue = slices.Clone(normalized[i].Cue) + } + + var fallbackEnd *int64 + if normalized[i].End != nil { + v := *normalized[i].End + fallbackEnd = &v + } else if i+1 < len(normalized) && normalized[i+1].Start != nil { + v := *normalized[i+1].Start + fallbackEnd = &v + } + + normalized[i] = normalizeCueLine(normalized[i], fallbackEnd) + } + + return normalized +} + +func normalizeLineTiming(line Line) Line { + if len(line.Cue) == 0 { + return line + } + + var earliestStart *int64 + var latestEnd *int64 + for i := range line.Cue { + token := line.Cue[i] + if token.Start != nil { + if earliestStart == nil || *token.Start < *earliestStart { + v := *token.Start + earliestStart = &v + } + } + + candidateEnd := token.End + if candidateEnd == nil { + candidateEnd = token.Start + } + if candidateEnd != nil { + if latestEnd == nil || *candidateEnd > *latestEnd { + v := *candidateEnd + latestEnd = &v + } + } + } + + if line.Start == nil && earliestStart != nil { + v := *earliestStart + line.Start = &v + } + if line.End == nil && latestEnd != nil { + v := *latestEnd + line.End = &v + } + return line +} + +func normalizeCueLine(line Line, fallbackEnd *int64) Line { + if len(line.Cue) == 0 { + return line + } + line.Cue = NormalizeCueEnds(line.Cue, fallbackEnd) + return normalizeLineTiming(line) +} + +// NormalizeCueEnds resolves missing cue end times within a single ordered cue +// group: each end is filled from the next cue's start, then from fallbackEnd, +// and is clamped so it never precedes the cue's own start nor overruns the next +// cue. End times are all-or-none — if any cue still lacks an end afterwards, all +// ends in the group are cleared. The input slice is never mutated. +// +// Exported because the Subsonic enhanced-lyrics serializer resolves cue ends +// per agent group while building the response; all other normalization is +// package-internal. +func NormalizeCueEnds(cues []Cue, fallbackEnd *int64) []Cue { + if len(cues) == 0 { + return cues + } + + out := slices.Clone(cues) + for i := range out { + end := out[i].End + if end == nil { + if i+1 < len(out) && out[i+1].Start != nil { + end = out[i+1].Start + } else { + end = fallbackEnd + } + } + if end != nil && i+1 < len(out) && out[i+1].Start != nil && *end > *out[i+1].Start { + end = out[i+1].Start + } + if end != nil && out[i].Start != nil && *end < *out[i].Start { + end = out[i].Start + } + out[i].End = gg.Clone(end) + } + + for i := range out { + if out[i].End == nil { + for j := range out { + out[j].End = nil + } + break + } + } + return out +} diff --git a/model/lyrics_normalize_test.go b/model/lyrics_normalize_test.go new file mode 100644 index 000000000..fc1ed0987 --- /dev/null +++ b/model/lyrics_normalize_test.go @@ -0,0 +1,33 @@ +package model + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("normalizeCueLines", func() { + It("should not mutate caller cue slices when filling missing cue end times", func() { + start0, start1, nextLineStart := int64(1000), int64(1500), int64(3000) + lines := []Line{ + { + Start: &start0, + Value: "Some lyrics", + Cue: []Cue{ + {Start: &start0, Value: "Some ", ByteStart: 0, ByteEnd: 4}, + {Start: &start1, Value: "lyrics", ByteStart: 5, ByteEnd: 10}, + }, + }, + { + Start: &nextLineStart, + Value: "Next line", + }, + } + + normalized := normalizeCueLines(lines) + + Expect(normalized[0].Cue[0].End).To(Equal(&start1)) + Expect(normalized[0].Cue[1].End).To(Equal(&nextLineStart)) + Expect(lines[0].Cue[0].End).To(BeNil()) + Expect(lines[0].Cue[1].End).To(BeNil()) + }) +}) diff --git a/model/lyrics_srt.go b/model/lyrics_srt.go index 28681c7cd..ff61d746b 100644 --- a/model/lyrics_srt.go +++ b/model/lyrics_srt.go @@ -34,7 +34,7 @@ func parseSRTWithLanguage(language string, contents []byte) (LyricList, error) { return nil, nil } - lyrics := NormalizeLyrics(Lyrics{ + lyrics := normalizeLyrics(Lyrics{ Lang: normalizeLyricLang(language), Line: lines, Synced: true, diff --git a/model/lyrics_test.go b/model/lyrics_test.go index 0a1df30cb..fd954ad26 100644 --- a/model/lyrics_test.go +++ b/model/lyrics_test.go @@ -5,33 +5,6 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("NormalizeCueLines", func() { - It("should not mutate caller cue slices when filling missing cue end times", func() { - start0, start1, nextLineStart := int64(1000), int64(1500), int64(3000) - lines := []Line{ - { - Start: &start0, - Value: "Some lyrics", - Cue: []Cue{ - {Start: &start0, Value: "Some ", ByteStart: 0, ByteEnd: 4}, - {Start: &start1, Value: "lyrics", ByteStart: 5, ByteEnd: 10}, - }, - }, - { - Start: &nextLineStart, - Value: "Next line", - }, - } - - normalized := NormalizeCueLines(lines) - - Expect(normalized[0].Cue[0].End).To(Equal(&start1)) - Expect(normalized[0].Cue[1].End).To(Equal(&nextLineStart)) - Expect(lines[0].Cue[0].End).To(BeNil()) - Expect(lines[0].Cue[1].End).To(BeNil()) - }) -}) - var _ = Describe("Lyrics.EffectiveKind", func() { It("defaults a blank kind to main", func() { Expect(Lyrics{}.EffectiveKind()).To(Equal(LyricKindMain)) diff --git a/model/lyrics_ttml.go b/model/lyrics_ttml.go index c700332a4..c82887f6f 100644 --- a/model/lyrics_ttml.go +++ b/model/lyrics_ttml.go @@ -199,7 +199,7 @@ func (p *ttmlParser) parseElement(start xml.StartElement, parent ttmlTimingConte if len(tokens) > 0 { parsedLine.Cue = tokens } - parsedLine = NormalizeLineTiming(parsedLine) + parsedLine = normalizeLineTiming(parsedLine) lineKey, _ := attrValue(start.Attr, "key") p.addMainLine(ctx.lang, lineKey, parsedLine) @@ -342,7 +342,7 @@ func (p *ttmlParser) parseMetadataText(start xml.StartElement, parent ttmlTiming if len(tokens) > 0 { line.Cue = tokens } - line = NormalizeLineTiming(line) + line = normalizeLineTiming(line) if line.Value == "" && len(line.Cue) == 0 { return ttmlMetadataEntry{}, false, nil @@ -630,7 +630,7 @@ func (p *ttmlParser) buildMetadataLyrics(kind string, langOrder []string, entrie endMs := *ref.line.End line.End = &endMs } - line = NormalizeLineTiming(line) + line = normalizeLineTiming(line) if line.Value == "" && len(line.Cue) == 0 { continue @@ -672,7 +672,7 @@ func (p *ttmlParser) buildMetadataLyrics(kind string, langOrder []string, entrie func (p *ttmlParser) finalizeLyrics(lyrics Lyrics) Lyrics { lyrics.Line, lyrics.Agents = p.resolveAgents(lyrics.Line) - return NormalizeLyrics(lyrics) + return normalizeLyrics(lyrics) } func (p *ttmlParser) resolveAgents(lines []Line) ([]Line, []Agent) {