lyrics: Fallback to plain lyrics if synced lyrics not available #5089
Conversation
| @@ -696,13 +696,27 @@ def test_fetch_synced_lyrics(self, mock_get): | |||
| mock_get.return_value.json.return_value = mock_response | |||
| mock_get.return_value.status_code = 200 | |||
|
|
|||
| self.plugin.config["synced"] = False | |||
There was a problem hiding this comment.
I don't know if it's a problem with my setup, but I had to add these lines to the already existing tests to have them pass. It looks like the plugin config is mutated in one test case and then reused in the next, even though setUp() is called before each test which creates a new plugin instance and takes the config from there. But maybe I'm missing something.
If it is a setup (or logic) issue on my part, then having config["synced"] = False explicitly stated at least makes it clear what the expected configuration is for that part of the test. :)
There was a problem hiding this comment.
Can you create an issue about this? Just so it doesn't get forgotten and can be referenced.
|
This fix is fine while there is only one source for synced lyrics. When more sources with synced lyric support are added, it will require a more complex solution for the fallback to work properly. Should I create an issue to keep track of this? |
|
Hi! Thanks for the PR. Is there a reason that the more advanced logic can't be included in this PR? Being able to deal with multiple sources seems like a fairly close continuation of this work. |
|
I agree, but I wanted to have this quick PR out, so that others can also benefit from it, while the more complex solution requires, IMO, some more in-depth discussion about the approach and the algorithm itself. I want it to be approved by the project people before I spend hours working on it. |
|
Let's get this merged in, since I doubt that #5406 is going to get merged anytime soon. It looks good - just needs rebasing off latest master and a small changelog note before merging |
edgars-supe
force-pushed
the
synced-lyrics-fallback
branch
from
1adec91 to
05f53ac
Compare
edgars-supe
force-pushed
the
synced-lyrics-fallback
branch
from
05f53ac to
0936025
Compare
|
@snejus I've rebased, and updated the changelog. :) |
Description
The
syncedconfig flag was not working the way the docs described it for the LRCLIB source. Withsynced: yes, if a track does not have synced lyrics, but does have plain lyrics, the plugin would return no lyrics. The docs imply that, with the flag enabled, it would still use plain lyrics if there are no synced lyrics.LRCLIB API call that returns plain lyrics can be found here.
To Do
Documentation