diff --git a/beetsplug/_typing.py b/beetsplug/_typing.py index 6c69aeed41..39e0379aa5 100644 --- a/beetsplug/_typing.py +++ b/beetsplug/_typing.py @@ -18,7 +18,7 @@ class Item(TypedDict): albumName: str duration: float | None instrumental: bool - plainLyrics: str + plainLyrics: str | None syncedLyrics: str | None diff --git a/beetsplug/lyrics.py b/beetsplug/lyrics.py index c9e5c529d9..61cf58011f 100644 --- a/beetsplug/lyrics.py +++ b/beetsplug/lyrics.py @@ -260,7 +260,7 @@ class LRCLyrics: id: int duration: float instrumental: bool - plain: str + plain: str | None synced: str | None def __le__(self, other: LRCLyrics) -> bool: @@ -308,13 +308,16 @@ def duration_dist(self) -> float: @cached_property def is_valid(self) -> bool: """Return whether the lyrics item is valid. + Lyrics duration must be within the tolerance defined by - :attr:`DURATION_DIFF_TOLERANCE`. + :attr:`DURATION_DIFF_TOLERANCE`, and the item must carry some usable + text: LRCLib serves records that have neither plain nor synced lyrics + without flagging them as instrumental, and those match nothing. """ return ( self.duration_dist <= self.target_duration * self.DURATION_DIFF_TOLERANCE - ) + ) and bool(self.instrumental or self.plain or self.synced) @cached_property def dist(self) -> tuple[bool, float]: @@ -329,15 +332,29 @@ def dist(self) -> tuple[bool, float]: """ return not self.synced, self.duration_dist + @staticmethod + def remove_timestamps(synced: str) -> str: + """Return synced lyrics with their LRC timestamps removed.""" + return "\n".join( + m[2] + for line in synced.splitlines() + if (m := Lyrics.LINE_PARTS_PAT.match(line)) + ) + def get_text(self, want_synced: bool) -> str: """Return the preferred text form for this candidate.""" if self.instrumental: return INSTRUMENTAL_LYRICS - if want_synced and self.synced: - return "\n".join(map(str.strip, self.synced.splitlines())) + if self.synced: + if want_synced: + return "\n".join(map(str.strip, self.synced.splitlines())) + + # A record may carry synced lyrics without the plain variant. + if not self.plain: + return self.remove_timestamps(self.synced) - return self.plain + return self.plain or "" class LRCLib(Backend): diff --git a/docs/changelog.rst b/docs/changelog.rst index 4d98aa214f..cb4b0d8d2d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -38,6 +38,14 @@ Bug fixes - :doc:`plugins/lyrics`: ``beet lyrics`` no longer crashes with an ``AttributeError`` on tracks that have no stored lyrics when ``force`` is enabled; a missing lyrics body is now treated as empty text. :bug:`6860` +- :doc:`plugins/lyrics`: LRCLib serves records that carry neither plain nor + synced lyrics without flagging them as instrumental. Such a record was + accepted as a match and produced lyrics whose body was ``None``, crashing the + importer with ``AttributeError: 'NoneType' object has no attribute + 'splitlines'``. These records are now rejected as matches, so the search falls + through to the remaining candidates. Relatedly, a record that carries only + synced lyrics now yields the plain variant with timestamps removed, rather + than nothing, when ``synced`` is disabled. :bug:`6888` - Flexible attributes whose names contain uppercase characters (for example ``beet import --set Tag_With_Uppercase=true``) can now be found by queries. Field names are lowercased when a query is parsed, so such attributes could diff --git a/test/plugins/lyrics_pages.py b/test/plugins/lyrics_pages.py index 09760ba568..df230709f2 100644 --- a/test/plugins/lyrics_pages.py +++ b/test/plugins/lyrics_pages.py @@ -278,7 +278,7 @@ def backend(self) -> str: url_title="Lady Madonna - The Beatles - LETRAS.MUS.BR", ), LyricsPage.make( - "https://lrclib.net/api/get/19648857", + "https://lrclib.net/api/get/23863037", """ [00:08.35] Lady Madonna, children at your feet [00:12.85] Wonder how you manage to make ends meet diff --git a/test/plugins/test_lyrics.py b/test/plugins/test_lyrics.py index e6384c4a34..fb0939acab 100644 --- a/test/plugins/test_lyrics.py +++ b/test/plugins/test_lyrics.py @@ -618,6 +618,20 @@ def test_synced_config_option( assert lyrics.text == expected_lyrics assert lyrics.backend == backend_name + @pytest.mark.parametrize( + "response_data", [[lyrics_match(plainLyrics=None)]] + ) + @pytest.mark.parametrize("plugin_config", [{"synced": False}]) + def test_plain_lyrics_derived_from_synced(self, fetch_lyrics): + """Derive plain lyrics from the synced text when the record omits them. + + Returning nothing here would hide lyrics the match actually carries. + """ + lyrics = fetch_lyrics() + + assert lyrics + assert lyrics.text == "synced" + @pytest.mark.parametrize( "response_data, expected_lyrics", [ @@ -629,6 +643,11 @@ def test_synced_config_option( pytest.param( [lyrics_match(instrumental=True)], "", id="instrumental track" ), + pytest.param( + [lyrics_match(plainLyrics=None, syncedLyrics=None)], + None, + id="none: no lyrics and not flagged instrumental", + ), pytest.param( [lyrics_match(syncedLyrics=None)], "plain",