Implement multi-value genres tag

[ibrokemypie]

Description

Resurrects #4751. Rebased just the genre field changes on to master.

"Adds support for setting the genre tag to be either a single or multi."

To Do

• Documentation.(not required / no place to put this except the changelog)
• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[github-actions]

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

[kergoth]

That makes sense, thanks for the clarification!

…

On Friday, September 20, 2024, Rylee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/test_library.py <#5426 (comment)>: > self._assert_dest(b"/base/Pop") def test_first_skip(self): - self.i.genres = "Pop; Rock; Classical Crossover" - self._setf("%first{$genres,1,2}") + self.i.sub_genres = "Pop; Rock; Classical Crossover" + self._setf("%first{$sub_genres,1,2}") The tests are testing the %first{} function extracting from a string split on ; . Genres was the tag used for the tests initially, but now genres is a MULTI_VALUE_DSV which is separated by null characters instead. Rather than rewriting the tests both to the full %first{text,count,skip,sep,join} format to specify the null character separator, which would be changing whats being tested, the original author just swapped out the genres tag for a nonexistent sub_genres tag which would continue to work as a standard string for %first to act on. — Reply to this email directly, view it on GitHub <#5426 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACJFY5YIKQOCKXT67PTHLZXTKQRAVCNFSM6AAAAABOPR74YGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJZGYZDIOBSHE> . You are receiving this because you commented.Message ID: ***@***.***>

-- Christopher Larson ***@***.***, ***@***.*** Principal Software Engineer, Embedded Linux Solutions, Siemens Digital Industries Software

[bal-e]

This is a nice and tiny PR, thanks @ibrokemypie!

[bal-e]

Can you expand a bit here, maybe with an example that shows why it's useful? The point of such a tag should be made obvious to end users scrolling through.

[JOJ0]

Suggested change

	* New multi-value `genres` tag
	* New multi-valued ``genres`` tag. This change brings up the ``genres`` tag to the same state as the ``*artists*`` multi-valued tags (see :bug:`4743` for details).
	:bug: `5426`

[JOJ0]

@bal-e In my initial version I even had "There is not much use-cases for this yet....", but I kicked it out again. You fine with a simple changelog like that?

[JOJ0]

@ibrokemypie have a look at my changelog suggestion and please rebase on-to master.

To all involved (@kergoth, @snejus @bal-e), am I seeing right that this is minimal and can be safely merged?

