search

strawberrymusicplayer / strawberry

:strawberry: Strawberry Music Player
https://www.strawberrymusicplayer.org/
GNU General Public License v3.0
2.74k stars 192 forks source link

Strawberry's re-import of tag info is inconsistent (Rating vs. other fields) when enabling new option "Save ratings to song tags when possible" #820

Closed ronjouch closed 3 years ago

ronjouch commented 3 years ago

Describe the bug In-file rating changes (tag-based, mp3/id3v2 in my case), even if picked up by the library watcher, do not translate to showing the new rating in the playlist column/field.

To Reproduce

  1. Find an mp3 file with no rating, as shown both by the Edit Track Information screen and by the playlist column/field
  2. Add a rating with an external tag editor
  3. Wait for the Updating Collection watcher to do its job
  4. Read the rating shown in the Edit Track Information screen
  5. Add the track to a playlist and read its Rating column/field

Expected behavior

Actual behavior

System Information

Additional context This issue is a follow-up to https://github.com/strawberrymusicplayer/strawberry/issues/790#issuecomment-953060977 . In your last comment, @jonaski you wrote:

The scanner in Clementine might not have picked up on the change

, but as you see from the screencast, it did! And it would be annoying to have to do a full re-scan each time to work around this 😕.

Screencast:

https://user-images.githubusercontent.com/522085/139592718-28a4f27a-f638-4ccd-b447-7a1aa3f2b0ea.mp4

jonaski commented 3 years ago

There are no standardized rating tag. A different software might write rating differently, and use completely different values. I have no idea how the software you use write ratings to tags, it is closed-source and Windows only. Strawberry reads/writes the FMPS_RATING tag for most formats, it also converts rating from/to the POPM "Popularimeter" frame for ID3v2. I have tested setting rating for songs with KID3 both with FLAC (Vorbis comment tags) and with MP3 (ID3v2 tags) that did not have a rating from before, Strawberry correctly reads both. For MP3 I've tested that it reads both POPM and FMPS_Rating.

ronjouch commented 3 years ago

@jonaski thx for the extra info 🙂. I'll test with another tag editor and report back.

I remain surprised by the Actual Behavior I described, which look like a bug/inconsistency to me regardless of the ID3v2 tag being used.

Said differently: I got UI confirmation that the file did get re-read because I saw the "Updating Collection" status bar message, but then somehow the new rating wasn't read.

Actual behavior

  • Bullet point 4: the new rating does show in the Edit Track Information screen
  • Bullet point 5: the new rating does not show in the playlist's Rating field (even though bullet point 3 Updating Collection did happen!)
ronjouch commented 3 years ago

@jonaski alright! With more experimentation, here's a better-defined issue report: I think that, for users who opt in to the new Save ratings to song tags when possible, the current and still-new implementation is not what these users want.

The current implementation is inconsistent in its handling of Ratings updates compared to other database/tag fields. Indeed, on file change and for Ratings specifically, Strawberry give precedence to the Strawberry database, contrarily to what it does with other tags. I think it should instead give precedence to in-file tags, overriding existing in-database ratings on re-scan like it does for other tags.

Updated steps to reproduce

  1. Run Strawberry and enable the the new option Save ratings to song tags when possible
  2. Have a file with no ratings, neither POPM nor FMPS_Rating nor in-Clementine-database.
  3. Add a rating with an external tool: kid3-cli -c 'set Rating 1' -c 'set FMPS_Rating 0.2' -c 'set Title "externally set rating to 1"' '/your/file.mp3' && touch '/your/file.mp3' && sync → At that point, Strawberry will show Updating Collection. Opening the Edit Track Information will show the correct title, the correct rating (1/5), and adding the track to a playlist will too.
  4. Change the rating with an external tool: kid3-cli -c 'set Rating 64' -c 'set FMPS_Rating 0.4' -c 'set Title "externally set rating to 2"' '/your/file.mp3' && touch '/your/file.mp3' && sync → Strawberry will show Updating Collection. But then,

Expected

After Updating Collection, Strawberry should have updated its collection from in-file tag information:

Actual

After Updating Collection, Strawberry only partially updates its database from in-file tag information:

Why I think the current implementation is undesirable

  1. Users who decide to opt in to the new Save ratings to song tags when possible option expect tags to be their source of truth, all of them. By honoring this contract for some tags (like Title), but not for Rating, Strawberry is surprising / inconsistent to these users.
  2. As an extra clue that the alternative behavior I suggest is reasonable: it is the behavior implemented by Clementine and foobar2000. In these players, when a file is detected as changed, it is re-imported and in-database information is overwritten for all fields including Rating, overriding existing in-db Rating info.

Commenters on issue #790 @bartdbx @foss- @wadbr: do you agree the current implementation is surprising for us using the new option Save ratings to song tags when possible? Do you agree that users who enable this new option expect the alternative behavior I'm describing?

jonaski commented 3 years ago

I watched your video again and realize I missed that the rating was showing the edit tag dialog. If the song is unrated, it should update it with the rating from the tag when the file is detected as modified. However, I can't reproduce any issues related to that. But it is intentional that the rating should not be re-read from the file tags if the song is already rated as that would overwrite the database rating for users who do not save rating to file.

ronjouch commented 3 years ago

it is intentional that the rating should not be re-read from the file tags if the song is already rated as that would overwrite the database rating for users who do not save rating to file.

@jonaski I hope you reconsider this decision.

jonaski commented 3 years ago

I can see how that is relevant if you use another program to edit ratings, but we definitely don't want that when the save rating option is off as that could possible overwrite database rating in cases where files have older ratings. Shouldn't be to hard to add an option for it that will be independent of the save rating option.

ronjouch commented 3 years ago

I can see how that is relevant if you use another program to edit ratings, but we definitely don't want that when the save rating option is off as that could possible overwrite database rating in cases where files have older ratings. Shouldn't be to hard to add an option for it.

That's my point; I 100% agree the behavior I expect is only to apply when the save rating option is on! But then, why would we need a new option?

jonaski commented 3 years ago

For users who don't use the save rating option, but still want to use another program to edit ratings, so there should be an option to make Strawberry re-reread ratings.

ronjouch commented 3 years ago

Okay, if you feel there's an audience for that. Also, maybe the three users I pinged will add some data points helping decide what's right/necessary.

At any rate, thaaaaanks for listening and thanks for the fast feedback! I'm keeping an eye on this issue and will re-test if the changed behavior (or extra option) lands. Cheers.

jonaski commented 3 years ago

Added a setting to the collection settings: "Overwrite database rating when songs are re-read from disk". It's off by default so you need to enable it. https://github.com/strawberrymusicplayer/strawberry/commit/ca10920bb546ea3c15fde7c0e4ae86a78fa2f47a Try it

ronjouch commented 3 years ago

Added a setting to the collection settings: "Overwrite database rating when songs are re-read from disk". It's off by default so you need to enable it. ca10920 Try it

@jonaski awesome! ca10920bb546ea3c15fde7c0e4ae86a78fa2f47a builds, I was able to turn the option on, and it works as expected. Closing; with that I'm now able to try Strawberry as my daily driver and look at sponsoring. Thanks for listening.