PICARD-2607: Add a “_genre” variable #2545

rakim0 · 2024-10-19T19:50:17Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences:
Adds a custom field for folksonomy tags and genre.

Problem

There was no custom field for folksonomy tags. User was forced to either set all genre as genre tags or all folksonomy tags as Genre.
This PR aims to resolve it by introducing two new tags _folksonomy_tag and _genre.

JIRA ticket (optional): PICARD-2607

Solution

To get both _genres and _folksonomy tags from the MusicBrainz server, I removed the 'use_folksonomy` check in picard/items.py and stored the tags in respective variables. Then for writing the metadata, I added two new functions in picard/track.py that would update the metadata with _genre and _folksonomy_tag. And before writing genre into the file, check whether genre should be set to _folksonomy_tag or _genre itself.

Check for 'Use only my genre' option is still done in picard/items.py

Action

Additional actions required:

Update Picard documentation (please include a reference to this PR)
Other (please specify below)

The check has been moved here from picard/mbjson.py as we are loading _genres and _folksonomy_tags regardless of user setting. Thus for filling in the actual genre tag, we are checking the user preference.

rakim0 · 2024-10-19T19:54:44Z

I tried to follow the Contribution Guidelines as much as possible but was a bit unsure about having multiple commits but couldn't figure a good commit message so broke the commit into 4 parts.

Sophist-UK · 2024-10-19T21:33:17Z

I am really unclear whether this is actually a bug fix. And if it is an enhancement instead, then if it is not going to do something much more generalised, then it least needs to make sure it will be compatible with a future generalised function.

The issue with Genres is that there are so many competing sources of variable quality - many are free-form rather than a fixed list, those that are fixed lists have different fixed lists, and even when there are identical names, the definitions may be different or the allocation subjective. So if you are trying to fill a genre tag from multiple sources, you need to make sense of the mess you may often be given. In addition, there really needs to be needs to be a hierarchy i.e. there are many sub-forms of "Rock", and indeed these need to be many-to-many because there are also fusion genres which are sub-categories of more than one parent category.

So Picard IMO really needs to provide several generalised functions...

Putting downloaded genres into separate metadata variables
Providing functionality to clean-up these multiple lists of genres
Providing functionality to consolidate them down into a single genre list for a track.
Providing a plugin category for genre downloading - various types (artist, album, track) from various sources (MB being only one possible source of genre data).

I therefore encourage the authors and reviewers of this PR to think about the above and do their best to at least try to guide this PR into a form that is in the same general direction and which will at a minimum not make delivery of comprehensive genre functionality more difficult and ideally at least takes us a step in that direction.

(See also: Lyric handling which has similar (but simpler) plugin-based requirements.)

phw

Thanks a lot for this PR. I initially thought we can handle it more simple inside mbjson and directly write the values for _genres and _folksonomy_tags in the metadadata object there. But your implementation of storing the list with counts is more complex but needed to run the proper filtering later on.

Also tests should be fixed.

picard/item.py

picard/mbjson.py

phw · 2024-10-20T14:03:21Z

picard/track.py

+        config = get_config()
+        tags = Counter(self._folksonomy_tags)
+        tags += self.album._folksonomy_tags
+        self.metadata['_folksonomy_tags'] = self._genres_to_metadata(tags, join_with=config.setting['join_genres'])


To my understanding we can do this simpler and don't need to filter the values for _folksonomy_tags and _genres. We just include all the loaded values in either variable. Only the final genre variable gets filtered.

But I asked UltimateRiff on PICARD-2607 for clarification also.

What I'm also unsure about if _folksonomy_tags should strictly only contain the non-genre tags. In that case we would filter out everything that is in _genres.

Hmm I think _folksonomy_tags should only have the non-genre tags. But well I'll just wait to see what UltimateRiff expects before I implement it.

For the implementation though, is iterating through _genres.keys() and deleting them from _folksonomy_tags going to be good enough or is there a more efficient method?

