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 LRCLIB as a provider for the lyrics plugin #4989

Merged
merged 2 commits into from
Nov 5, 2023

Conversation

mumumumu
Copy link
Contributor

@mumumumu mumumumu commented Nov 5, 2023

Description

This PR adds support for LRCLIB as a lyrics provider.

I recently set up Navidrome on my home server and the web UI for that only supports embedded, synced lyrics. From what I can tell, the current lyric providers only return plain lyrics. I did a little research and found LRCLIB, which offers an easy to use API that includes both synced and plain lyrics. Note that all four parameters (track, artist, album, duration) to the /get endpoint are required.

Inspired by tranxuanthang/lrcget#2 (comment).

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Wow; this looks really great! I've reviewed the code carefully and everything looks good from here.

The one high-level question I have is: Do we want to have this source enabled by default, given that it returns synced lyrics by default? To be specific, "synced" lyrics from LRCLIB look like this:

[00:09.17] Come back from San Francisco
[00:12.97] It can't be all that pretty
[00:16.75] When all of New York City misses you
...

…which may be surprising to most users of the plugin, who (from other sources) are expecting plain "unsynced" lyrics.

One option could be to add a new top-level configuration option to the plugin, synced. This would prefer synced lyrics for any source that provides them (currently just this new source), and otherwise would only try to get plain lyrics. Would this make sense?

@mumumumu mumumumu force-pushed the lyrics-pluging-lrclib branch 2 times, most recently from 9d295f8 to 973a57c Compare November 5, 2023 16:04
@mumumumu
Copy link
Contributor Author

mumumumu commented Nov 5, 2023

@sampsyo thanks for the quick review, and that makes total sense. Added a synced option and updated the plugin docs.

@mumumumu mumumumu requested a review from sampsyo November 5, 2023 16:21
@mumumumu mumumumu force-pushed the lyrics-pluging-lrclib branch from 973a57c to 84259b1 Compare November 5, 2023 16:23
@mumumumu mumumumu force-pushed the lyrics-pluging-lrclib branch from 84259b1 to 7b0f5fb Compare November 5, 2023 16:26
Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Wonderful; this looks great! Thanks again for the awesome work on this.

@sampsyo sampsyo merged commit 708a04d into beetbox:master Nov 5, 2023
13 checks passed
@mumumumu mumumumu deleted the lyrics-pluging-lrclib branch November 5, 2023 22:00
@jackwilsdon jackwilsdon mentioned this pull request Feb 15, 2024
3 tasks
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