Lastgenre: New config option keep_existing

beetsplug/lastgenre/__init__.py

@@ -103,15 +103,27 @@ def __init__(self):
                 "canonical": False,
                 "source": "album",
                 "force": True,
+                "keep_existing": True,
                 "auto": True,
                 "separator": ", ",
                 "prefer_specific": False,
                 "title_case": True,
             }
         )
-
+        self.config_validation()
         self.setup()
 
+    def config_validation(self) -> None:
+        """Quits plugin when invalid configurations are detected."""
+        keep_existing = self.config["keep_existing"].get()
+        force = self.config["force"].get()
+
+        if keep_existing and not force:
+            raise ui.UserError(
+                "Invalid lastgenre plugin configuration (enable force with "
+                "keep_existing!)"
+            )
+
     def setup(self):
         """Setup plugin from config options"""
         if self.config["auto"]:
@@ -165,6 +177,20 @@ def sources(self):
         elif source == "artist":
             return ("artist",)
 
+    # More canonicalization and general helpers.
+
+    def _to_delimited_genre_string(self, tags: list[str]) -> str:
+        """Reduce tags list to configured count, format and return as delimited
+        string."""
+        separator = self.config["separator"].as_str()
+        max_count = self.config["count"].get(int)
+
+        genres = tags[:max_count]
+        if self.config["title_case"]:
+            genres = [g.title() for g in genres]
+
+        return separator.join(genres)
+
     def _get_depth(self, tag):
         """Find the depth of a tag in the genres tree."""
         depth = None
@@ -184,9 +210,7 @@ def _sort_by_depth(self, tags):
         return [p[1] for p in depth_tag_pairs]
 
     def _resolve_genres(self, tags):
-        """Given a list of strings, return a genre by joining them into a
-        single string and (optionally) canonicalizing each.
-        """
+        """Given a list of genre strings, filters, sorts and canonicalizes."""
         if not tags:
             return None
 
@@ -201,7 +225,7 @@ def _resolve_genres(self, tags):
                     parents = [
                         x
                         for x in find_parents(tag, self.c14n_branches)
-                        if self._is_allowed(x)
+                        if self._is_valid(x)
                     ]
                 else:
                     parents = [find_parents(tag, self.c14n_branches)[-1]]
@@ -224,13 +248,9 @@ def _resolve_genres(self, tags):
 
         # c14n only adds allowed genres but we may have had forbidden genres in
         # the original tags list
-        tags = [self._format_tag(x) for x in tags if self._is_allowed(x)]
+        return [x for x in tags if self._is_valid(x)]
 
-        return (
-            self.config["separator"]
-            .as_str()
-            .join(tags[: self.config["count"].get(int)])
-        )
+        return tags
 
     def _format_tag(self, tag):
         if self.config["title_case"]:
@@ -242,19 +262,21 @@ def fetch_genre(self, lastfm_obj):
         can be found. Ex. 'Electronic, House, Dance'
         """
         min_weight = self.config["min_weight"].get(int)
-        return self._resolve_genres(self._tags_for(lastfm_obj, min_weight))
+        return self._tags_for(lastfm_obj, min_weight)
 
-    def _is_allowed(self, genre):
-        """Determine whether the genre is present in the whitelist,
-        returning a boolean.
+    def _is_valid(self, genre) -> bool:
+        """Check if the genre is valid.
+
+        Depending on the whitelist property, valid means a genre is in the
+        whitelist or any genre is allowed.
         """
-        if genre is None:
-            return False
-        if not self.whitelist or genre in self.whitelist:
-            return True
-        return False
+        return (
+            genre is None
+            or not self.whitelist
+            or genre.lower() in self.whitelist
+        )
 