@phw

Would you expect the _genres and _folksonomy_tags variables to hold all genres / folksonomy tags, or them to be filtered according to the genre filter rules? I assumed the first.

Would you expect the _folksonomy_tags variable to hold all folksoonomy tags (of which the genres are just a part of) or strictly only the non-genre tags?

UltimateRiff replied:

on the first expectation, I'm honestly not sure what my preference would be
on the second tho, I think I'd expect _folksonomy_tags to be only non-genre tags

I think for first, including everything is the right way to go for now.
For the deleting genres from folksonomy tags, is my earlier approach fine?

Is iterating through _genres.keys() and deleting them from _folksonomy_tags going to be good enough or is there a more efficient method?

@phw Implemented deleting genres from tags in commit: 58286b7

picard/track.py

phw · 2024-10-20T14:28:44Z

I therefore encourage the authors and reviewers of this PR to think about the above and do their best to at least try to guide this PR into a form that is in the same general direction and which will at a minimum not make delivery of comprehensive genre functionality more difficult and ideally at least takes us a step in that direction.

@rakim-0 @Sophist-UK We have a separate ticket PICARD-2054 for this, but this needs additional considerations. But it has no effect on this story, which is only about adding the _genres and _folksonomy_tags variables with data loaded from MB.

Genres for separate sources would be handled by the genre handling functions in item.py. Generally we have most functionality available for this, and probably we could change plugins to use MetadataItem.add_genre / MetadataItem.add_folksonomy tags feature. But for full implementation we should have genre providers similar to cover art providers that can be enabled / disabled. If we do this correctly we could also solve additional tickets, e.g. we could have providers separately for recordings, releases, artist etc. and users could choose.

It's not unlikely that if we implement this the code added here will need some refactoring as well, but that needs to be done when PICARD-2054 actually gets implemented.

rakim0 · 2024-10-20T18:44:03Z

I'll squash the smaller commits after I fix the tests.

Check for _genres and _folksonomy_tags individually as both are filled in separately now.

rakim0 · 2024-10-22T05:30:20Z

Fixed tests and resolved most comments.
I'm just unsure about this one: #2545 (comment) (filtering out genres from the folksonomy tags).

picard/track.py

phw

Thanks a lot for the updates. This looks good to me and should work.

zas

LGTM, thanks for your contribution.

rdswift · 2024-11-13T01:36:31Z

If you don't mind, can you please enter a ticket to update the Picard documentation to include this new variable? I suspect that the page that needs to be updated is https://github.com/metabrainz/picard-docs/blob/master/variables/variables_basic.rst

Thanks.

This function would remove the tags that are present in genre.

picard/track.py

phw

Thank you again. For me this looks great. Waiting for last review from @zas

zas

Declaring attributes as "protected" (prefixing them with underscore) is good, but then they aren't meant to be used outside the class, apart its subclasses.

For _genres and _folksonomy_tags, please check carefully, and use the getter method if not in the class itself or a subclass.

zas · 2024-11-18T09:20:19Z

picard/track.py

-        genres += self.album.genres
+        use_folksonomy = config.setting['folksonomy_tags']
+        if use_folksonomy:
+            genres += self.album._folksonomy_tags


Suggested change

genres += self.album._folksonomy_tags

genres += self.album.folksonomy_tags

zas · 2024-11-18T09:20:43Z

picard/track.py

+        if use_folksonomy:
+            genres += self.album._folksonomy_tags
+        else:
+            genres += self.album._genres


Suggested change

genres += self.album._genres

genres += self.album.genres

zas · 2024-11-18T09:23:04Z

test/test_mbjson.py

