From 996a116a6260ab0ff660f2ad1f2fb234b3c0d409 Mon Sep 17 00:00:00 2001 From: Serene-Arc Date: Sun, 22 Jan 2023 11:48:52 +1000 Subject: [PATCH 1/7] artresizer: type module --- beets/util/artresizer.py | 123 ++++++++++++++++++++++++++++----------- 1 file changed, 89 insertions(+), 34 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index ffbc2edba..18a5ef82d 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -22,6 +22,8 @@ import platform import re import subprocess from itertools import chain +from typing import AnyStr, Tuple, Optional, Mapping, Union +from PIL import Image from urllib.parse import urlencode from beets import logging, util @@ -32,7 +34,7 @@ PROXY_URL = "https://images.weserv.nl/" log = logging.getLogger("beets") -def resize_url(url, maxwidth, quality=0): +def resize_url(url: str, maxwidth: int, quality: int = 0) -> str: """Return a proxied image URL that resizes the original image to maxwidth (preserving aspect ratio). """ @@ -124,8 +126,13 @@ class IMBackend(LocalBackend): self.compare_cmd = ["magick", "compare"] def resize( - self, maxwidth, path_in, path_out=None, quality=0, max_filesize=0 - ): + self, + maxwidth: int, + path_in: AnyStr, + path_out: Optional[AnyStr] = None, + quality: int = 0, + max_filesize: int = 0, + ) -> AnyStr: """Resize using ImageMagick. Use the ``magick`` program or ``convert`` on older versions. Return @@ -174,7 +181,7 @@ class IMBackend(LocalBackend): return path_out - def get_size(self, path_in): + def get_size(self, path_in: str) -> Optional[Tuple[int, ...]]: cmd = self.identify_cmd + [ "-format", "%w %h", @@ -199,7 +206,7 @@ class IMBackend(LocalBackend): log.warning("Could not understand IM output: {0!r}", out) return None - def deinterlace(self, path_in, path_out=None): + def deinterlace(self, path_in: AnyStr, path_out: Optional[AnyStr] = None) -> AnyStr: if not path_out: path_out = get_temp_filename(__name__, "deinterlace_IM_", path_in) @@ -217,7 +224,7 @@ class IMBackend(LocalBackend): # FIXME: Should probably issue a warning? return path_in - def get_format(self, filepath): + def get_format(self, filepath: AnyStr) -> Optional[bytes]: cmd = self.identify_cmd + ["-format", "%[magick]", syspath(filepath)] try: @@ -226,7 +233,12 @@ class IMBackend(LocalBackend): # FIXME: Should probably issue a warning? return None - def convert_format(self, source, target, deinterlaced): + def convert_format( + self, + source: AnyStr, + target: AnyStr, + deinterlaced: bool, + ) -> AnyStr: cmd = self.convert_cmd + [ syspath(source), *(["-interlace", "none"] if deinterlaced else []), @@ -243,10 +255,15 @@ class IMBackend(LocalBackend): return source @property - def can_compare(self): + def can_compare(self) -> bool: return self.version() > (6, 8, 7) - def compare(self, im1, im2, compare_threshold): + def compare( + self, + im1: Image, + im2: Image, + compare_threshold: float, + ) -> Optional[bool]: is_windows = platform.system() == "Windows" # Converting images to grayscale tends to minimize the weight @@ -329,10 +346,10 @@ class IMBackend(LocalBackend): return phash_diff <= compare_threshold @property - def can_write_metadata(self): + def can_write_metadata(self) -> bool: return True - def write_metadata(self, file, metadata): + def write_metadata(self, file: AnyStr, metadata: Mapping): assignments = list( chain.from_iterable(("-set", k, v) for k, v in metadata.items()) ) @@ -359,8 +376,13 @@ class PILBackend(LocalBackend): self.version() def resize( - self, maxwidth, path_in, path_out=None, quality=0, max_filesize=0 - ): + self, + maxwidth: int, + path_in: AnyStr, + path_out: Optional[AnyStr] = None, + quality: int = 0, + max_filesize: int = 0, + ) -> AnyStr: """Resize using Python Imaging Library (PIL). Return the output path of resized image. """ @@ -429,7 +451,7 @@ class PILBackend(LocalBackend): ) return path_in - def get_size(self, path_in): + def get_size(self, path_in: AnyStr) -> Optional[Tuple[int, int]]: from PIL import Image try: @@ -441,7 +463,11 @@ class PILBackend(LocalBackend): ) return None - def deinterlace(self, path_in, path_out=None): + def deinterlace( + self, + path_in: AnyStr, + path_out: Optional[AnyStr] = None, + ) -> AnyStr: if not path_out: path_out = get_temp_filename(__name__, "deinterlace_PIL_", path_in) @@ -455,7 +481,7 @@ class PILBackend(LocalBackend): # FIXME: Should probably issue a warning? return path_in - def get_format(self, filepath): + def get_format(self, filepath: AnyStr) -> Optional[str]: from PIL import Image, UnidentifiedImageError try: @@ -470,7 +496,12 @@ class PILBackend(LocalBackend): log.exception("failed to detect image format for {}", filepath) return None - def convert_format(self, source, target, deinterlaced): + def convert_format( + self, + source: AnyStr, + target: AnyStr, + deinterlaced: bool, + ) -> str: from PIL import Image, UnidentifiedImageError try: @@ -488,18 +519,23 @@ class PILBackend(LocalBackend): return source @property - def can_compare(self): + def can_compare(self) -> bool: return False - def compare(self, im1, im2, compare_threshold): + def compare( + self, + im1: Image, + im2: Image, + compare_threshold: float, + ): # It is an error to call this when ArtResizer.can_compare is not True. raise NotImplementedError() @property - def can_write_metadata(self): + def can_write_metadata(self) -> bool: return True - def write_metadata(self, file, metadata): + def write_metadata(self, file: AnyStr, metadata: Mapping): from PIL import Image, PngImagePlugin # FIXME: Detect and handle other file types (currently, the only user @@ -523,7 +559,7 @@ class Shareable(type): cls._instance = None @property - def shared(cls): + def shared(cls) -> 'Shareable': if cls._instance is None: cls._instance = cls() return cls._instance @@ -554,14 +590,19 @@ class ArtResizer(metaclass=Shareable): self.local_method = None @property - def method(self): + def method(self) -> str: if self.local: return self.local_method.NAME else: return "WEBPROXY" def resize( - self, maxwidth, path_in, path_out=None, quality=0, max_filesize=0 + self, + maxwidth: int, + path_in: AnyStr, + path_out: Optional[AnyStr]=None, + quality: int = 0, + max_filesize: int = 0, ): """Manipulate an image file according to the method, returning a new path. For PIL or IMAGEMAGIC methods, resizes the image to a @@ -580,7 +621,11 @@ class ArtResizer(metaclass=Shareable): # Handled by `proxy_url` already. return path_in - def deinterlace(self, path_in, path_out=None): + def deinterlace( + self, + path_in: AnyStr, + path_out: Optional[AnyStr] = None, + ) -> AnyStr: """Deinterlace an image. Only available locally. @@ -591,7 +636,7 @@ class ArtResizer(metaclass=Shareable): # FIXME: Should probably issue a warning? return path_in - def proxy_url(self, maxwidth, url, quality=0): + def proxy_url(self, maxwidth: int, url: str, quality: int = 0): """Modifies an image URL according the method, returning a new URL. For WEBPROXY, a URL on the proxy server is returned. Otherwise, the URL is returned unmodified. @@ -603,13 +648,13 @@ class ArtResizer(metaclass=Shareable): return resize_url(url, maxwidth, quality) @property - def local(self): + def local(self) -> bool: """A boolean indicating whether the resizing method is performed locally (i.e., PIL or ImageMagick). """ return self.local_method is not None - def get_size(self, path_in): + def get_size(self, path_in: AnyStr) -> Union[Tuple[int, int], AnyStr]: """Return the size of an image file as an int couple (width, height) in pixels. @@ -621,7 +666,7 @@ class ArtResizer(metaclass=Shareable): # FIXME: Should probably issue a warning? return path_in - def get_format(self, path_in): + def get_format(self, path_in: AnyStr) -> Optional[str]: """Returns the format of the image as a string. Only available locally. @@ -632,7 +677,12 @@ class ArtResizer(metaclass=Shareable): # FIXME: Should probably issue a warning? return None - def reformat(self, path_in, new_format, deinterlaced=True): + def reformat( + self, + path_in: AnyStr, + new_format: str, + deinterlaced: bool = True, + ) -> AnyStr: """Converts image to desired format, updating its extension, but keeping the same filename. @@ -664,7 +714,7 @@ class ArtResizer(metaclass=Shareable): return result_path @property - def can_compare(self): + def can_compare(self) -> bool: """A boolean indicating whether image comparison is available""" if self.local: @@ -672,7 +722,12 @@ class ArtResizer(metaclass=Shareable): else: return False - def compare(self, im1, im2, compare_threshold): + def compare( + self, + im1: Image, + im2: Image, + compare_threshold: float, + ) -> Optional[bool]: """Return a boolean indicating whether two images are similar. Only available locally. @@ -684,7 +739,7 @@ class ArtResizer(metaclass=Shareable): return None @property - def can_write_metadata(self): + def can_write_metadata(self) -> bool: """A boolean indicating whether writing image metadata is supported.""" if self.local: @@ -692,7 +747,7 @@ class ArtResizer(metaclass=Shareable): else: return False - def write_metadata(self, file, metadata): + def write_metadata(self, file: AnyStr, metadata: Mapping): """Write key-value metadata to the image file. Only available locally. Currently, expects the image to be a PNG file. From 7acfe8932a9dc6af34979a06722af395a9b183bf Mon Sep 17 00:00:00 2001 From: Serene-Arc Date: Sun, 22 Jan 2023 12:12:44 +1000 Subject: [PATCH 2/7] artresizer: add some missing typings --- beets/util/artresizer.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 18a5ef82d..24c8e5e68 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -58,7 +58,7 @@ _NOT_AVAILABLE = object() class LocalBackend: @classmethod - def available(cls): + def available(cls) -> bool: try: cls.version() return True @@ -76,7 +76,7 @@ class IMBackend(LocalBackend): _legacy = None @classmethod - def version(cls): + def version(cls) -> Optional[Union[object, Tuple[int, int, int]]]: """Obtain and cache ImageMagick version. Raises `LocalBackendNotAvailableError` if not available. @@ -206,7 +206,11 @@ class IMBackend(LocalBackend): log.warning("Could not understand IM output: {0!r}", out) return None - def deinterlace(self, path_in: AnyStr, path_out: Optional[AnyStr] = None) -> AnyStr: + def deinterlace( + self, + path_in: AnyStr, + path_out: Optional[AnyStr] = None, + ) -> AnyStr: if not path_out: path_out = get_temp_filename(__name__, "deinterlace_IM_", path_in) @@ -603,7 +607,7 @@ class ArtResizer(metaclass=Shareable): path_out: Optional[AnyStr]=None, quality: int = 0, max_filesize: int = 0, - ): + ) -> AnyStr: """Manipulate an image file according to the method, returning a new path. For PIL or IMAGEMAGIC methods, resizes the image to a temporary file and encodes with the specified quality level. @@ -636,7 +640,7 @@ class ArtResizer(metaclass=Shareable): # FIXME: Should probably issue a warning? return path_in - def proxy_url(self, maxwidth: int, url: str, quality: int = 0): + def proxy_url(self, maxwidth: int, url: str, quality: int = 0) -> str: """Modifies an image URL according the method, returning a new URL. For WEBPROXY, a URL on the proxy server is returned. Otherwise, the URL is returned unmodified. From c90ff273153d92364db52f17a62f45319e75fbff Mon Sep 17 00:00:00 2001 From: Serene-Arc Date: Mon, 13 Feb 2023 13:20:09 +1000 Subject: [PATCH 3/7] artresizer: make import conditional --- beets/util/artresizer.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 24c8e5e68..0eff9b77f 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -22,13 +22,15 @@ import platform import re import subprocess from itertools import chain -from typing import AnyStr, Tuple, Optional, Mapping, Union -from PIL import Image +from typing import AnyStr, Tuple, Optional, Mapping, Union, TYPE_CHECKING from urllib.parse import urlencode from beets import logging, util from beets.util import displayable_path, get_temp_filename, syspath +if TYPE_CHECKING: + from PIL import Image + PROXY_URL = "https://images.weserv.nl/" log = logging.getLogger("beets") From b18e6e0654607ad0f60f8934b49859b3f8c12932 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 15 Apr 2025 22:25:13 +0200 Subject: [PATCH 4/7] artresizer: revise typings This is more of a "what should the types be", in particular regarding paths, it has not yet been run through mypy. That will be done next, which is probably going to highlight a bunch of issues that should lead to either the code being fixed, or the types adjusted. --- beets/util/artresizer.py | 160 ++++++++++++++++++++++----------------- 1 file changed, 89 insertions(+), 71 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 0eff9b77f..1baca3cd0 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -16,20 +16,26 @@ public resizing proxy if neither is available. """ +from __future__ import annotations + import os import os.path import platform import re import subprocess from itertools import chain -from typing import AnyStr, Tuple, Optional, Mapping, Union, TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar, Mapping, Self from urllib.parse import urlencode from beets import logging, util from beets.util import displayable_path, get_temp_filename, syspath if TYPE_CHECKING: - from PIL import Image + try: + from PIL import Image + except ImportError: + from typing import Any + Image = Any PROXY_URL = "https://images.weserv.nl/" @@ -58,7 +64,10 @@ class LocalBackendNotAvailableError(Exception): _NOT_AVAILABLE = object() +# FIXME: Turn this into an ABC with all methods that a backend should have class LocalBackend: + NAME: ClassVar[str] + @classmethod def available(cls) -> bool: try: @@ -74,11 +83,11 @@ class IMBackend(LocalBackend): # These fields are used as a cache for `version()`. `_legacy` indicates # whether the modern `magick` binary is available or whether to fall back # to the old-style `convert`, `identify`, etc. commands. - _version = None - _legacy = None + _version: tuple[int, int, int] | None = None + _legacy: bool | None = None @classmethod - def version(cls) -> Optional[Union[object, Tuple[int, int, int]]]: + def version(cls) -> tuple[int, int, int]: """Obtain and cache ImageMagick version. Raises `LocalBackendNotAvailableError` if not available. @@ -107,7 +116,7 @@ class IMBackend(LocalBackend): else: return cls._version - def __init__(self): + def __init__(self) -> None: """Initialize a wrapper around ImageMagick for local image operations. Stores the ImageMagick version and legacy flag. If ImageMagick is not @@ -130,11 +139,11 @@ class IMBackend(LocalBackend): def resize( self, maxwidth: int, - path_in: AnyStr, - path_out: Optional[AnyStr] = None, + path_in: bytes, + path_out: bytes | None = None, quality: int = 0, max_filesize: int = 0, - ) -> AnyStr: + ) -> bytes: """Resize using ImageMagick. Use the ``magick`` program or ``convert`` on older versions. Return @@ -183,7 +192,7 @@ class IMBackend(LocalBackend): return path_out - def get_size(self, path_in: str) -> Optional[Tuple[int, ...]]: + def get_size(self, path_in: bytes) -> tuple[int, int] | None: cmd = self.identify_cmd + [ "-format", "%w %h", @@ -209,10 +218,10 @@ class IMBackend(LocalBackend): return None def deinterlace( - self, - path_in: AnyStr, - path_out: Optional[AnyStr] = None, - ) -> AnyStr: + self, + path_in: bytes, + path_out: bytes | None = None, + ) -> bytes: if not path_out: path_out = get_temp_filename(__name__, "deinterlace_IM_", path_in) @@ -230,8 +239,8 @@ class IMBackend(LocalBackend): # FIXME: Should probably issue a warning? return path_in - def get_format(self, filepath: AnyStr) -> Optional[bytes]: - cmd = self.identify_cmd + ["-format", "%[magick]", syspath(filepath)] + def get_format(self, path_in: bytes) -> bytes | None: + cmd = self.identify_cmd + ["-format", "%[magick]", syspath(path_in)] try: return util.command_output(cmd).stdout @@ -241,10 +250,10 @@ class IMBackend(LocalBackend): def convert_format( self, - source: AnyStr, - target: AnyStr, + source: bytes, + target: bytes, deinterlaced: bool, - ) -> AnyStr: + ) -> bytes: cmd = self.convert_cmd + [ syspath(source), *(["-interlace", "none"] if deinterlaced else []), @@ -265,11 +274,11 @@ class IMBackend(LocalBackend): return self.version() > (6, 8, 7) def compare( - self, - im1: Image, - im2: Image, - compare_threshold: float, - ) -> Optional[bool]: + self, + im1: bytes, + im2: bytes, + compare_threshold: float, + ) -> bool | None: is_windows = platform.system() == "Windows" # Converting images to grayscale tends to minimize the weight @@ -355,7 +364,7 @@ class IMBackend(LocalBackend): def can_write_metadata(self) -> bool: return True - def write_metadata(self, file: AnyStr, metadata: Mapping): + def write_metadata(self, file: bytes, metadata: Mapping) -> None: assignments = list( chain.from_iterable(("-set", k, v) for k, v in metadata.items()) ) @@ -368,13 +377,13 @@ class PILBackend(LocalBackend): NAME = "PIL" @classmethod - def version(cls): + def version(cls) -> None: try: __import__("PIL", fromlist=["Image"]) except ImportError: raise LocalBackendNotAvailableError() - def __init__(self): + def __init__(self) -> None: """Initialize a wrapper around PIL for local image operations. If PIL is not available, raise an Exception. @@ -384,11 +393,11 @@ class PILBackend(LocalBackend): def resize( self, maxwidth: int, - path_in: AnyStr, - path_out: Optional[AnyStr] = None, + path_in: bytes, + path_out: bytes | None = None, quality: int = 0, max_filesize: int = 0, - ) -> AnyStr: + ) -> bytes: """Resize using Python Imaging Library (PIL). Return the output path of resized image. """ @@ -457,7 +466,7 @@ class PILBackend(LocalBackend): ) return path_in - def get_size(self, path_in: AnyStr) -> Optional[Tuple[int, int]]: + def get_size(self, path_in: bytes) -> tuple[int, int] | None: from PIL import Image try: @@ -470,10 +479,10 @@ class PILBackend(LocalBackend): return None def deinterlace( - self, - path_in: AnyStr, - path_out: Optional[AnyStr] = None, - ) -> AnyStr: + self, + path_in: bytes, + path_out: bytes | None = None, + ) -> bytes: if not path_out: path_out = get_temp_filename(__name__, "deinterlace_PIL_", path_in) @@ -487,11 +496,11 @@ class PILBackend(LocalBackend): # FIXME: Should probably issue a warning? return path_in - def get_format(self, filepath: AnyStr) -> Optional[str]: + def get_format(self, path_in: bytes) -> bytes | None: from PIL import Image, UnidentifiedImageError try: - with Image.open(syspath(filepath)) as im: + with Image.open(syspath(path_in)) as im: return im.format except ( ValueError, @@ -499,15 +508,15 @@ class PILBackend(LocalBackend): UnidentifiedImageError, FileNotFoundError, ): - log.exception("failed to detect image format for {}", filepath) + log.exception("failed to detect image format for {}", path_in) return None def convert_format( self, - source: AnyStr, - target: AnyStr, + source: bytes, + target: bytes, deinterlaced: bool, - ) -> str: + ) -> bytes: from PIL import Image, UnidentifiedImageError try: @@ -529,11 +538,11 @@ class PILBackend(LocalBackend): return False def compare( - self, - im1: Image, - im2: Image, - compare_threshold: float, - ): + self, + im1: bytes, + im2: bytes, + compare_threshold: float, + ) -> bool | None: # It is an error to call this when ArtResizer.can_compare is not True. raise NotImplementedError() @@ -541,7 +550,7 @@ class PILBackend(LocalBackend): def can_write_metadata(self) -> bool: return True - def write_metadata(self, file: AnyStr, metadata: Mapping): + def write_metadata(self, file: bytes, metadata: Mapping) -> None: from PIL import Image, PngImagePlugin # FIXME: Detect and handle other file types (currently, the only user @@ -560,18 +569,18 @@ class Shareable(type): ``MyClass()`` to construct a new object works as usual. """ - def __init__(cls, name, bases, dict): + def __init__(cls, name, bases, dict) -> None: super().__init__(name, bases, dict) cls._instance = None @property - def shared(cls) -> 'Shareable': + def shared(cls) -> Self: if cls._instance is None: cls._instance = cls() return cls._instance -BACKEND_CLASSES = [ +BACKEND_CLASSES: list[LocalBackend] = [ IMBackend, PILBackend, ] @@ -580,7 +589,7 @@ BACKEND_CLASSES = [ class ArtResizer(metaclass=Shareable): """A singleton class that performs image resizes.""" - def __init__(self): + def __init__(self) -> None: """Create a resizer object with an inferred method.""" # Check if a local backend is available, and store an instance of the # backend class. Otherwise, fallback to the web proxy. @@ -592,6 +601,15 @@ class ArtResizer(metaclass=Shareable): except LocalBackendNotAvailableError: continue else: + # FIXME: Turn WEBPROXY into a backend class as well to remove all + # the special casing. Then simply delegate all methods to the + # backends. (How does proxy_url fit in here, however?) + # Use an ABC (or maybe a typing Protocol?) for backend + # methods, such that both individual backends as well as + # ArtResizer implement it. + # It should probably be configurable which backends classes to + # consider, similar to fetchart or lyrics backends (i.e. a list + # of backends sorted by priority). log.debug("artresizer: method is WEBPROXY") self.local_method = None @@ -605,11 +623,11 @@ class ArtResizer(metaclass=Shareable): def resize( self, maxwidth: int, - path_in: AnyStr, - path_out: Optional[AnyStr]=None, + path_in: bytes, + path_out: bytes | None = None, quality: int = 0, max_filesize: int = 0, - ) -> AnyStr: + ) -> bytes: """Manipulate an image file according to the method, returning a new path. For PIL or IMAGEMAGIC methods, resizes the image to a temporary file and encodes with the specified quality level. @@ -628,10 +646,10 @@ class ArtResizer(metaclass=Shareable): return path_in def deinterlace( - self, - path_in: AnyStr, - path_out: Optional[AnyStr] = None, - ) -> AnyStr: + self, + path_in: bytes, + path_out: bytes | None = None, + ) -> bytes: """Deinterlace an image. Only available locally. @@ -660,7 +678,7 @@ class ArtResizer(metaclass=Shareable): """ return self.local_method is not None - def get_size(self, path_in: AnyStr) -> Union[Tuple[int, int], AnyStr]: + def get_size(self, path_in: bytes) -> tuple[int, int] | None: """Return the size of an image file as an int couple (width, height) in pixels. @@ -672,7 +690,7 @@ class ArtResizer(metaclass=Shareable): # FIXME: Should probably issue a warning? return path_in - def get_format(self, path_in: AnyStr) -> Optional[str]: + def get_format(self, path_in: bytes) -> bytes | None: """Returns the format of the image as a string. Only available locally. @@ -684,11 +702,11 @@ class ArtResizer(metaclass=Shareable): return None def reformat( - self, - path_in: AnyStr, - new_format: str, - deinterlaced: bool = True, - ) -> AnyStr: + self, + path_in: bytes, + new_format: str, + deinterlaced: bool = True, + ) -> bytes: """Converts image to desired format, updating its extension, but keeping the same filename. @@ -729,11 +747,11 @@ class ArtResizer(metaclass=Shareable): return False def compare( - self, - im1: Image, - im2: Image, - compare_threshold: float, - ) -> Optional[bool]: + self, + im1: bytes, + im2: bytes, + compare_threshold: float, + ) -> bool | None: """Return a boolean indicating whether two images are similar. Only available locally. @@ -753,7 +771,7 @@ class ArtResizer(metaclass=Shareable): else: return False - def write_metadata(self, file: AnyStr, metadata: Mapping): + def write_metadata(self, file: bytes, metadata: Mapping) -> None: """Write key-value metadata to the image file. Only available locally. Currently, expects the image to be a PNG file. From 720023c76ffb18f7f1c37cbc59c9bb0f121d1579 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 15 Apr 2025 23:46:17 +0200 Subject: [PATCH 5/7] artresizer: adjust code & typings to satisfy mypy Notably, this replaces the `Shareable` metaclass by a different implementation of a descriptor: We don't really need to modify class creation here, because the singleton is only available via the `shared` method, not via the constructor. Additionally, it appears that mypy can understand the new code. --- beets/util/__init__.py | 25 +++++ beets/util/artresizer.py | 218 ++++++++++++++++++++++++++------------- 2 files changed, 170 insertions(+), 73 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index e17de1f51..78b1fafc9 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -40,6 +40,7 @@ from typing import ( Any, AnyStr, Callable, + Generic, Iterable, NamedTuple, TypeVar, @@ -1041,6 +1042,30 @@ class cached_classproperty: return self.cache[owner] +T = TypeVar("T") + + +class LazySharedInstance(Generic[T]): + """A descriptor that provides access to a lazily-created shared instance of + the containing class, while calling the class constructor to construct a + new object works as usual. + """ + + _instance: T | None = None + + def __get__(self, instance: T | None, owner: type[T]) -> T: + if instance is not None: + raise RuntimeError( + "shared instances must be obtained from the class property, " + "not an instance" + ) + + if self._instance is None: + self._instance = owner() + + return self._instance + + def get_module_tempdir(module: str) -> Path: """Return the temporary directory for the given module. diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 1baca3cd0..55ea1754e 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -23,19 +23,19 @@ import os.path import platform import re import subprocess +from abc import ABC, abstractmethod +from enum import Enum from itertools import chain -from typing import TYPE_CHECKING, ClassVar, Mapping, Self +from typing import Any, ClassVar, Mapping from urllib.parse import urlencode from beets import logging, util -from beets.util import displayable_path, get_temp_filename, syspath - -if TYPE_CHECKING: - try: - from PIL import Image - except ImportError: - from typing import Any - Image = Any +from beets.util import ( + LazySharedInstance, + displayable_path, + get_temp_filename, + syspath, +) PROXY_URL = "https://images.weserv.nl/" @@ -61,13 +61,23 @@ class LocalBackendNotAvailableError(Exception): pass -_NOT_AVAILABLE = object() +# Singleton pattern that the typechecker understands: +# https://peps.python.org/pep-0484/#support-for-singleton-types-in-unions +class NotAvailable(Enum): + token = 0 -# FIXME: Turn this into an ABC with all methods that a backend should have -class LocalBackend: +_NOT_AVAILABLE = NotAvailable.token + + +class LocalBackend(ABC): NAME: ClassVar[str] + @classmethod + @abstractmethod + def version(cls) -> Any: + pass + @classmethod def available(cls) -> bool: try: @@ -76,6 +86,63 @@ class LocalBackend: except LocalBackendNotAvailableError: return False + @abstractmethod + def resize( + self, + maxwidth: int, + path_in: bytes, + path_out: bytes | None = None, + quality: int = 0, + max_filesize: int = 0, + ) -> bytes: + pass + + @abstractmethod + def get_size(self, path_in: bytes) -> tuple[int, int] | None: + pass + + @abstractmethod + def deinterlace( + self, + path_in: bytes, + path_out: bytes | None = None, + ) -> bytes: + pass + + @abstractmethod + def get_format(self, path_in: bytes) -> str | None: + pass + + @abstractmethod + def convert_format( + self, + source: bytes, + target: bytes, + deinterlaced: bool, + ) -> bytes: + pass + + @property + def can_compare(self) -> bool: + return False + + def compare( + self, + im1: bytes, + im2: bytes, + compare_threshold: float, + ) -> bool | None: + # It is an error to call this when ArtResizer.can_compare is not True. + raise NotImplementedError() + + @property + def can_write_metadata(self) -> bool: + return False + + def write_metadata(self, file: bytes, metadata: Mapping[str, str]) -> None: + # It is an error to call this when ArtResizer.can_write_metadata is not True. + raise NotImplementedError() + class IMBackend(LocalBackend): NAME = "ImageMagick" @@ -83,7 +150,7 @@ class IMBackend(LocalBackend): # These fields are used as a cache for `version()`. `_legacy` indicates # whether the modern `magick` binary is available or whether to fall back # to the old-style `convert`, `identify`, etc. commands. - _version: tuple[int, int, int] | None = None + _version: tuple[int, int, int] | NotAvailable | None = None _legacy: bool | None = None @classmethod @@ -111,11 +178,16 @@ class IMBackend(LocalBackend): ) cls._legacy = legacy - if cls._version is _NOT_AVAILABLE: + # cls._version is never None here, but mypy doesn't get that + if cls._version is _NOT_AVAILABLE or cls._version is None: raise LocalBackendNotAvailableError() else: return cls._version + convert_cmd: list[str | bytes] + identify_cmd: list[str | bytes] + compare_cmd: list[str | bytes] + def __init__(self) -> None: """Initialize a wrapper around ImageMagick for local image operations. @@ -163,7 +235,7 @@ class IMBackend(LocalBackend): # with regards to the height. # ImageMagick already seems to default to no interlace, but we include # it here for the sake of explicitness. - cmd = self.convert_cmd + [ + cmd: list[str | bytes] = self.convert_cmd + [ syspath(path_in, prefix=False), "-resize", f"{maxwidth}x>", @@ -193,7 +265,7 @@ class IMBackend(LocalBackend): return path_out def get_size(self, path_in: bytes) -> tuple[int, int] | None: - cmd = self.identify_cmd + [ + cmd: list[str | bytes] = self.identify_cmd + [ "-format", "%w %h", syspath(path_in, prefix=False), @@ -212,11 +284,17 @@ class IMBackend(LocalBackend): ) return None try: - return tuple(map(int, out.split(b" "))) + size = tuple(map(int, out.split(b" "))) except IndexError: log.warning("Could not understand IM output: {0!r}", out) return None + if len(size) != 2: + log.warning("Could not understand IM output: {0!r}", out) + return None + + return size + def deinterlace( self, path_in: bytes, @@ -239,20 +317,23 @@ class IMBackend(LocalBackend): # FIXME: Should probably issue a warning? return path_in - def get_format(self, path_in: bytes) -> bytes | None: + def get_format(self, path_in: bytes) -> str | None: cmd = self.identify_cmd + ["-format", "%[magick]", syspath(path_in)] try: - return util.command_output(cmd).stdout - except subprocess.CalledProcessError: + # Image formats should really only be ASCII strings such as "PNG", + # if anything else is returned, something is off and we return + # None for safety. + return util.command_output(cmd).stdout.decode("ascii", "strict") + except (subprocess.CalledProcessError, UnicodeError): # FIXME: Should probably issue a warning? return None def convert_format( - self, - source: bytes, - target: bytes, - deinterlaced: bool, + self, + source: bytes, + target: bytes, + deinterlaced: bool, ) -> bytes: cmd = self.convert_cmd + [ syspath(source), @@ -318,6 +399,10 @@ class IMBackend(LocalBackend): close_fds=not is_windows, ) + # help out mypy + assert convert_proc.stdout is not None + assert convert_proc.stderr is not None + # Check the convert output. We're not interested in the # standard output; that gets piped to the next stage. convert_proc.stdout.close() @@ -364,7 +449,7 @@ class IMBackend(LocalBackend): def can_write_metadata(self) -> bool: return True - def write_metadata(self, file: bytes, metadata: Mapping) -> None: + def write_metadata(self, file: bytes, metadata: Mapping[str, str]) -> None: assignments = list( chain.from_iterable(("-set", k, v) for k, v in metadata.items()) ) @@ -391,12 +476,12 @@ class PILBackend(LocalBackend): self.version() def resize( - self, - maxwidth: int, - path_in: bytes, - path_out: bytes | None = None, - quality: int = 0, - max_filesize: int = 0, + self, + maxwidth: int, + path_in: bytes, + path_out: bytes | None = None, + quality: int = 0, + max_filesize: int = 0, ) -> bytes: """Resize using Python Imaging Library (PIL). Return the output path of resized image. @@ -496,7 +581,7 @@ class PILBackend(LocalBackend): # FIXME: Should probably issue a warning? return path_in - def get_format(self, path_in: bytes) -> bytes | None: + def get_format(self, path_in: bytes) -> str | None: from PIL import Image, UnidentifiedImageError try: @@ -512,10 +597,10 @@ class PILBackend(LocalBackend): return None def convert_format( - self, - source: bytes, - target: bytes, - deinterlaced: bool, + self, + source: bytes, + target: bytes, + deinterlaced: bool, ) -> bytes: from PIL import Image, UnidentifiedImageError @@ -550,7 +635,7 @@ class PILBackend(LocalBackend): def can_write_metadata(self) -> bool: return True - def write_metadata(self, file: bytes, metadata: Mapping) -> None: + def write_metadata(self, file: bytes, metadata: Mapping[str, str]) -> None: from PIL import Image, PngImagePlugin # FIXME: Detect and handle other file types (currently, the only user @@ -558,36 +643,20 @@ class PILBackend(LocalBackend): im = Image.open(syspath(file)) meta = PngImagePlugin.PngInfo() for k, v in metadata.items(): - meta.add_text(k, v, 0) + meta.add_text(k, v, zip=False) im.save(os.fsdecode(file), "PNG", pnginfo=meta) -class Shareable(type): - """A pseudo-singleton metaclass that allows both shared and - non-shared instances. The ``MyClass.shared`` property holds a - lazily-created shared instance of ``MyClass`` while calling - ``MyClass()`` to construct a new object works as usual. - """ - - def __init__(cls, name, bases, dict) -> None: - super().__init__(name, bases, dict) - cls._instance = None - - @property - def shared(cls) -> Self: - if cls._instance is None: - cls._instance = cls() - return cls._instance - - -BACKEND_CLASSES: list[LocalBackend] = [ +BACKEND_CLASSES: list[type[LocalBackend]] = [ IMBackend, PILBackend, ] -class ArtResizer(metaclass=Shareable): - """A singleton class that performs image resizes.""" +class ArtResizer: + """A class that dispatches image operations to an available backend.""" + + local_method: LocalBackend | None def __init__(self) -> None: """Create a resizer object with an inferred method.""" @@ -613,9 +682,11 @@ class ArtResizer(metaclass=Shareable): log.debug("artresizer: method is WEBPROXY") self.local_method = None + shared: LazySharedInstance[ArtResizer] = LazySharedInstance() + @property def method(self) -> str: - if self.local: + if self.local_method is not None: return self.local_method.NAME else: return "WEBPROXY" @@ -633,7 +704,7 @@ class ArtResizer(metaclass=Shareable): temporary file and encodes with the specified quality level. For WEBPROXY, returns `path_in` unmodified. """ - if self.local: + if self.local_method is not None: return self.local_method.resize( maxwidth, path_in, @@ -654,7 +725,7 @@ class ArtResizer(metaclass=Shareable): Only available locally. """ - if self.local: + if self.local_method is not None: return self.local_method.deinterlace(path_in, path_out) else: # FIXME: Should probably issue a warning? @@ -684,18 +755,19 @@ class ArtResizer(metaclass=Shareable): Only available locally. """ - if self.local: + if self.local_method is not None: return self.local_method.get_size(path_in) else: - # FIXME: Should probably issue a warning? - return path_in + raise RuntimeError( + "image cannot be obtained without artresizer backend" + ) - def get_format(self, path_in: bytes) -> bytes | None: + def get_format(self, path_in: bytes) -> str | None: """Returns the format of the image as a string. Only available locally. """ - if self.local: + if self.local_method is not None: return self.local_method.get_format(path_in) else: # FIXME: Should probably issue a warning? @@ -712,7 +784,7 @@ class ArtResizer(metaclass=Shareable): Only available locally. """ - if not self.local: + if self.local_method is None: # FIXME: Should probably issue a warning? return path_in @@ -741,7 +813,7 @@ class ArtResizer(metaclass=Shareable): def can_compare(self) -> bool: """A boolean indicating whether image comparison is available""" - if self.local: + if self.local_method is not None: return self.local_method.can_compare else: return False @@ -756,7 +828,7 @@ class ArtResizer(metaclass=Shareable): Only available locally. """ - if self.local: + if self.local_method is not None: return self.local_method.compare(im1, im2, compare_threshold) else: # FIXME: Should probably issue a warning? @@ -766,17 +838,17 @@ class ArtResizer(metaclass=Shareable): def can_write_metadata(self) -> bool: """A boolean indicating whether writing image metadata is supported.""" - if self.local: + if self.local_method is not None: return self.local_method.can_write_metadata else: return False - def write_metadata(self, file: bytes, metadata: Mapping) -> None: + def write_metadata(self, file: bytes, metadata: Mapping[str, str]) -> None: """Write key-value metadata to the image file. Only available locally. Currently, expects the image to be a PNG file. """ - if self.local: + if self.local_method is not None: self.local_method.write_metadata(file, metadata) else: # FIXME: Should probably issue a warning? From aa49385d27c85297be270ad12bb9db3e66a10f4b Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 13 May 2025 10:48:57 +0200 Subject: [PATCH 6/7] artresizer: address review --- beets/util/__init__.py | 3 --- beets/util/artresizer.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 78b1fafc9..964051d85 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -1042,9 +1042,6 @@ class cached_classproperty: return self.cache[owner] -T = TypeVar("T") - - class LazySharedInstance(Generic[T]): """A descriptor that provides access to a lazily-created shared instance of the containing class, while calling the class constructor to construct a diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 55ea1754e..33b98c413 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -76,10 +76,15 @@ class LocalBackend(ABC): @classmethod @abstractmethod def version(cls) -> Any: + """Return the backend version if its dependencies are satisfied or + raise `LocalBackendNotAvailableError`. + """ pass @classmethod def available(cls) -> bool: + """Return `True` this backend's dependencies are satisfied and it can + be used, `False` otherwise.""" try: cls.version() return True @@ -95,10 +100,15 @@ class LocalBackend(ABC): quality: int = 0, max_filesize: int = 0, ) -> bytes: + """Resize an image to the given width and return the output path. + + On error, logs a warning and returns `path_in`. + """ pass @abstractmethod def get_size(self, path_in: bytes) -> tuple[int, int] | None: + """Return the (width, height) of the image or None if unavailable.""" pass @abstractmethod @@ -107,10 +117,15 @@ class LocalBackend(ABC): path_in: bytes, path_out: bytes | None = None, ) -> bytes: + """Remove interlacing from an image and return the output path. + + On error, logs a warning and returns `path_in`. + """ pass @abstractmethod def get_format(self, path_in: bytes) -> str | None: + """Return the image format (e.g., 'PNG') or None if undetectable.""" pass @abstractmethod @@ -120,10 +135,15 @@ class LocalBackend(ABC): target: bytes, deinterlaced: bool, ) -> bytes: + """Convert an image to a new format and return the new file path. + + On error, logs a warning and returns `source`. + """ pass @property def can_compare(self) -> bool: + """Indicate whether image comparison is supported by this backend.""" return False def compare( @@ -132,14 +152,24 @@ class LocalBackend(ABC): im2: bytes, compare_threshold: float, ) -> bool | None: + """Compare two images and return `True` if they are similar enough, or + `None` if there is an error. + + This must only be called if `self.can_compare()` returns `True`. + """ # It is an error to call this when ArtResizer.can_compare is not True. raise NotImplementedError() @property def can_write_metadata(self) -> bool: + """Indicate whether writing metadata to images is supported.""" return False def write_metadata(self, file: bytes, metadata: Mapping[str, str]) -> None: + """Write key-value metadata into the image file. + + This must only be called if `self.can_write_metadata()` returns `True`. + """ # It is an error to call this when ArtResizer.can_write_metadata is not True. raise NotImplementedError() From 26008eb9229567da03587eeb1d4be8bf409b94c0 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 13 May 2025 11:00:50 +0200 Subject: [PATCH 7/7] add example for LazySharedInstance --- beets/util/__init__.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 964051d85..02ab9cf56 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -1046,6 +1046,32 @@ class LazySharedInstance(Generic[T]): """A descriptor that provides access to a lazily-created shared instance of the containing class, while calling the class constructor to construct a new object works as usual. + + ``` + ID: int = 0 + + class Foo: + def __init__(): + global ID + + self.id = ID + ID += 1 + + def func(self): + print(self.id) + + shared: LazySharedInstance[Foo] = LazySharedInstance() + + a0 = Foo() + a1 = Foo.shared + a2 = Foo() + a3 = Foo.shared + + a0.func() # 0 + a1.func() # 1 + a2.func() # 2 + a3.func() # 1 + ``` """ _instance: T | None = None