jacquesh
Bulk search failing to respect "Marked as Instrumental" flag
When I click "Mark as Instrumental" on a track or a group of tracks, it fails with the message Finished marking 0 tracks as instrumental with X failures, where X is a seemingly random number (-579785234, 1362716963, 1483497535, etc).
Steps to reproduce
- Install OpenLyrics v1.12
- RIght click on a file -> Mark as Instrumental
- View the console
Expected behavior
Songs should be properly marked as Instrumental.
Versions
- foobar2000 version: 2.24.6
- OpenLyrics version: 1.12
Debug logs
INFO-OpenLyrics: Marking 1 tracks as instrumental from the context menu...
INFO-OpenLyrics: Searching for lyrics for artist='ConcernedApe', album='Stardew Valley: Original Soundtrack', title='Stardew Valley Overture'...
INFO-OpenLyrics: Save file name format '$replace([%artist% - ][%title%],/,-,\,-,<,-,>,-,:,-,",-,|,-,?,-,*,-)' with directory class 'ConfigDirectory' evaluated to 'file://C:\Users\User\AppData\Roaming\foobar2000-v2\lyrics\ConcernedApe - Stardew Valley Overture'
INFO-OpenLyrics: Querying for lyrics in file://C:\Users\User\AppData\Roaming\foobar2000-v2\lyrics\ConcernedApe - Stardew Valley Overture.lrc...
INFO-OpenLyrics: Querying for lyrics in file://C:\Users\User\AppData\Roaming\foobar2000-v2\lyrics\ConcernedApe - Stardew Valley Overture.txt...
INFO-OpenLyrics: Found 0 lyrics in local files: file://C:\Users\User\AppData\Roaming\foobar2000-v2\lyrics\ConcernedApe - Stardew Valley Overture
INFO-OpenLyrics: Failed to retrieve lyrics from source: Local files
INFO-OpenLyrics: Searching for lyrics in tag: 'UNSYNCED LYRICS'
INFO-OpenLyrics: Searching for lyrics in tag: 'LYRICS'
INFO-OpenLyrics: Searching for lyrics in tag: 'SYNCEDLYRICS'
INFO-OpenLyrics: Searching for lyrics in tag: 'UNSYNCEDLYRICS'
INFO-OpenLyrics: Failed to retrieve lyrics from source: Metadata tags
INFO-OpenLyrics: Current search is only considering local sources and LRCLIB is not marked as local, skipping...
INFO-OpenLyrics: Parsing lyrics text...
INFO-OpenLyrics: Parsing LRC lyric text...
INFO-OpenLyrics: Lyric loading complete
INFO-OpenLyrics: Finished marking 0 tracks as instrumental with -691786274 failures
Additional information
Looks like the logging statement isn't properly formatted: https://github.com/jacquesh/foo_openlyrics/commit/48b6808aedd738fcd16b6071c680e1a9669567e3, however I don't think that's the only issue I'm having, as if I mark songs as instrumental then go and bulk search for lyrics, the instrumental songs are not skipped as they should be
Looks like the logging statement isn't properly formatted: https://github.com/jacquesh/foo_openlyrics/commit/48b6808aedd738fcd16b6071c680e1a9669567e3, however I don't think that's the only issue I'm having, as if I mark songs as instrumental then go and bulk search for lyrics, the instrumental songs are not skipped as they should be
Those are two different things. Songs failing to be marked as instrumental for some unknown reason would (almost certainly) be a bug. Bulk search disregarding the instrumental flag isn't necessarily a bug. I haven't checked yet but it wouldn't surprise me if disregarding search avoidance was an intentional decision (I guess at least in the code modified in your PR its not clearly documented so...presumably not for that specific case). In general we've struggled back and forth more than once on how we should think about bulk search by comparison to the manual- and auto-searches.
I think we can safely assume that auto-search should respect the instrumental flag and avoid searches accordingly. That's the point of it - instrumental tracks by definition do not have lyrics and so we should not search for lyrics for those tracks. For manual search it was previously not clear because we did not always distinguish between "don't search for this track because it always fails" and "don't search for this track because its instrumental", but for manual search the user is very specifically requesting a searching for lyrics for this track, so we should probably just search. Bulk search is the weird in between and I guess we kinda erred on the side of treating it more like the "it always fails" case and so if you're requesting a search then maybe we should just search anyway (maybe new lyrics have been added for an entire new album or whatever).
Now that we have the separate flag, it probably does make sense to treat the instrumental flag as a stronger signal that searching for this track is pointless. All of which is a lot of words to say:
not skipped as they should be
Yes, I think we can conclude that you are correct that they should be skipped in this case, but from the outset this was not obvious, given the historical context above.
So there are 2 problems here:
- The logging code around mark-instrumental is wrong, as remedied in your PR
- We should add a check for instrumental to the bulk-search, document it with at least the relevant info above and the resulting reasoning, and see if we can add some form of unit test for this so that future logic changes do not accidentally change this behaviour
But seemingly there isn't an actual problem with the "mark items as instrumental" itself?
Yes, there is no error in marking the items as instrumental, just in the handling of it for bulk searches (in my opinion)