-        self.assertEqual(a.genres, {
-            'genre1': 6, 'genre2': 3,
-            'tag1': 6, 'tag2': 3})
+        self.assertEqual(a._genres, {


Suggested change

self.assertEqual(a._genres, {

self.assertEqual(a.genres, {

zas · 2024-11-18T09:23:15Z

test/test_mbjson.py

+        self.assertEqual(a._genres, {
+            'genre1': 6, 'genre2': 3
+        })
+        self.assertEqual(a._folksonomy_tags, {


Suggested change

self.assertEqual(a._folksonomy_tags, {

self.assertEqual(a.folksonomy_tags, {

zas · 2024-11-18T09:23:26Z

test/test_mbjson.py

        for artist in a._album_artists:
-            self.assertEqual(artist.genres, {
+            self.assertEqual(artist._folksonomy_tags, {


Suggested change

self.assertEqual(artist._folksonomy_tags, {

self.assertEqual(artist.folksonomy_tags, {

zas · 2024-11-18T09:24:15Z

test/test_mbjson.py

            'blue-eyed soul': 1,
            'pop': 3})
        for artist in t._track_artists:
-            self.assertEqual(artist.genres, {
+            self.assertEqual(artist._folksonomy_tags, {


Suggested change

self.assertEqual(artist._folksonomy_tags, {

self.assertEqual(artist.folksonomy_tags, {

zas · 2024-11-18T09:24:26Z

test/test_mbjson.py

@@ -770,7 +776,7 @@ def test_release_group(self):
        self.assertEqual(m['releasetype'], 'album')
        self.assertEqual(m['~primaryreleasetype'], 'album')
        self.assertEqual(m['~releasegroup'], 'The Dark Side of the Moon')
-        self.assertEqual(r.genres, {'test2': 3, 'test': 6})
+        self.assertEqual(r._folksonomy_tags, {'test2': 3, 'test': 6})


Suggested change

self.assertEqual(r._folksonomy_tags, {'test2': 3, 'test': 6})

self.assertEqual(r.folksonomy_tags, {'test2': 3, 'test': 6})

zas · 2024-11-18T09:24:59Z

picard/track.py

+
+    def _add_folksonomy_tags(self):
+        tags = Counter(self._folksonomy_tags)
+        tags += self.album._folksonomy_tags


Suggested change

tags += self.album._folksonomy_tags

tags += self.album.folksonomy_tags

zas · 2024-11-18T09:25:11Z

picard/track.py

+
+    def _add_genres(self):
+        genre = Counter(self._genres)
+        genre += self.album._genres


Suggested change

genre += self.album._genres

genre += self.album.genres

zas · 2024-11-18T09:26:47Z

picard/track.py

+        genre += self.album._genres
+        self._add_tags(genre, '_genres')
+
+    def _delete_genres_from_tags(self):


Here we're in Track and delete stuff in Album, what about moving this method to Album instead and calling it from here?

Sophist-UK · 2024-11-18T10:23:42Z

Declaring attributes as "protected" (prefixing them with underscore) is good, but then they aren't meant to be used outside the class, apart its subclasses.

Aside from in tests, are these used outside the class?

Because I wonder whether using protected attributes in tests is actually a good thing or not. (And funny enough about 20mins before reading this I was considering exactly this question but for PHP - except in PHP protected attributes are not actually accessible outside a class except through reflection manipulation).

The reason I wonder is because the idea behind protecting them is to prevent their use in genuine application code that shouldn't have access - and tests are IMO a special case. So on the one hand, making them non-protected fits the language intent and avoids e.g. codacy flags, but then makes them available to other genuine code when they probably should be hidden - and on the other hand using protected attributes outside the class in tests may lead to issues if Python ever decides that protected really means protected.

On the whole, my personal opinion leans towards continuing to have them as protected attributes and using them outside the class only in tests - and I am wondering how to achieve this in php tests using reflection.

rakim0 added 4 commits October 20, 2024 00:42

Add _folksonomy_tags property to MetadataItem Class

43d3805

Load tags to _folksonomy_tags and genres to _genres

a3c9b7f

Write _genres and _folksonomy_tags into file

a05a1e8

Check 'folksonomy_tag' setting and fill genre

2785839

The check has been moved here from picard/mbjson.py as we are loading _genres and _folksonomy_tags regardless of user setting. Thus for filling in the actual genre tag, we are checking the user preference.

phw requested changes Oct 20, 2024

View reviewed changes

rakim0 added 3 commits October 20, 2024 23:49

Move the method to follow add_genre

cf267f6

Remove the check for 'use_genre'

47a139c

Move repeated code to _add_tags, remove filter

d543ec0

Fix tests

c82f01c

Check for _genres and _folksonomy_tags individually as both are filled in separately now.

zas requested a review from phw November 5, 2024 19:49

zas requested changes Nov 5, 2024

View reviewed changes

picard/track.py Outdated Show resolved Hide resolved

picard/track.py Outdated Show resolved Hide resolved

removing unneeded config variables

5b50798

phw previously approved these changes Nov 12, 2024

View reviewed changes

zas previously approved these changes Nov 12, 2024

View reviewed changes

Add function to remove genres from tags

58286b7

This function would remove the tags that are present in genre.

rakim0 dismissed stale reviews from zas and phw via 58286b7 November 17, 2024 10:29

rdswift reviewed Nov 17, 2024

View reviewed changes

picard/track.py Outdated Show resolved Hide resolved

Change genres to genre

3f0a795

phw approved these changes Nov 18, 2024

View reviewed changes

phw requested a review from zas November 18, 2024 07:11

zas requested changes Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PICARD-2607: Add a “_genre” variable #2545

PICARD-2607: Add a “_genre” variable #2545

rakim0 commented Oct 19, 2024 •

edited

Loading

rakim0 commented Oct 19, 2024

Sophist-UK commented Oct 19, 2024

phw left a comment

phw Oct 20, 2024

rakim0 Oct 20, 2024 •

edited

Loading

rakim0 Oct 26, 2024 •

edited

Loading

rakim0 Nov 17, 2024 •

edited

Loading

phw commented Oct 20, 2024

rakim0 commented Oct 20, 2024

rakim0 commented Oct 22, 2024

phw left a comment

zas left a comment

rdswift commented Nov 13, 2024

phw left a comment

zas left a comment

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

zas Nov 18, 2024

Sophist-UK commented Nov 18, 2024

	genres += self.album._folksonomy_tags
	genres += self.album.folksonomy_tags

	self.assertEqual(a._folksonomy_tags, {
	self.assertEqual(a.folksonomy_tags, {

	self.assertEqual(artist._folksonomy_tags, {
	self.assertEqual(artist.folksonomy_tags, {

	self.assertEqual(r._folksonomy_tags, {'test2': 3, 'test': 6})
	self.assertEqual(r.folksonomy_tags, {'test2': 3, 'test': 6})

	tags += self.album._folksonomy_tags
	tags += self.album.folksonomy_tags

PICARD-2607: Add a “_genre” variable #2545

Are you sure you want to change the base?

PICARD-2607: Add a “_genre” variable #2545

Conversation

rakim0 commented Oct 19, 2024 • edited Loading

Summary

Problem

Solution

Action

rakim0 commented Oct 19, 2024

Sophist-UK commented Oct 19, 2024

phw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakim0 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

rakim0 Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

rakim0 Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

phw commented Oct 20, 2024

rakim0 commented Oct 20, 2024

rakim0 commented Oct 22, 2024

phw left a comment

Choose a reason for hiding this comment

zas left a comment

Choose a reason for hiding this comment

rdswift commented Nov 13, 2024

phw left a comment

Choose a reason for hiding this comment

zas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sophist-UK commented Nov 18, 2024

rakim0 commented Oct 19, 2024 •

edited

Loading

rakim0 Oct 20, 2024 •

edited

Loading

rakim0 Oct 26, 2024 •

edited

Loading

rakim0 Nov 17, 2024 •

edited

Loading