jacquesh / foo_openlyrics

An open-source lyric display panel for foobar2000
MIT License
450 stars 26 forks source link

Ability to set Autosave Tag to Disabled #18

Closed TiredRobot closed 3 years ago

TiredRobot commented 3 years ago

Title is self-explanatory I think.

jacquesh commented 3 years ago

The title is indeed mostly self-explanatory but just to clarify: Do you mean that you want the chosen save method to be "save to tag" but disable autosaving completely? If that is not what you mean then the rest of this comment will not be very helpful so please do correct me!

Can you give me a little more background on why this would be useful to you? With that setup OpenLyrics would search every time you played the track (probably returning exactly the same lyrics each time) until you saved something and then it would search every time on sources configured to run before the if the Tags source.

I have so far avoided allowing users to disable auto-save entirely because I think it leaves the possibility that a significant number of people re-request the same lyrics from the same websites over and over again which is just a lot of unnecessary traffic for those websites to have to serve. I've also considered enforcing that the save source is the first source searched (as this would alleviate the issue a little and is I expect everybody wants anyway).

TiredRobot commented 3 years ago

Do you mean that you want the chosen save method to be "save to tag" but disable autosaving completely?

Yeah.

I think you can pretty much avoid the problem of unnecessary traffic by not defaulting it to Disabled. The vast majority of users wouldn't change it, I don't think. It's just that I don't want the player to modify any tags without me explicitly clicking Save.

jacquesh commented 3 years ago

Fair enough about the default covering the majority case. It's entirely possible that my concern in this area is unfounded. I'll think on it at least.

TiredRobot commented 3 years ago

When the user selects Disabled, you could have a warning pop up reminding users that lyrics are gonna be fetched online every time the song is played unless they manually save.

Maybe you could have a small but visible "Save to tag/file" button somewhere on the panel as well but I don't think this is necessary.

elandorr commented 3 years ago

Replying to what you wrote in #31:

Sorry for the duplicate, didn't find this thread.

tl;dr I want to see what was found before I save it.

Basically what I already said:

But for the love of all that is holy to collectors: Can you please add an option to NEVER autosave? Creating files or editing tags should always be on-demand only. It messes up hashes.

The original component does this perfectly, you can save whenever you want to, but still see the current results.

I wouldn't randomly let online sources mess with my files. Especially if your musical taste is slightly less mainstream, many lyrics are simply wrong/bad/not matching the exact release. Even the 'big' commercial tools that try to match the exact release based on their private dbs are wrong more often than not. Releases that are not even on discogs are guaranteed to lead to chaos as well.

Before saving anything I always verify and checksum afterwards. This keeps the collection in a controlled state at all times. Once you get it messy it's a gigantic effort to deal with it, so I have a manual-first approach. It's a lot faster than the automated tools out there in the long run: I deal with it once. Also applies to the discogs link in the tags. If you ever get the wrong reference you'll have to dig through every release in the future to fix it, and life's too short for that.

My usual 'workflow' with the original component was:

If it's not close at all, ignore the result. When time and mood strike right, write it myself, or try to find it elsewhere online for pasting.

I don't like random files around, so saving like that's not an option. And if I were, it'd represent the same issues. The tags are the perfect place if you use the fb2k feature to optimize the layout. It places the meta in front, so it's as efficient as it can be.

Traffic is not really an issue. The little bit of text generates practically zero traffic. Literally just visiting a single modern webpage will load orders of magnitude more data than you could generate in a month in lyrics text, most of which is even marketing trash. (That'll depend on how you search though of course. I didn't check if you use the API or scrape the entire site.)

An option to only manually search online sources would help minimize that too. I use the original lyrics panel offline already, as there are hardly any online sources. The most useful LyricWiki had a lot of user-contributed lyrics, but they got killed recently. And formatting was broken anyway. A way for users to create custom searches would be nice, although I have to admit I don't even know a single other lyrics website right now.

In general the online search is really more a 'nice if it works and I'm lazy'. If it finds the right one I can hit save, or just ignore it. A manual search for sake of saving the target server's traffic sounds quite reasonable. And/or a toggle in the context menu maybe, so you can flip it on when you're tagging, and keep it local only for regular use.

jacquesh commented 3 years ago

@elandorr Thanks for the long explanation! That makes sense, it's just very different to my own usage.

You're right about the small amount of traffic, but I thought I'd err on the side of caution. I'll probably go with @TiredRobot's suggestion of adding the option but putting a warning dialog when you select it asking if you're sure that's what you want (because if nothing else, searching the internet every time takes longer than loading from local files).

Manual searching is certainly something I'm going to add (in the relatively near future). See #29 :)

jacquesh commented 3 years ago

Woops, forgot to tag the issue in the follow-up commit. There is now a "Never" option for auto-save, along with a manual save button in the panel's context menu. :)

elandorr commented 3 years ago

Thank you very much! That was quick. Would you mind publishing a build? VS now forces a login even for the CE, and it's also mindbogglingly huge.

jacquesh commented 3 years ago

@elandorr Done :)

elandorr commented 3 years ago

Thank you @jacquesh!

I hate to bother you again so much, but:

I don't want to pressure you or sound demanding, just happened to notice and see your post.

jacquesh commented 3 years ago

Hmmm not saving when you specifically click 'Save Lyrics' sounds like a bug. Woops, sorry about that!

Regarding the editor save behaviour: When you put it like that, yeah it sounds a bit silly doesn't it? >_< I shall have to take a look because I'm pretty sure there was a sensible reason for that change I just don't remember exactly what it was off the top of my head (probably somethingsomething consistent behaviour somethingsomething)

jacquesh commented 3 years ago

@elandorr I'm struggling to reproduce your failure to save lyrics. I've tried saving to file and to tags using exactly the steps you listed above and it always works for me =/ I'm going to add some more "verbose" logging at some point, but for now could you check your console output (foobar menu bar -> View -> Console) and see if there is anything from OpenLyrics? Any lines that start with ERROR-OpenLyrics: ... or anything like that? Is it possible that the files you're trying to save to (which is the audio files themselves if you're saving to tags) are read-only?

anazzani commented 3 years ago

Apologies for spamming this thread, but I have a small issue that I think might be relevant.

I just discovered this plugin and it's working great (I only had to dig a bit to find the "additional" preferences).

I configured it to save to file (I also prefer not having my music files modified automatically): my problem is that the plugin creates a new file even if the "source" of the lyrics is a tag of the playing file, which I find a bit pointless.

Would it be possible to add an option to save ONLY lyrics from external sources?

Thanks.

jacquesh commented 3 years ago

@anazzani Sure, I've created a separate bug for that

@elandorr I'm closing this for inactivity because I still can't reproduce your problem. If you encounter it again in future and have more details, please open a new issue.