beets icon indicating copy to clipboard operation
beets copied to clipboard

Add support for multi-valued genre tag

Open jmbannon opened this issue 2 years ago • 8 comments

Description

https://github.com/beetbox/beets/issues/505

Adds support for setting the genre tag to be either a single or multi. Follow-up to https://github.com/beetbox/beets/pull/4743

And somewhat related to https://github.com/beetbox/beets/pull/4582#issuecomment-1445023493, when writing unit tests, I noticed that tags that have separate names for both their single and multi variant in Mediafile (albumtype/albumtypes, genre/genres), there was conflicts on which took precedence when writing the tag.

This PR addresses that via introducing mapping them with the _single_to_multi_fields dict, and writing case-logic to decide what to write when given differing single/multi values

To Do

  • [ ] Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • [ ] Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • [x] Tests. (Encouraged but not strictly required.)

jmbannon avatar Apr 11 '23 07:04 jmbannon

Hi, thanks for your work, i want to use this with lastgenre and vorbis\id3v2.4 tags, how do i proceed? Simply compiling from your branch and running lastgenre doesn't work

Seqularise avatar Apr 13 '23 09:04 Seqularise

Hi, thanks for your work, i want to use this with lastgenre and vorbis\id3v2.4 tags, how do i proceed? Simply compiling from your branch and running lastgenre doesn't work

More work needs to be done to integrate lastgenre with multi-tags. That plugin is written on the assumption of single tags

jmbannon avatar Apr 13 '23 15:04 jmbannon

Hi, thanks for your work, i want to use this with lastgenre and vorbis\id3v2.4 tags, how do i proceed? Simply compiling from your branch and running lastgenre doesn't work

+1 Currently lastgenre also has an issue when multiple genres (genre1, genre2, ...) are used with detecting that genres in such a comma-delimited list are actually valid arleady and should not be overwritten. It seems to compare the whole string. That is (genre1, genre2) would overwrite (genre2, genre3). Does that make sense?

JOJ0 avatar Apr 28 '23 22:04 JOJ0

+1 Currently lastgenre also has an issue when multiple genres (genre1, genre2, ...) are used with detecting that genres in such a comma-delimited list are actually valid arleady and should not be overwritten. It seems to compare the whole string. That is (genre1, genre2) would overwrite (genre2, genre3). Does that make sense?

Sounds like it naively overwrites it with whatever the current genre is versus doing a merge (i.e. genre1, genre2, genre3), is that right?

The genres multi-tag points to the single-tag form. So once it's technically a multi in the database, it should be much easier to do a merge since you're dealing with a list versus a delimited string.

I think lastgenre would also need support to either use multis are continue as delimited, I'm sure many downstream apps depend on that form

jmbannon avatar Apr 28 '23 23:04 jmbannon

@jmbannon after putting some debug prints into the validation function in the lastgenre plugin, my assumption proved right. This should make clear what is happening. Trying to run on one track (set in configuration to track mode) and matching for track, not album (-A) outputs this:

$ beet lastgenre -A klute lie cheat overchoice
LastGenrePlugin._is_allowed got genre
Glitch, Techno
and decided it's NOT a valid genre.

LastGenrePlugin._is_allowed got genre
techno
and decided it's a valid genre.

LastGenrePlugin._is_allowed got genre
glitch
and decided it's a valid genre.

LastGenrePlugin._is_allowed got genre
electronica
and decided it's a valid genre.

LastGenrePlugin._is_allowed got genre
glitch
and decided it's a valid genre.

LastGenrePlugin._is_allowed got genre
techno
and decided it's a valid genre.

LastGenrePlugin._is_allowed got genre
electronica
and decided it's a valid genre.

Prior to the lastgenre run, genre was "Glitch, Techno". It seems the plugin does not divide the current genre into single entities before checking.

count is set to 2.

So the actual bug in short is: force: no in the lastgenre plugin is broken. It will always overwrite if count is set to greater than 1.

JOJ0 avatar May 02 '23 11:05 JOJ0

Is there anything left to do here @jmbannon except fixing the conflicts? I am asking since in a couple of days a "black formatter thing" will be merged and will most probably produce a hell lot of more conflicts. So just giving a heads up. If we are quick we could get this one through before that. :-)

JOJ0 avatar Oct 20 '23 12:10 JOJ0

I would love this feature to be added and will be happy to test things out once the docs are added.

arsaboo avatar Oct 20 '23 14:10 arsaboo

This branch needs a lot of work. I haven't forgotten about it, have just been super busy. We could probably close it and I'll make a new one that's rebased on latest master when I get to it

jmbannon avatar Oct 20 '23 14:10 jmbannon

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 17 '24 13:03 stale[bot]

keep open

JOJ0 avatar Mar 17 '24 13:03 JOJ0

It's probably okay to close. I will reopen it properly rebased off master when I get the time - haven't forgotten about it :slightly_smiling_face:

jmbannon avatar Mar 19 '24 05:03 jmbannon