It probably is not of much use yet, but brings genres up to speed with artists, albumartists and so on tags (#4743). At least it would pave the way for future experiments

[ibrokemypie]

@ibrokemypie have a look at my changelog suggestion and please rebase on-to master.

To all involved (@kergoth, @snejus @bal-e), am I seeing right that this is minimal and can be safely merged?

It probably is not of much use yet, but brings genres up to speed with artists, albumartists and so on tags (#4743). At least it would pave the way for future experiments

Done and Done.

[snejus]

Let me test this - I have a feeling this will suffer from the same issue like #5045 so we'd need one additional small change to address it

[snejus]

Unfortunately this change is a bit less innocent than it may seem. From what I've this does not interact very well with the pre-existing genre field. The main issues:

1. The pre-existing genre field is now ignored. Running beet write with an (initially) empty genres field:

     genres: pop, rock, stage & screen -> 
   

   This means genre gets removed from all files in the library.

2. However once the above happens running beet write again shows:

     genre:  -> pop, rock, stage & screen
   

   However, nothing happens, since genre field is ignored when it comes to writing the file.

This fake change will always show up if genre in the file is not in line with genre field in the database. This is because both genre and genres fields read the same metadata in the file, so this will always show up if they are not in sync.

This is the same issue as we've seen with other fields that @JOJ0 mentioned above. See issues below for more context

• Lastgenre changes other tags than only GENRE #5443
• beet write: false positive for changes in fields with multiple values #5371
• beet write continuously writes mb_artistid and mb_albumartistid (1.7.0_pre20240501) #5265
• beets cannot write the albumtype tags to the file metadata #5045
• beets always want to write mb_artistid and mb_albumartistid tags #5043
• Continued retagging of the albumtype field (and potentially other fields) #4715

We've got a hack for this issue which synchronises values between the two fields, however it only does so when the item is re-imported / autotagged. Before then, genres in music files will get cleared on write.

This makes me think we need a more appropriate solution here. I don't think we have a reason to keep both singular and plural fields in place (except for backwards-compatibility), and we've seen how much headache this has been causing us with other fields (albumtype/albumtypes, artist/artists etc.).

I want to keep a single source of truth (don't care whether it's named genre or genres, but most probably genre to make this backwards-compatible), so I suggest we adjust the ..._DSV type implementation to handle both list and str inputs, where the latter is split by the separator if needed. The field will always return a list on access.

Does this need to have the MULTI_VALUE_DSV type which uses the funny \␀ separator? Or could it use the more straightforward SEMICOLON_SPACE_DSV type, which is only used by albumtypes?

[JOJ0]

Thanks for your detailed analysis @snejus I will read it later in detail. In my opinion a lot of headache is also caused because mediafile, i.e. on the lowest level genre and genres interact with each other (exactly the same as albumtypes and albumtype.

Let me test this - I have a feeling this will suffer from the same issue like #5045 so we'd need one additional small change to address it

Would a change like this allow to save genre and fix or improve our situation: beetbox/mediafile#81

Hypothetically for now BUT if we see that it solves our problems, I would want to vote and investigate why a low-level "magic" ever was needed and if we can get rid of it. Even though it might be a breaking change, I honestly don't see the point anymore why beets should constantly work around this issue. Mediafile is used by a few other projects, well I actually only know of only one and we can get in contact.

[snejus]

@JOJ0 the above encourages duplication, and I imagine it would suffer from the same issue since this way both genre and genres fields still point at the same field, except at the lower (file) level.

[snejus]

I was thinking about something like this: #5606

[JOJ0]

@JOJ0 the above encourages duplication, and I imagine it would suffer from the same issue since this way both genre and genres fields still point at the same field, except at the lower (file) level.

That is true. It encourages believing genre and genres attribute is something different, which then it is not. Actually I tried to find out what the "official" name of the low-level tag is and it took me longer than I thought. Maybe I opened that draft idea over there only to "make my tagging 101 homework. Maybe something you didn't know yet in there, don't know...: beetbox/mediafile#81 (comment)

And BTW maybe the code in the PR is also misleading, I kind of had the idea that maybe we can make 2 differnt tags with 2 differnt things....at least at the low-level. But that turns out to be not possible or useful anyway, since id3 only defines one tag for genre (actually content type) and that technical name is TCON

Anyway, not really helpful, will better read your code now! ;-) Thanks!

[JOJ0]

Unfortunately this change is a bit less innocent than it may seem. From what I've this does not interact very well with the pre-existing genre field. The main issues:

1. The pre-existing genre field is now ignored. Running beet write with an (initially) empty genres field:

     genres: pop, rock, stage & screen -> 
   

   This means genre gets removed from all files in the library.

2. However once the above happens running beet write again shows:

     genre:  -> pop, rock, stage & screen
   

   However, nothing happens, since genre field is ignored when it comes to writing the file.

a reason for removing the misleading genre field in mediafile entirely in my opinion. We can only read it? And not write to it? Is this because of beets or because of mediafile? At least the docstring of the mediafile function says something like "...can get and set the first item of the list...". beetbox/mediafile#81 (comment)

This fake change will always show up if genre in the file is not in line with genre field in the database. This is because both genre and genres fields read the same metadata in the file, so this will always show up if they are not in sync.

another reason to get rid of one of those fields in mediafile. It's just confusing useless (magic) in my opinion 🤣

Does this need to have the MULTI_VALUE_DSV type which uses the funny \␀ separator? Or could it use the more straightforward SEMICOLON_SPACE_DSV type, which is only used by albumtypes?

See my linked comment again. I was wondering if this chosen separator has anything to do with the fact that id3v2.4 was the first (and it's still current) to define an actual "delimiter standard" for saving lists.

[JOJ0]

I want to keep a single source of truth (don't care whether it's named genre or genres, but most probably genre to make this backwards-compatible), so I suggest we adjust the ..._DSV type implementation to handle both list and str inputs, where the latter is split by the separator if needed. The field will always return a list on access.

Something else: To avoid confusion we should name it the same as it is in mediafile (if this is not a prerequisiste anyway?). There we have genres. So maybe we should just never use genre anywhere in beets anymore?

[JOJ0]

I want to keep a single source of truth (don't care whether it's named genre or genres, but most probably genre to make this backwards-compatible), so I suggest we adjust the ..._DSV type implementation to handle both list and str inputs, where the latter is split by the separator if needed. The field will always return a list on access.

Something else: To avoid confusion we should name it the same as it is in mediafile (if this is not a prerequisiste anyway?). There we have genres. So maybe we should just never use genre anywhere in beets anymore?

But after looking through your code in #5606.... Am I right that (so far) we write to a beets library field genre that is in the mediafile code named genres? Really?

[snejus]

I think the way it's implemented in mediafile is fine - it provides flexibility to use either the single genre or multiple genres field, supporting both old and new beets versions.

I think the issue is with the implementation on our side, where both singular and plural fields are used simultaneously which gave rise to the issues we had. We should use either one of them, preferably the plural version.

To avoid confusion we should name it the same as it is in mediafile (if this is not a prerequisiste anyway?). There we have genres. So maybe we should just never use genre anywhere in beets anymore?

Indeed. We should just drop genre, artist, mb_artistid etc. but we need to be mindful that external autotaggers use them, so we need to allow AlbumInfo and TrackInfo to be initialised with them (at which point we just make them plural).

[snejus]

Regarding the delimiter, I tested a couple of them and 'anything goes' it seems, so we could make it configurable by the user methinks.