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

More tag variants for albumartists #47

Merged
merged 2 commits into from
Aug 20, 2022
Merged

Conversation

JuniorJPDJ
Copy link
Contributor

@JuniorJPDJ JuniorJPDJ commented May 13, 2021

Some fight about tag naming in Picard is still having place: https://tickets.metabrainz.org/browse/PICARD-700
Meanwhile different software using this tag uses other variants for it.
e.g. Kodi can read ALBUMARTISTS and ALBUM ARTISTS while jaudiotagger is tagging files using ALBUM_ARTISTS tag.

@JuniorJPDJ
Copy link
Contributor Author

Rebased on top of master and updated description.
Tbh. I need some feedback.

@sampsyo
Copy link
Member

sampsyo commented May 18, 2021

Argh; really too bad that people haven't standardized this. I'm not really sure what to do about the concatenated values from multiple lists—it does not seem good! Ideally, we would have the first non-empty list take precedence, and any remaining values would be ignored. But I don't really understand where the concatenation is coming from in the first place… tracking down the source of that issue seems like an important first step.

@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented May 21, 2021

This seems to come from there:

values.extend(style.get_list(mediafile.mgfile))

Anyway - I would leave this as a draft for now and wait a bit for Picard's.

@JuniorJPDJ
Copy link
Contributor Author

Ideally, we would have the first non-empty list take precedence, and any remaining values would be ignored.

Done ;)

@JuniorJPDJ
Copy link
Contributor Author

.

@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented Sep 19, 2021

Nothing is happening in Picard issue, so I think we should think about merging this one to maintain compatibility with all software ;/

What do you think about it? Does it need some changes?

Concatenated lists are fixed now ;)

@JuniorJPDJ JuniorJPDJ marked this pull request as ready for review September 26, 2021 19:03
@JuniorJPDJ
Copy link
Contributor Author

bump, can we merge this?

@sampsyo
Copy link
Member

sampsyo commented Apr 18, 2022

Sorry for letting this slip through the cracks!! Seems like we should head toward merging it. A couple of high-level challenging questions:

  • Do we understand the impact of the new "concatenation" policy for lists? I think we should look carefully at the other list-valued fields to make sure we aren't messing things up too badly for how those work. Maybe we should also dive back into the history to see why we originally made these values concatenated to see if there are specific fields that actually want that behavior.
  • These two new tag mappings for the field (ALBUM ARTISTS and ALBUM_ARTISTS) could potentially lead to more "junk" tags for average users. If we think either of these are somewhat uncommon, maybe it would make sense to mark one or both as read-only? Or maybe we don't need the one with the space at all if Kodi can use the "plain" one and jaudiotagger uses the underscore?

Adding a changelog entry would also be great!

@JuniorJPDJ
Copy link
Contributor Author

If we think either of these are somewhat uncommon, maybe it would make sense to mark one or both as read-only?

There's no actual standard for those. I'd go with marking the one with space as read-only as I personally don't like this one :D
I remember there was some software unable to read one of ALBUMARTISTS or ALBUM_ARTISTS and other not being able to read the second so I'd leave those 2 tags as RW. There's some discussion about this and guys mention only ALBUMARTISTS or ALBUM_ARTISTS.

Maybe we should also dive back into the history to see why we originally made these values concatenated to see if there are specific fields that actually want that behavior.

TBH. I can't even imagine where it could be useful and I found no regression when testing this. This was probably by mistake as it was easier to write it like this. This would cause some issues for re-reading files with multiple RW tags with the same data - cloning values. AFAIK we didn't before have any multi-tags with list values.

Based on Kodi and Jaikoz/Songkong
Now it returns first found tag like `MediaField` does.
@JuniorJPDJ
Copy link
Contributor Author

Fixed mentioned read_only properties and added changelog entities to commits

@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented May 22, 2022

I'd like also mention that readthedocs seems not to update automagically and seems to be not updated since looong time (v0.4.0).

@JuniorJPDJ
Copy link
Contributor Author

@sampsyo bump ;)

@JuniorJPDJ
Copy link
Contributor Author

bump! :D

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.

Sorry for the long delay on this; finally cycling back in. Looks good; merging now!

@sampsyo sampsyo merged commit b354a3d into beetbox:master Aug 20, 2022
sampsyo added a commit that referenced this pull request Aug 20, 2022
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