-    # Cached entity lookups.
+    # Cached last.fm entity lookups.
 
     def _last_lookup(self, entity, method, *args):
         """Get a genre based on the named entity using the callable `method`
@@ -270,7 +292,7 @@ def _last_lookup(self, entity, method, *args):
 
         key = "{}.{}".format(entity, "-".join(str(a) for a in args))
         if key in self._genre_cache:
-            return self._genre_cache[key]
+            result = self._genre_cache[key]
         else:
             args_replaced = []
             for arg in args:
@@ -280,7 +302,8 @@ def _last_lookup(self, entity, method, *args):
 
             genre = self.fetch_genre(method(*args_replaced))
             self._genre_cache[key] = genre
-            return genre
+            result = genre
+        return result
 
     def fetch_album_genre(self, obj):
         """Return the album genre for this Item or Album."""
@@ -302,42 +325,119 @@ def fetch_track_genre(self, obj):
             "track", LASTFM.get_track, obj.artist, obj.title
         )
 
+    # Main processing: _get_genre() and helpers.
+
+    def _get_existing_genres(self, obj, separator):
+        """Return a list of genres for this Item or Album."""
+        if isinstance(obj, library.Item):
+            item_genre = obj.get("genre", with_album=False).split(separator)
+        else:
+            item_genre = obj.get("genre").split(separator)
+
+        if any(item_genre):
+            return item_genre
+        return []
+
+    def _dedup_genres(self, genres, whitelist_only=False):
+        """Return a list of deduplicated genres. Depending on the
+        whitelist_only option, gives filtered or unfiltered results.
+        Makes sure genres are handled all lower case."""
+        if whitelist_only:
+            return deduplicate(
+                [g.lower() for g in genres if self._is_valid(g)]
+            )
+        return deduplicate([g.lower() for g in genres])
+
+    def _combine_and_label_genres(
+        self, new_genres: list, keep_genres: list, log_label: str
+    ) -> tuple:
+        """Combines genres and returns them with a logging label.
+
+        Parameters:
+            new_genres (list): The new genre result to process.
+            keep_genres (list): Existing genres to combine with new ones
+            log_label (str): A label (like "track", "album") we possibly
+                combine with a prefix. For example resulting in something like
+                "keep + track" or just "track".
+
+        Returns:
+            tuple: A tuple containing the combined genre string and the
+            'logging label'.
+        """
+        self._log.debug(f"fetched last.fm tags: {new_genres}")
+        combined = deduplicate(keep_genres + new_genres)
+        resolved = self._resolve_genres(combined)
+        reduced = self._to_delimited_genre_string(resolved)
+
+        if new_genres and keep_genres:
+            return reduced, f"keep + {log_label}"
+        if new_genres:
+            return reduced, log_label
+        return None, log_label
+
     def _get_genre(self, obj):
-        """Get the genre string for an Album or Item object based on
-        self.sources. Return a `(genre, source)` pair. The
-        prioritization order is:
+        """Get the final genre string for an Album or Item object
+
+        `self.sources` specifies allowed genre sources, prioritized as follows:
             - track (for Items only)
             - album
             - artist
             - original
             - fallback
             - None
+
+        Parameters:
+            obj: Either an Album or Item object.
+
+         Returns:
+             tuple: A `(genre, label)` pair, where `label` is a string used for
+                logging that describes the result. For example, "keep + artist"
+                indicates that existing genres were combined with new last.fm
+                genres, while "artist" means only new last.fm genres are
+                included.
         """
 
-        # Shortcut to existing genre if not forcing.
-        if not self.config["force"] and self._is_allowed(obj.genre):
-            return obj.genre, "keep"
+        separator = self.config["separator"].get()
+        keep_genres = []
+
+        genres = self._get_existing_genres(obj, separator)
+        if genres and not self.config["force"]:
+            # Without force we don't touch pre-populated tags and return early
+            # with the original contents. We format back to string tough.
+            keep_genres = self._dedup_genres(genres)
+            return separator.join(keep_genres), "keep"
+
+        if self.config["force"]:
+            # Simply forcing doesn't keep any.
+            keep_genres = []
+            # keep_existing remembers, according to the whitelist setting,
+            # any or just allowed genres.
+            if self.config["keep_existing"] and self.config["whitelist"]:
+                keep_genres = self._dedup_genres(genres, whitelist_only=True)
+            elif self.config["keep_existing"]:
+                keep_genres = self._dedup_genres(genres)
 
         # Track genre (for Items only).
-        if isinstance(obj, library.Item):
-            if "track" in self.sources:
-                result = self.fetch_track_genre(obj)
-                if result:
-                    return result, "track"
+        if isinstance(obj, library.Item) and "track" in self.sources:
+            if new_genres := self.fetch_track_genre(obj):
+                return self._combine_and_label_genres(
+                    new_genres, keep_genres, "track"
+                )
 
         # Album genre.
         if "album" in self.sources:
-            result = self.fetch_album_genre(obj)
-            if result:
-                return result, "album"
+            if new_genres := self.fetch_album_genre(obj):
+                return self._combine_and_label_genres(
+                    new_genres, keep_genres, "album"
+                )
 
         # Artist (or album artist) genre.
         if "artist" in self.sources:
-            result = None
+            new_genres = None
             if isinstance(obj, library.Item):
-                result = self.fetch_artist_genre(obj)
+                new_genres = self.fetch_artist_genre(obj)
             elif obj.albumartist != config["va_name"].as_str():
-                result = self.fetch_album_artist_genre(obj)
+                new_genres = self.fetch_album_artist_genre(obj)
             else:
                 # For "Various Artists", pick the most popular track genre.
                 item_genres = []
