Skip to content

Add support for multi-valued genre tag - #4751

Closed
jmbannon wants to merge 22 commits into
beetbox:masterfrom
jmbannon:jesse/multi-genre
Closed

Add support for multi-valued genre tag#4751
jmbannon wants to merge 22 commits into
beetbox:masterfrom
jmbannon:jesse/multi-genre

Conversation

@jmbannon

@jmbannon jmbannon commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

Description

#505

Adds support for setting the genre tag to be either a single or multi. Follow-up to #4743

And somewhat related to #4582 (comment), 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.)
  • Tests. (Encouraged but not strictly required.)

@Seqularise

Copy link
Copy Markdown

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

@jmbannon

Copy link
Copy Markdown
Contributor Author

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

@JOJ0

JOJ0 commented Apr 28, 2023

Copy link
Copy Markdown
Member

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?

@jmbannon

Copy link
Copy Markdown
Contributor Author

+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

@JOJ0

JOJ0 commented May 2, 2023

Copy link
Copy Markdown
Member

@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 JOJ0 changed the title Add support for multi genre tag Add support for multi-valued genre tag Sep 9, 2023
@JOJ0

JOJ0 commented Oct 20, 2023

Copy link
Copy Markdown
Member

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. :-)

@arsaboo

arsaboo commented Oct 20, 2023

Copy link
Copy Markdown
Contributor

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

@jmbannon

Copy link
Copy Markdown
Contributor Author

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

@stale

stale Bot commented Mar 17, 2024

Copy link
Copy Markdown

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 stale Bot added the stale label Mar 17, 2024
@JOJ0

JOJ0 commented Mar 17, 2024

Copy link
Copy Markdown
Member

keep open

@stale stale Bot removed the stale label Mar 17, 2024
@jmbannon

Copy link
Copy Markdown
Contributor Author

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

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.

4 participants