@@ -348,26 +448,34 @@ def _get_genre(self, obj):
                     if not item_genre:
                         item_genre = self.fetch_artist_genre(item)
                     if item_genre:
-                        item_genres.append(item_genre)
+                        item_genres += item_genre
                 if item_genres:
-                    result, _ = plurality(item_genres)
+                    most_popular, rank = plurality(item_genres)
+                    new_genres = [most_popular]
+                    self._log.debug(
+                        'Most popular track genre "{}" ({}) for VA album.',
+                        most_popular,
+                        rank,
+                    )
 
-            if result:
-                return result, "artist"
+            if new_genres:
+                return self._combine_and_label_genres(
+                    new_genres, keep_genres, "artist"
+                )
 
-        # Filter the existing genre.
+        # Didn't find anything, leave original
         if obj.genre:
-            result = self._resolve_genres([obj.genre])
-            if result:
-                return result, "original"
+            return obj.genre, "original fallback"
 
-        # Fallback string.
-        fallback = self.config["fallback"].get()
-        if fallback:
+        # No original, return fallback string
+        if fallback := self.config["fallback"].get():
             return fallback, "fallback"
 
+        # No fallback configured
         return None, None
 
+    # Beets plugin hooks and CLI.
+
     def commands(self):
         lastgenre_cmd = ui.Subcommand("lastgenre", help="fetch genres")
         lastgenre_cmd.parser.add_option(
@@ -377,6 +485,20 @@ def commands(self):
             action="store_true",
             help="re-download genre when already present",
         )
+        lastgenre_cmd.parser.add_option(
+            "-k",
+            "--keep-existing",
+            dest="keep_existing",
+            action="store_true",
+            help="keep already present genres",
+        )
+        lastgenre_cmd.parser.add_option(
+            "-K",
+            "--keep-none",
+            dest="keep_existing",
+            action="store_false",
+            help="don't keep already present genres",
+        )
         lastgenre_cmd.parser.add_option(
             "-s",
             "--source",

docs/changelog.rst

@@ -38,6 +38,10 @@ Bug fixes:
   request their own last.fm genre. Also log messages regarding what's been
   tagged are now more polished.
   :bug:`5582`
+  * :doc:`plugins/lastgenre`: The new configuration option, ``keep_existing``,
+  provides more fine-grained control over how pre-populated genre tags are
+  handled. The ``force`` option now behaves in a more conventional manner.
+  :bug:`4982`
 
 For packagers:
 

docs/plugins/lastgenre.rst

@@ -111,6 +111,27 @@ Last.fm returns both of those tags, lastgenre is going to use the most
 popular, which is often the most generic (in this case ``folk``). By setting
 ``prefer_specific`` to true, lastgenre would use ``americana`` instead.
 
+Handling pre-populated tags
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The ``force``, ``keep_existing`` and ``whitelist`` options control how
+pre-existing genres are handled.
+
+By default, the plugin *combines* newly fetched last.fm genres with whitelisted
+pre-existing ones (``force: yes`` and ``keep_existing: yes``).
+
+To write new genres to empty tags only and keep pre-populated tags untouched,
+set ``force: no``.
+
+To *overwrite* any content of pre-populated tags, set ``force: yes`` and
+``keep_existing: no``.
+
+To *combine* newly fetched last.fm genres with any pre-existing ones, set
+``force: yes``, ``keep_existing: yes`` and ``whitelist: False``.
+
+Combining ``force: no`` and ``keep_existing: yes`` is invalid (since ``force:
+no`` means `not touching` existing tags anyway).
+
 Configuration
 -------------
 
@@ -128,9 +149,15 @@ configuration file. The available options are:
 - **fallback**: A string if to use a fallback genre when no genre is found.
   You can use the empty string ``''`` to reset the genre.
   Default: None.
-- **force**: By default, beets will always fetch new genres, even if the files
-  already have one. To instead leave genres in place in when they pass the
-  whitelist, set the ``force`` option to ``no``.
+- **force**: By default, lastgenre will fetch new genres for empty as well as
+  pre-populated tags. Enable the ``keep_existing`` option to combine existing
+  and new genres. (see `Handling pre-populated tags`_).
+  Default: ``no``.
+- **keep_existing**: By default, genres remain in pre-populated tags. Depending
+  on whether or not ``whitelist`` is enabled, existing genres get "a cleanup".
+  Enabling ``keep_existing`` is only valid in combination with an active
+  ``force`` option. To ensure only fresh last.fm genres, disable this option.
+  (see `Handling pre-populated tags`_)
   Default: ``yes``.
 - **min_weight**: Minimum popularity factor below which genres are discarded.
   Default: 10.