From d3120463076158b28fe9e2823a1b84490c5ab21b Mon Sep 17 00:00:00 2001 From: Sumner Evans Date: Mon, 18 May 2020 20:57:03 -0600 Subject: [PATCH] Fixed a bunch of perf issues; handled null album/artist IDs; sort directories & albums in artist view & genres Closes #181 --- CHANGELOG.rst | 1 + sublime/adapters/adapter_base.py | 11 +- sublime/adapters/api_objects.py | 15 +- sublime/adapters/filesystem/adapter.py | 56 +++-- sublime/adapters/filesystem/models.py | 4 + sublime/adapters/manager.py | 99 +++++---- sublime/adapters/subsonic/api_objects.py | 22 +- sublime/config.py | 1 + sublime/ui/albums.py | 31 +-- sublime/ui/artists.py | 4 +- sublime/ui/browse.py | 96 +++++---- sublime/ui/common/album_with_songs.py | 32 +-- sublime/ui/main.py | 4 +- sublime/ui/player_controls.py | 2 +- sublime/ui/playlists.py | 72 +++++-- sublime/ui/state.py | 2 + sublime/ui/util.py | 44 ++-- .../adapter_tests/filesystem_adapter_tests.py | 195 ++++++++++++------ .../get_song_details_no_albumid-airsonic.json | 30 +++ ...get_song_details_no_artistid-airsonic.json | 30 +++ tests/adapter_tests/subsonic_adapter_tests.py | 44 ++++ 21 files changed, 551 insertions(+), 244 deletions(-) create mode 100644 tests/adapter_tests/mock_data/get_song_details_no_albumid-airsonic.json create mode 100644 tests/adapter_tests/mock_data/get_song_details_no_artistid-airsonic.json diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b23ad50..8494339 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -48,6 +48,7 @@ v0.9.1 keyring notification popup. * The ``NM`` library is used instead of the deprecated ``NetworkManager`` and ``NMClient``. (Contributed by @anarcat.) + * Sublime Music will crash less often due to missing dependencies. * Fixed some bugs where the state of the application wouldn't update when you deleted/downloaded songs from certain parts of the application. diff --git a/sublime/adapters/adapter_base.py b/sublime/adapters/adapter_base.py index 1f96d8d..c99d7b2 100644 --- a/sublime/adapters/adapter_base.py +++ b/sublime/adapters/adapter_base.py @@ -815,11 +815,12 @@ class CachingAdapter(Adapter): # Cache-Specific Methods # ================================================================================== @abc.abstractmethod - def get_cached_status(self, song: Song) -> SongCacheStatus: + def get_cached_statuses(self, songs: Sequence[Song]) -> Sequence[SongCacheStatus]: """ - Returns the cache status of a given song. See the :class:`SongCacheStatus` - documentation for more details about what each status means. + Returns the cache statuses for the given list of songs. See the + :class:`SongCacheStatus` documentation for more details about what each status + means. - :params song: The song to get the cache status for. - :returns: The :class:`SongCacheStatus` for the song. + :params songs: The songs to get the cache status for. + :returns: A list of :class:`SongCacheStatus` objects for each of the songs. """ diff --git a/sublime/adapters/api_objects.py b/sublime/adapters/api_objects.py index 93824a1..127e80c 100644 --- a/sublime/adapters/api_objects.py +++ b/sublime/adapters/api_objects.py @@ -29,8 +29,13 @@ class Genre(abc.ABC): class Album(abc.ABC): - id: str + """ + The ``id`` field is optional, because there are some situations where an adapter + (such as Subsonic) sends an album name, but not an album ID. + """ + name: str + id: Optional[str] artist: Optional["Artist"] cover_art: Optional[str] created: Optional[datetime] @@ -44,8 +49,14 @@ class Album(abc.ABC): class Artist(abc.ABC): - id: str + """ + The ``id`` field is optional, because there are some situations where an adapter + (such as Subsonic) sends an artist name, but not an artist ID. This especially + happens when there are multiple artists. + """ + name: str + id: Optional[str] album_count: Optional[int] artist_image_url: Optional[str] starred: Optional[datetime] diff --git a/sublime/adapters/filesystem/adapter.py b/sublime/adapters/filesystem/adapter.py index 227edd7..df9428b 100644 --- a/sublime/adapters/filesystem/adapter.py +++ b/sublime/adapters/filesystem/adapter.py @@ -1,12 +1,13 @@ import hashlib +import itertools import logging import shutil import threading from datetime import datetime from pathlib import Path -from typing import Any, cast, Dict, Optional, Sequence, Set, Union +from typing import Any, cast, Dict, Optional, Sequence, Set, Tuple, Union -from peewee import fn +from peewee import fn, prefetch from sublime.adapters import api_objects as API @@ -137,8 +138,12 @@ class FilesystemAdapter(CachingAdapter): model: Any, cache_key: CachingAdapter.CachedDataKey, ignore_cache_miss: bool = False, + where_clauses: Tuple[Any, ...] = None, ) -> Sequence: - result = list(model.select()) + result = model.select() + if where_clauses is not None: + result = result.where(*where_clauses) + if self.is_cache and not ignore_cache_miss: # Determine if the adapter has ingested data for this key before, and if # not, cache miss. @@ -192,10 +197,11 @@ class FilesystemAdapter(CachingAdapter): # Data Retrieval Methods # ================================================================================== - def get_cached_status(self, song: API.Song) -> SongCacheStatus: - try: - song_model = self.get_song_details(song.id) - file = song_model.file + def get_cached_statuses( + self, songs: Sequence[API.Song] + ) -> Sequence[SongCacheStatus]: + def compute_song_cache_status(song: models.Song) -> SongCacheStatus: + file = song.file if self._compute_song_filename(file).exists(): if file.valid: if file.cache_permanently: @@ -204,10 +210,22 @@ class FilesystemAdapter(CachingAdapter): # The file is on disk, but marked as stale. return SongCacheStatus.CACHED_STALE + return SongCacheStatus.NOT_CACHED + + try: + # TODO batch getting song details for songs that aren't already a song + # model? + song_models = [ + song + if isinstance(song, models.Song) + else self.get_song_details(song.id) + for song in songs + ] + return [compute_song_cache_status(s) for s in song_models] except Exception: pass - return SongCacheStatus.NOT_CACHED + return list(itertools.repeat(SongCacheStatus.NOT_CACHED, len(songs))) _playlists = None @@ -270,6 +288,7 @@ class FilesystemAdapter(CachingAdapter): models.Artist, CachingAdapter.CachedDataKey.ARTISTS, ignore_cache_miss=ignore_cache_miss, + where_clauses=(~(models.Artist.id.startswith("invalid:")),), ) def get_artist(self, artist_id: str) -> API.Artist: @@ -300,7 +319,9 @@ class FilesystemAdapter(CachingAdapter): # If we haven't ever cached the query result, try to construct one, and return # it as a CacheMissError result. - sql_query = models.Album.select() + sql_query = models.Album.select().where( + ~(models.Album.id.startswith("invalid:")) + ) Type = AlbumSearchQuery.Type if query.type == Type.GENRE: @@ -328,7 +349,10 @@ class FilesystemAdapter(CachingAdapter): def get_all_albums(self) -> Sequence[API.Album]: return self._get_list( - models.Album, CachingAdapter.CachedDataKey.ALBUMS, ignore_cache_miss=True + models.Album, + CachingAdapter.CachedDataKey.ALBUMS, + ignore_cache_miss=True, + where_clauses=(~(models.Album.id.startswith("invalid:")),), ) def get_album(self, album_id: str) -> API.Album: @@ -371,6 +395,9 @@ class FilesystemAdapter(CachingAdapter): # Data Ingestion Methods # ================================================================================== + def _strhash(self, string: str) -> str: + return hashlib.sha1(bytes(string, "utf8")).hexdigest() + def ingest_new_data( self, data_key: CachingAdapter.CachedDataKey, param: Optional[str], data: Any, ): @@ -474,8 +501,9 @@ class FilesystemAdapter(CachingAdapter): def ingest_album_data( api_album: API.Album, exclude_artist: bool = False ) -> models.Album: + album_id = api_album.id or f"invalid:{self._strhash(api_album.name)}" album_data = { - "id": api_album.id, + "id": album_id, "name": api_album.name, "created": getattr(api_album, "created", None), "duration": getattr(api_album, "duration", None), @@ -522,8 +550,9 @@ class FilesystemAdapter(CachingAdapter): ] ).on_conflict_replace().execute() + artist_id = api_artist.id or f"invalid:{self._strhash(api_artist.name)}" artist_data = { - "id": api_artist.id, + "id": artist_id, "name": api_artist.name, "album_count": getattr(api_artist, "album_count", None), "starred": getattr(api_artist, "starred", None), @@ -542,7 +571,7 @@ class FilesystemAdapter(CachingAdapter): } artist, created = models.Artist.get_or_create( - id=api_artist.id, defaults=artist_data + id=artist_id, defaults=artist_data ) if not created: @@ -661,6 +690,7 @@ class FilesystemAdapter(CachingAdapter): ingest_artist_data(a) models.Artist.delete().where( models.Artist.id.not_in([a.id for a in data]) + & ~models.Artist.id.startswith("invalid") ).execute() elif data_key == KEYS.COVER_ART_FILE: diff --git a/sublime/adapters/filesystem/models.py b/sublime/adapters/filesystem/models.py index 634d59d..ee64e22 100644 --- a/sublime/adapters/filesystem/models.py +++ b/sublime/adapters/filesystem/models.py @@ -132,6 +132,10 @@ class Directory(BaseModel): ) + list(Song.select().where(Song.parent_id == self.id)) return self._children + @children.setter + def children(self, value: List[Union["Directory", "Song"]]): + self._children = value + class Song(BaseModel): id = TextField(unique=True, primary_key=True) diff --git a/sublime/adapters/manager.py b/sublime/adapters/manager.py index 731f7dc..681c92c 100644 --- a/sublime/adapters/manager.py +++ b/sublime/adapters/manager.py @@ -1,4 +1,5 @@ import hashlib +import itertools import logging import os import random @@ -7,6 +8,7 @@ import threading from concurrent.futures import Future, ThreadPoolExecutor from dataclasses import dataclass from datetime import timedelta +from functools import partial from pathlib import Path from time import sleep from typing import ( @@ -26,6 +28,7 @@ from typing import ( ) import requests +from gi.repository import GLib from sublime.config import AppConfiguration @@ -318,7 +321,9 @@ class AdapterManager: return Result(future_fn) @staticmethod - def _create_download_fn(uri: str, id: str) -> Callable[[], str]: + def _create_download_fn( + uri: str, id: str, before_download: Callable[[], None] = None, + ) -> Callable[[], str]: """ Create a function to download the given URI to a temporary file, and return the filename. The returned function will spin-loop if the resource is already being @@ -337,6 +342,9 @@ class AdapterManager: resource_downloading = True AdapterManager.current_download_ids.add(id) + if before_download: + before_download() + # TODO (#122): figure out how to retry if the other request failed. if resource_downloading: logging.info(f"{uri} already being downloaded.") @@ -688,7 +696,7 @@ class AdapterManager: @staticmethod def get_cover_art_filename( cover_art_id: str = None, - before_download: Callable[[], None] = lambda: None, + before_download: Callable[[], None] = None, force: bool = False, # TODO: rename to use_ground_truth_adapter? allow_download: bool = True, ) -> Result[str]: @@ -731,15 +739,13 @@ class AdapterManager: return Result(existing_cover_art_filename) # TODO: make _get_from_cache_or_ground_truth work with downloading - if before_download: - before_download() - future: Result[str] = Result( AdapterManager._create_download_fn( AdapterManager._instance.ground_truth_adapter.get_cover_art_uri( cover_art_id, AdapterManager._get_scheme(), size=300 ), cover_art_id, + before_download, ), is_download=True, default_value=existing_cover_art_filename, @@ -835,6 +841,7 @@ class AdapterManager: song_id, AdapterManager._get_scheme() ), song_id, + lambda: before_download(song_id), )() AdapterManager._instance.caching_adapter.ingest_new_data( CachingAdapter.CachedDataKey.SONG_FILE, @@ -963,20 +970,23 @@ class AdapterManager: if not AdapterManager._any_adapter_can_do("get_ignored_articles"): return set() try: - return AdapterManager._get_from_cache_or_ground_truth( + ignored_articles: Set[str] = AdapterManager._get_from_cache_or_ground_truth( "get_ignored_articles", None, use_ground_truth_adapter=use_ground_truth_adapter, cache_key=CachingAdapter.CachedDataKey.IGNORED_ARTICLES, ).result() + return set(map(str.lower, ignored_articles)) except Exception: logging.exception("Failed to retrieve ignored_articles") return set() @staticmethod - def _strip_ignored_articles(use_ground_truth_adapter: bool, string: str) -> str: + def _strip_ignored_articles( + use_ground_truth_adapter: bool, ignored_articles: Set[str], string: str + ) -> str: first_word, *rest = string.split(maxsplit=1) - if first_word in AdapterManager._get_ignored_articles(use_ground_truth_adapter): + if first_word in ignored_articles: return rest[0] return string @@ -988,12 +998,15 @@ class AdapterManager: key: Callable[[_S], str], use_ground_truth_adapter: bool = False, ) -> List[_S]: - return sorted( - it, - key=lambda x: AdapterManager._strip_ignored_articles( - use_ground_truth_adapter, key(x).lower() - ), + ignored_articles = AdapterManager._get_ignored_articles( + use_ground_truth_adapter ) + strip_fn = partial( + AdapterManager._strip_ignored_articles, + use_ground_truth_adapter, + ignored_articles, + ) + return sorted(it, key=lambda x: strip_fn(key(x).lower())) @staticmethod def get_artist( @@ -1060,36 +1073,30 @@ class AdapterManager: before_download: Callable[[], None] = lambda: None, force: bool = False, ) -> Result[Directory]: - return AdapterManager._get_from_cache_or_ground_truth( - "get_directory", - directory_id, - before_download=before_download, - use_ground_truth_adapter=force, - cache_key=CachingAdapter.CachedDataKey.DIRECTORY, - ) + def do_get_directory() -> Directory: + directory: Directory = AdapterManager._get_from_cache_or_ground_truth( + "get_directory", + directory_id, + before_download=before_download, + use_ground_truth_adapter=force, + cache_key=CachingAdapter.CachedDataKey.DIRECTORY, + ).result() + directory.children = AdapterManager.sort_by_ignored_articles( + directory.children, + key=lambda c: cast(Directory, c).name or "" + if hasattr(c, "name") + else cast(Song, c).title, + use_ground_truth_adapter=force, + ) + return directory + + return Result(do_get_directory) # Play Queue @staticmethod def get_play_queue() -> Result[Optional[PlayQueue]]: assert AdapterManager._instance - future: Result[ - Optional[PlayQueue] - ] = AdapterManager._create_ground_truth_result("get_play_queue") - - if AdapterManager._instance.caching_adapter: - - def future_finished(f: Result): - assert AdapterManager._instance - assert AdapterManager._instance.caching_adapter - if play_queue := f.result(): - for song in play_queue.songs: - AdapterManager._instance.caching_adapter.ingest_new_data( - CachingAdapter.CachedDataKey.SONG, song.id, song - ) - - future.add_done_callback(future_finished) - - return future + return AdapterManager._create_ground_truth_result("get_play_queue") @staticmethod def save_play_queue( @@ -1192,12 +1199,18 @@ class AdapterManager: # Cache Status Methods # ================================================================================== @staticmethod - def get_cached_status(song: Song) -> SongCacheStatus: + def get_cached_statuses(songs: Sequence[Song]) -> Sequence[SongCacheStatus]: assert AdapterManager._instance if not AdapterManager._instance.caching_adapter: - return SongCacheStatus.NOT_CACHED + return list(itertools.repeat(SongCacheStatus.NOT_CACHED, len(songs))) - if song.id in AdapterManager.current_download_ids: - return SongCacheStatus.DOWNLOADING + cache_statuses = [] + for song, cache_status in zip( + songs, AdapterManager._instance.caching_adapter.get_cached_statuses(songs) + ): + if song.id in AdapterManager.current_download_ids: + cache_statuses.append(SongCacheStatus.DOWNLOADING) + else: + cache_statuses.append(cache_status) - return AdapterManager._instance.caching_adapter.get_cached_status(song) + return cache_statuses diff --git a/sublime/adapters/subsonic/api_objects.py b/sublime/adapters/subsonic/api_objects.py index 78052d0..3a00182 100644 --- a/sublime/adapters/subsonic/api_objects.py +++ b/sublime/adapters/subsonic/api_objects.py @@ -47,8 +47,8 @@ class Genre(SublimeAPI.Genre): @dataclass_json(letter_case=LetterCase.CAMEL) @dataclass class Album(SublimeAPI.Album): - id: str name: str + id: Optional[str] cover_art: Optional[str] = None song_count: Optional[int] = None year: Optional[int] = None @@ -73,8 +73,8 @@ class Album(SublimeAPI.Album): # Initialize the cross-references self.artist = ( None - if not self.artist_id - else ArtistAndArtistInfo(self.artist_id, self._artist) + if not self.artist_id and not self._artist + else ArtistAndArtistInfo(id=self.artist_id, name=self._artist) ) self.genre = None if not self._genre else Genre(self._genre) @@ -82,9 +82,8 @@ class Album(SublimeAPI.Album): @dataclass_json(letter_case=LetterCase.CAMEL) @dataclass class ArtistAndArtistInfo(SublimeAPI.Artist): - id: str = field(init=False) - _id: Optional[str] = field(metadata=config(field_name="id")) name: str + id: Optional[str] albums: List[Album] = field( default_factory=list, metadata=config(field_name="album") ) @@ -106,11 +105,6 @@ class ArtistAndArtistInfo(SublimeAPI.Artist): return hashlib.sha1(bytes(string, "utf8")).hexdigest() def __post_init__(self): - self.id = ( - self._id - if self._id is not None - else ArtistAndArtistInfo._strhash(self.name) - ) self.album_count = self.album_count or len(self.albums) if not self.artist_image_url: self.artist_image_url = self.cover_art @@ -198,10 +192,12 @@ class Song(SublimeAPI.Song, DataClassJsonMixin): self.parent_id = (self.parent_id or "root") if self.id != "root" else None self.artist = ( None - if not self.artist_id - else ArtistAndArtistInfo(self.artist_id, self._artist) + if not self._artist + else ArtistAndArtistInfo(id=self.artist_id, name=self._artist) + ) + self.album = ( + None if not self._album else Album(id=self.album_id, name=self._album) ) - self.album = None if not self.album_id else Album(self.album_id, self._album) self.genre = None if not self._genre else Genre(self._genre) diff --git a/sublime/config.py b/sublime/config.py index cbdb0d1..6f30fe6 100644 --- a/sublime/config.py +++ b/sublime/config.py @@ -158,6 +158,7 @@ class AppConfiguration: # Do the import in the function to avoid circular imports. from sublime.adapters import AdapterManager + AdapterManager.reset(self) @property diff --git a/sublime/ui/albums.py b/sublime/ui/albums.py index c1d6359..3eda1a3 100644 --- a/sublime/ui/albums.py +++ b/sublime/ui/albums.py @@ -95,7 +95,6 @@ class AlbumsPanel(Gtk.Box): ) actionbar.pack_start(self.alphabetical_type_combo) - # TODO: Sort genre combo box alphabetically? self.genre_combo, self.genre_combo_store = self.make_combobox( (), self.on_genre_change ) @@ -203,9 +202,8 @@ class AlbumsPanel(Gtk.Box): def get_genres_done(f: Result): try: - new_store = [ - (genre.name, genre.name, True) for genre in (f.result() or []) - ] + genre_names = map(lambda g: g.name, f.result() or []) + new_store = [(name, name, True) for name in sorted(genre_names)] util.diff_song_store(self.genre_combo_store, new_store) @@ -497,10 +495,11 @@ class AlbumsGrid(Gtk.Overlay): @property def id(self) -> str: + assert self.album.id return self.album.id def __repr__(self) -> str: - return f"" + return f"" current_query: AlbumSearchQuery = AlbumSearchQuery(AlbumSearchQuery.Type.RANDOM) current_models: List[_AlbumModel] = [] @@ -538,7 +537,6 @@ class AlbumsGrid(Gtk.Overlay): row_spacing=5, column_spacing=5, margin_top=5, - # margin_bottom=5, homogeneous=True, valign=Gtk.Align.START, halign=Gtk.Align.CENTER, @@ -560,7 +558,6 @@ class AlbumsGrid(Gtk.Overlay): self.detail_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL) self.detail_box.pack_start(Gtk.Box(), True, True, 0) - # TODO wrap in revealer? self.detail_box_inner = Gtk.Box() self.detail_box.pack_start(self.detail_box_inner, False, False, 0) @@ -745,7 +742,8 @@ class AlbumsGrid(Gtk.Overlay): self.emit("cover-clicked", self.list_store_bottom[selected_index].id) def on_grid_resize(self, flowbox: Gtk.FlowBox, rect: Gdk.Rectangle): - # TODO (#124): this doesn't work with themes that add extra padding. + # TODO (#124): this doesn't work at all consistency, especially with themes that + # add extra padding. # 200 + (10 * 2) + (5 * 2) = 230 # picture + (padding * 2) + (margin * 2) new_items_per_row = min((rect.width // 230), 10) @@ -755,7 +753,7 @@ class AlbumsGrid(Gtk.Overlay): self.items_per_row * 230 - 10, -1, ) - self.reflow_grids() + self.reflow_grids(force_reload_from_master=True) # Helper Methods # ========================================================================= @@ -829,7 +827,11 @@ class AlbumsGrid(Gtk.Overlay): page_offset = self.page_size * self.page # Calculate the look-at window. - models = models if models is not None else self.list_store_top + models = ( + models + if models is not None + else list(self.list_store_top) + list(self.list_store_bottom) + ) if self.sort_dir == "ascending": window = models[page_offset : (page_offset + self.page_size)] else: @@ -852,10 +854,8 @@ class AlbumsGrid(Gtk.Overlay): self.detail_box_revealer.set_reveal_child(False) if force_reload_from_master: - # Just remove everything and re-add all of the items. - # TODO (#114): make this smarter somehow to avoid flicker. Maybe - # change this so that it removes one by one and adds back one by - # one. + # Just remove everything and re-add all of the items. It's not worth trying + # to diff in this case. self.list_store_top.splice( 0, len(self.list_store_top), window[:entries_before_fold], ) @@ -863,6 +863,8 @@ class AlbumsGrid(Gtk.Overlay): 0, len(self.list_store_bottom), window[entries_before_fold:], ) elif self.currently_selected_index or entries_before_fold != self.page_size: + # This case handles when the selection changes and the entries need to be + # re-allocated to the top and bottom grids # Move entries between the two stores. top_store_len = len(self.list_store_top) bottom_store_len = len(self.list_store_bottom) @@ -909,6 +911,7 @@ class AlbumsGrid(Gtk.Overlay): # to add another flag for this function. else: self.grid_top.unselect_all() + self.grid_bottom.unselect_all() # If we had to change the page to select the index, then update the window. It # should basically be a no-op. diff --git a/sublime/ui/artists.py b/sublime/ui/artists.py index 76bcfdc..72a6648 100644 --- a/sublime/ui/artists.py +++ b/sublime/ui/artists.py @@ -96,7 +96,6 @@ class ArtistList(Gtk.Box): margin=12, halign=Gtk.Align.START, ellipsize=Pango.EllipsizeMode.END, - max_width_chars=30, ) ) row.show_all() @@ -414,6 +413,7 @@ class ArtistDetailPanel(Gtk.ScrolledWindow): def get_artist_song_ids(self) -> List[str]: songs = [] for album in AdapterManager.get_artist(self.artist_id).result().albums or []: + assert album.id album_songs = AdapterManager.get_album(album.id).result() for song in album_songs.songs or []: songs.append(song.id) @@ -455,7 +455,7 @@ class AlbumsListWithSongs(Gtk.Overlay): self.spinner.hide() return - new_albums = list(artist.albums or []) + new_albums = sorted(artist.albums or [], key=lambda a: a.name) if self.albums == new_albums: # Just go through all of the colidren and update them. diff --git a/sublime/ui/browse.py b/sublime/ui/browse.py index da7f5ef..83bcb36 100644 --- a/sublime/ui/browse.py +++ b/sublime/ui/browse.py @@ -1,5 +1,5 @@ from functools import partial -from typing import Any, cast, Optional, Tuple +from typing import Any, cast, List, Optional, Tuple from gi.repository import Gdk, Gio, GLib, GObject, Gtk, Pango @@ -61,9 +61,7 @@ class BrowsePanel(Gtk.Overlay): return # TODO pass order token here? - self.root_directory_listing.update( - id_stack.result(), app_config, force=force, - ) + self.root_directory_listing.update(id_stack.result(), app_config, force) self.spinner.hide() def calculate_path() -> Tuple[str, ...]: @@ -253,8 +251,9 @@ class MusicDirectoryList(Gtk.Box): self.directory_id, force=force, order_token=self.update_order_token, ) - # TODO this causes probalems because the callback may try and call an object that - # doesn't exist anymore since we delete these panels a lot. + _current_child_ids: List[str] = [] + songs: List[API.Song] = [] + @util.async_callback( AdapterManager.get_directory, before_download=lambda self: self.loading_indicator.show(), @@ -270,49 +269,70 @@ class MusicDirectoryList(Gtk.Box): if order_token != self.update_order_token: return - new_directories_store = [] - new_songs_store = [] + # This doesn't look efficient, since it's doing a ton of passses over the data, + # but there is some annoying memory overhead for generating the stores to diff, + # so we are short-circuiting by checking to see if any of the the IDs have + # changed. + # + # The entire algorithm ends up being O(2n), but the first loop is very tight, + # and the expensive parts of the second loop are avoided if the IDs haven't + # changed. + children_ids, children = [], [] selected_dir_idx = None + for i, c in enumerate(directory.children): + if i >= len(self._current_child_ids) or c.id != self._current_child_ids[i]: + force = True - for el in directory.children: - if hasattr(el, "children"): - new_directories_store.append( - MusicDirectoryList.DrilldownElement(cast(API.Directory, el)) + if c.id == self.selected_id: + selected_dir_idx = i + + children_ids.append(c.id) + children.append(c) + + if force: + new_directories_store = [] + self._current_child_ids = children_ids + + self.songs = [] + for el in children: + if hasattr(el, "children"): + new_directories_store.append( + MusicDirectoryList.DrilldownElement(cast(API.Directory, el)) + ) + else: + self.songs.append(cast(API.Song, el)) + + util.diff_model_store( + self.drilldown_directories_store, new_directories_store + ) + + new_songs_store = [ + [ + status_icon, + util.esc(song.title), + util.format_song_duration(song.duration), + song.id, + ] + for status_icon, song in zip( + util.get_cached_status_icons(self.songs), self.songs ) - else: - song = cast(API.Song, el) - new_songs_store.append( - [ - util.get_cached_status_icon(song), - util.esc(song.title), - util.format_song_duration(song.duration), - song.id, - ] + ] + else: + new_songs_store = [ + [status_icon] + song_model[1:] + for status_icon, song_model in zip( + util.get_cached_status_icons(self.songs), self.directory_song_store ) + ] - # TODO figure out a way to push the sorting into the AdapterManager. - # start = time() - new_directories_store = AdapterManager.sort_by_ignored_articles( - new_directories_store, key=lambda d: d.name, use_ground_truth_adapter=force - ) - new_songs_store = AdapterManager.sort_by_ignored_articles( - new_songs_store, key=lambda s: s[1], use_ground_truth_adapter=force - ) - # print("CONSTRUCTING STORE TOOK", time() - start, force) - - for idx, el in enumerate(new_directories_store): - if el.id == self.selected_id: - selected_dir_idx = idx - - util.diff_model_store(self.drilldown_directories_store, new_directories_store) util.diff_song_store(self.directory_song_store, new_songs_store) - if len(new_directories_store) == 0: + if len(self.drilldown_directories_store) == 0: self.list.hide() else: self.list.show() - if len(new_songs_store) == 0: + if len(self.directory_song_store) == 0: self.directory_song_list.hide() self.scroll_window.set_min_content_width(275) else: diff --git a/sublime/ui/common/album_with_songs.py b/sublime/ui/common/album_with_songs.py index 011e8ba..3446bc2 100644 --- a/sublime/ui/common/album_with_songs.py +++ b/sublime/ui/common/album_with_songs.py @@ -120,12 +120,11 @@ class AlbumWithSongs(Gtk.Box): ) ) - self.album_song_store = Gtk.ListStore( - str, str, str, str, # cache status, title, duration, song ID - ) + self.loading_indicator_container = Gtk.Box() + album_details.add(self.loading_indicator_container) - self.loading_indicator = Gtk.Spinner(name="album-list-song-list-spinner") - album_details.add(self.loading_indicator) + # cache status, title, duration, song ID + self.album_song_store = Gtk.ListStore(str, str, str, str) self.album_songs = Gtk.TreeView( model=self.album_song_store, @@ -244,11 +243,16 @@ class AlbumWithSongs(Gtk.Box): def set_loading(self, loading: bool): if loading: - self.loading_indicator.start() - self.loading_indicator.show() + if len(self.loading_indicator_container.get_children()) == 0: + self.loading_indicator_container.pack_start(Gtk.Box(), True, True, 0) + spinner = Gtk.Spinner(name="album-list-song-list-spinner") + spinner.start() + self.loading_indicator_container.add(spinner) + self.loading_indicator_container.pack_start(Gtk.Box(), True, True, 0) + + self.loading_indicator_container.show_all() else: - self.loading_indicator.stop() - self.loading_indicator.hide() + self.loading_indicator_container.hide() @util.async_callback( AdapterManager.get_album, @@ -264,12 +268,14 @@ class AlbumWithSongs(Gtk.Box): ): new_store = [ [ - util.get_cached_status_icon(song), + cached_status, util.esc(song.title), util.format_song_duration(song.duration), song.id, ] - for song in list(album.songs or []) + for cached_status, song in zip( + util.get_cached_status_icons(list(album.songs or [])), album.songs or [] + ) ] song_ids = [song[-1] for song in new_store] @@ -284,4 +290,6 @@ class AlbumWithSongs(Gtk.Box): self.add_to_queue_btn.set_action_name("app.play-next") util.diff_song_store(self.album_song_store, new_store) - self.loading_indicator.hide() + + # Have to idle_add here so that his happens after the component is rendered. + self.set_loading(False) diff --git a/sublime/ui/main.py b/sublime/ui/main.py index a4e51a2..4e51f88 100644 --- a/sublime/ui/main.py +++ b/sublime/ui/main.py @@ -395,7 +395,7 @@ class MainWindow(Gtk.ApplicationWindow): f"{util.esc(song.title)}", util.esc(song.artist.name if song.artist else None), ) - assert song.album + assert song.album and song.album.id self.song_results.add( self._create_search_result_row( label_text, "album", song.album.id, song.cover_art @@ -412,6 +412,7 @@ class MainWindow(Gtk.ApplicationWindow): f"{util.esc(album.name)}", util.esc(album.artist.name if album.artist else None), ) + assert album.id self.album_results.add( self._create_search_result_row( label_text, "album", album.id, album.cover_art @@ -425,6 +426,7 @@ class MainWindow(Gtk.ApplicationWindow): self._remove_all_from_widget(self.artist_results) for artist in search_results.artists: label_text = util.esc(artist.name) + assert artist.id self.artist_results.add( self._create_search_result_row( label_text, "artist", artist.id, artist.artist_image_url diff --git a/sublime/ui/player_controls.py b/sublime/ui/player_controls.py index 41d0333..fd14436 100644 --- a/sublime/ui/player_controls.py +++ b/sublime/ui/player_controls.py @@ -477,7 +477,7 @@ class PlayerControls(Gtk.ActionBar): "refresh-window", { "current_song_index": currently_playing_index, - "play_queue": [s[-1] for s in self.play_queue_store], + "play_queue": tuple(s[-1] for s in self.play_queue_store), }, False, ) diff --git a/sublime/ui/playlists.py b/sublime/ui/playlists.py index b89750c..7585df2 100644 --- a/sublime/ui/playlists.py +++ b/sublime/ui/playlists.py @@ -1,12 +1,11 @@ from functools import lru_cache from random import randint -from typing import Any, Iterable, List, Tuple +from typing import Any, cast, Iterable, List, Tuple from fuzzywuzzy import process from gi.repository import Gdk, Gio, GLib, GObject, Gtk, Pango -from sublime.adapters import AdapterManager -from sublime.adapters.api_objects import Playlist, PlaylistDetails +from sublime.adapters import AdapterManager, api_objects as API from sublime.config import AppConfiguration from sublime.ui import util from sublime.ui.common import ( @@ -177,7 +176,7 @@ class PlaylistList(Gtk.Box): ) def update_list( self, - playlists: List[Playlist], + playlists: List[API.Playlist], app_config: AppConfiguration, force: bool = False, order_token: int = None, @@ -436,6 +435,9 @@ class PlaylistDetailPanel(Gtk.Overlay): order_token=self.update_playlist_view_order_token, ) + _current_song_ids: List[str] = [] + songs: List[API.Song] = [] + @util.async_callback( AdapterManager.get_playlist_details, before_download=lambda self: self.show_loading_all(), @@ -443,7 +445,7 @@ class PlaylistDetailPanel(Gtk.Overlay): ) def update_playlist_view( self, - playlist: PlaylistDetails, + playlist: API.PlaylistDetails, app_config: AppConfiguration = None, force: bool = False, order_token: int = None, @@ -469,27 +471,53 @@ class PlaylistDetailPanel(Gtk.Overlay): self.playlist_stats.set_markup(self._format_stats(playlist)) # Update the artwork. - self.update_playlist_artwork( - playlist.cover_art, order_token=order_token, - ) + self.update_playlist_artwork(playlist.cover_art, order_token=order_token) # Update the song list model. This requires some fancy diffing to # update the list. self.editing_playlist_song_list = True - new_store = [ - [ - util.get_cached_status_icon(song), - song.title, - album.name if (album := song.album) else None, - artist.name if (artist := song.artist) else None, - util.format_song_duration(song.duration), - song.id, - ] - for song in playlist.songs - ] + # This doesn't look efficient, since it's doing a ton of passses over the data, + # but there is some annoying memory overhead for generating the stores to diff, + # so we are short-circuiting by checking to see if any of the the IDs have + # changed. + # + # The entire algorithm ends up being O(2n), but the first loop is very tight, + # and the expensive parts of the second loop are avoided if the IDs haven't + # changed. + song_ids, songs = [], [] + for i, c in enumerate(playlist.songs): + if i >= len(self._current_song_ids) or c.id != self._current_song_ids[i]: + force = True + song_ids.append(c.id) + songs.append(c) - util.diff_song_store(self.playlist_song_store, new_store) + if force: + self._current_song_ids = song_ids + self.songs = [cast(API.Song, s) for s in songs] + + new_songs_store = [ + [ + status_icon, + song.title, + album.name if (album := song.album) else None, + artist.name if (artist := song.artist) else None, + util.format_song_duration(song.duration), + song.id, + ] + for status_icon, song in zip( + util.get_cached_status_icons(self.songs), self.songs + ) + ] + else: + new_songs_store = [ + [status_icon] + song_model[1:] + for status_icon, song_model in zip( + util.get_cached_status_icons(self.songs), self.playlist_song_store + ) + ] + + util.diff_song_store(self.playlist_song_store, new_songs_store) self.editing_playlist_song_list = False @@ -713,7 +741,7 @@ class PlaylistDetailPanel(Gtk.Overlay): @util.async_callback(AdapterManager.get_playlist_details) def _update_playlist_order( - self, playlist: PlaylistDetails, app_config: AppConfiguration, **kwargs, + self, playlist: API.PlaylistDetails, app_config: AppConfiguration, **kwargs, ): self.playlist_view_loading_box.show_all() update_playlist_future = AdapterManager.update_playlist( @@ -730,7 +758,7 @@ class PlaylistDetailPanel(Gtk.Overlay): ) ) - def _format_stats(self, playlist: PlaylistDetails) -> str: + def _format_stats(self, playlist: API.PlaylistDetails) -> str: created_date_text = "" if playlist.created: created_date_text = f" on {playlist.created.strftime('%B %d, %Y')}" diff --git a/sublime/ui/state.py b/sublime/ui/state.py index f103951..440f996 100644 --- a/sublime/ui/state.py +++ b/sublime/ui/state.py @@ -68,12 +68,14 @@ class UIState: state = self.__dict__.copy() del state["song_stream_cache_progress"] del state["current_notification"] + del state["playing"] return state def __setstate__(self, state: Dict[str, Any]): self.__dict__.update(state) self.song_stream_cache_progress = None self.current_notification = None + self.playing = False class _DefaultGenre(Genre): def __init__(self): diff --git a/sublime/ui/util.py b/sublime/ui/util.py index 61a72d0..afd59f5 100644 --- a/sublime/ui/util.py +++ b/sublime/ui/util.py @@ -35,12 +35,13 @@ def format_song_duration(duration_secs: Union[int, timedelta, None]) -> str: >>> format_song_duration(None) '-:--' """ - # TODO remove int compatibility eventually? if isinstance(duration_secs, timedelta): duration_secs = round(duration_secs.total_seconds()) if duration_secs is None: return "-:--" + duration_secs = max(duration_secs, 0) + return f"{duration_secs // 60}:{duration_secs % 60:02}" @@ -121,14 +122,16 @@ def dot_join(*items: Any) -> str: return " • ".join(map(str, filter(lambda x: x is not None, items))) -def get_cached_status_icon(song: Song) -> str: +def get_cached_status_icons(songs: List[Song]) -> List[str]: cache_icon = { - SongCacheStatus.NOT_CACHED: "", SongCacheStatus.CACHED: "folder-download-symbolic", SongCacheStatus.PERMANENTLY_CACHED: "view-pin-symbolic", SongCacheStatus.DOWNLOADING: "emblem-synchronizing-symbolic", } - return cache_icon[AdapterManager.get_cached_status(song)] + return [ + cache_icon.get(cache_status, "") + for cache_status in AdapterManager.get_cached_statuses(songs) + ] def _parse_diff_location(location: str) -> Tuple: @@ -223,22 +226,22 @@ def show_song_popover( # Determine if we should enable the download button. download_sensitive, remove_download_sensitive = False, False albums, artists, parents = set(), set(), set() - for song_id in song_ids: + # TODO lazy load these + song_details = [ + AdapterManager.get_song_details(song_id).result() for song_id in song_ids + ] + song_cache_statuses = AdapterManager.get_cached_statuses(song_details) + for song, status in zip(song_details, song_cache_statuses): # TODO lazy load these - details = AdapterManager.get_song_details(song_id).result() - status = AdapterManager.get_cached_status(details) - albums.add(album.id if (album := details.album) else None) - artists.add(artist.id if (artist := details.artist) else None) - parents.add(parent_id if (parent_id := details.parent_id) else None) + albums.add(album.id if (album := song.album) else None) + artists.add(artist.id if (artist := song.artist) else None) + parents.add(parent_id if (parent_id := song.parent_id) else None) - if download_sensitive or status == SongCacheStatus.NOT_CACHED: - download_sensitive = True - - if remove_download_sensitive or status in ( + download_sensitive |= status == SongCacheStatus.NOT_CACHED + remove_download_sensitive |= status in ( SongCacheStatus.CACHED, SongCacheStatus.PERMANENTLY_CACHED, - ): - remove_download_sensitive = True + ) go_to_album_button = Gtk.ModelButton( text="Go to album", action_name="app.go-to-album" @@ -386,9 +389,14 @@ def async_callback( ) if is_immediate: - GLib.idle_add(fn) - else: + # The data is available now, no need to wait for the future to + # finish, and no need to incur the overhead of adding to the GLib + # event queue. fn() + else: + # We don'h have the data, and we have to idle add so that we don't + # seg fault GTK. + GLib.idle_add(fn) result: Result = future_fn( *args, before_download=on_before_download, force=force, **kwargs, diff --git a/tests/adapter_tests/filesystem_adapter_tests.py b/tests/adapter_tests/filesystem_adapter_tests.py index e7ba830..7b1516e 100644 --- a/tests/adapter_tests/filesystem_adapter_tests.py +++ b/tests/adapter_tests/filesystem_adapter_tests.py @@ -9,7 +9,12 @@ import pytest from peewee import SelectQuery -from sublime.adapters import api_objects as SublimeAPI, CacheMissError, SongCacheStatus +from sublime.adapters import ( + AlbumSearchQuery, + api_objects as SublimeAPI, + CacheMissError, + SongCacheStatus, +) from sublime.adapters.filesystem import FilesystemAdapter from sublime.adapters.subsonic import api_objects as SubsonicAPI @@ -374,36 +379,31 @@ def test_malformed_song_path(cache_adapter: FilesystemAdapter): assert song_uri2.endswith("fine/path/song2.mp3") -def test_get_cached_status(cache_adapter: FilesystemAdapter): +def test_get_cached_statuses(cache_adapter: FilesystemAdapter): cache_adapter.ingest_new_data(KEYS.SONG, "1", MOCK_SUBSONIC_SONGS[1]) - assert ( - cache_adapter.get_cached_status(cache_adapter.get_song_details("1")) - == SongCacheStatus.NOT_CACHED - ) + assert cache_adapter.get_cached_statuses([cache_adapter.get_song_details("1")]) == [ + SongCacheStatus.NOT_CACHED + ] cache_adapter.ingest_new_data(KEYS.SONG_FILE, "1", (None, MOCK_SONG_FILE)) - assert ( - cache_adapter.get_cached_status(cache_adapter.get_song_details("1")) - == SongCacheStatus.CACHED - ) + assert cache_adapter.get_cached_statuses([cache_adapter.get_song_details("1")]) == [ + SongCacheStatus.CACHED + ] cache_adapter.ingest_new_data(KEYS.SONG_FILE_PERMANENT, "1", None) - assert ( - cache_adapter.get_cached_status(cache_adapter.get_song_details("1")) - == SongCacheStatus.PERMANENTLY_CACHED - ) + assert cache_adapter.get_cached_statuses([cache_adapter.get_song_details("1")]) == [ + SongCacheStatus.PERMANENTLY_CACHED + ] cache_adapter.invalidate_data(KEYS.SONG_FILE, "1") - assert ( - cache_adapter.get_cached_status(cache_adapter.get_song_details("1")) - == SongCacheStatus.CACHED_STALE - ) + assert cache_adapter.get_cached_statuses([cache_adapter.get_song_details("1")]) == [ + SongCacheStatus.CACHED_STALE + ] cache_adapter.delete_data(KEYS.SONG_FILE, "1") - assert ( - cache_adapter.get_cached_status(cache_adapter.get_song_details("1")) - == SongCacheStatus.NOT_CACHED - ) + assert cache_adapter.get_cached_statuses([cache_adapter.get_song_details("1")]) == [ + SongCacheStatus.NOT_CACHED + ] def test_delete_playlists(cache_adapter: FilesystemAdapter): @@ -544,9 +544,9 @@ def test_caching_get_song_details(cache_adapter: FilesystemAdapter): song = cache_adapter.get_song_details("1") assert song.id == "1" assert song.title == "Song 1" - assert song.album + assert song.album and song.artist assert (song.album.id, song.album.name) == ("a2", "bar") - assert song.artist and song.artist.name == "bar" + assert (song.artist.id, song.artist.name) == ("art2", "bar") assert song.parent_id == "bar" assert song.duration == timedelta(seconds=10.2) assert song.path == "bar/song1.mp3" @@ -556,6 +556,71 @@ def test_caching_get_song_details(cache_adapter: FilesystemAdapter): cache_adapter.get_playlist_details("2") +def test_caching_get_song_details_missing_data(cache_adapter: FilesystemAdapter): + with pytest.raises(CacheMissError): + cache_adapter.get_song_details("1") + + # Ingest a song without an album ID and artist ID, but with album and artist name. + cache_adapter.ingest_new_data( + KEYS.SONG, + "1", + SubsonicAPI.Song( + "1", + title="Song 1", + parent_id="bar", + _album="bar", + _artist="foo", + duration=timedelta(seconds=10.2), + path="foo/bar/song1.mp3", + _genre="Bar", + ), + ) + + song = cache_adapter.get_song_details("1") + assert song.id == "1" + assert song.title == "Song 1" + assert song.album + assert (song.album.id, song.album.name) == ( + "invalid:62cdb7020ff920e5aa642c3d4066950dd1f01f4d", + "bar", + ) + assert (song.artist.id, song.artist.name) == ( + "invalid:0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33", + "foo", + ) + assert song.parent_id == "bar" + assert song.duration == timedelta(seconds=10.2) + assert song.path == "foo/bar/song1.mp3" + assert song.genre and song.genre.name == "Bar" + + # Because the album and artist are invalid (doesn't have an album/artist ID), it + # shouldn't show up in any results. + try: + list( + cache_adapter.get_albums( + AlbumSearchQuery(AlbumSearchQuery.Type.ALPHABETICAL_BY_NAME) + ) + ) + except CacheMissError as e: + assert e.partial_data is not None + assert len(e.partial_data) == 0 + + albums = list(cache_adapter.get_all_albums()) + assert len(albums) == 0 + + with pytest.raises(CacheMissError): + cache_adapter.get_album("invalid:62cdb7020ff920e5aa642c3d4066950dd1f01f4d") + + try: + list(cache_adapter.get_artists()) + except CacheMissError as e: + assert e.partial_data is not None + assert len(e.partial_data) == 0 + + with pytest.raises(CacheMissError): + cache_adapter.get_artist("invalid:0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33") + + def test_caching_less_info(cache_adapter: FilesystemAdapter): cache_adapter.ingest_new_data( KEYS.SONG, @@ -600,8 +665,10 @@ def test_caching_get_artists(cache_adapter: FilesystemAdapter): KEYS.ARTISTS, None, [ - SubsonicAPI.ArtistAndArtistInfo("1", "test1", album_count=3, albums=[]), - SubsonicAPI.ArtistAndArtistInfo("2", "test2", album_count=4), + SubsonicAPI.ArtistAndArtistInfo( + id="1", name="test1", album_count=3, albums=[] + ), + SubsonicAPI.ArtistAndArtistInfo(id="2", name="test2", album_count=4), ], ) @@ -615,8 +682,8 @@ def test_caching_get_artists(cache_adapter: FilesystemAdapter): KEYS.ARTISTS, None, [ - SubsonicAPI.ArtistAndArtistInfo("1", "test1", album_count=3), - SubsonicAPI.ArtistAndArtistInfo("3", "test3", album_count=8), + SubsonicAPI.ArtistAndArtistInfo(id="1", name="test1", album_count=3), + SubsonicAPI.ArtistAndArtistInfo(id="3", name="test3", album_count=8), ], ) @@ -651,17 +718,17 @@ def test_caching_get_artist(cache_adapter: FilesystemAdapter): KEYS.ARTIST, "1", SubsonicAPI.ArtistAndArtistInfo( - "1", - "Bar", + id="1", + name="Bar", album_count=1, artist_image_url="image", similar_artists=[ - SubsonicAPI.ArtistAndArtistInfo("A", "B"), - SubsonicAPI.ArtistAndArtistInfo("C", "D"), + SubsonicAPI.ArtistAndArtistInfo(id="A", name="B"), + SubsonicAPI.ArtistAndArtistInfo(id="C", name="D"), ], biography="this is a bio", music_brainz_id="mbid", - albums=[SubsonicAPI.Album("1", "Foo", artist_id="1")], + albums=[SubsonicAPI.Album(id="1", name="Foo", artist_id="1")], ), ) @@ -675,30 +742,32 @@ def test_caching_get_artist(cache_adapter: FilesystemAdapter): artist.music_brainz_id, ) == ("1", "Bar", 1, "image", "this is a bio", "mbid") assert artist.similar_artists == [ - SubsonicAPI.ArtistAndArtistInfo("A", "B"), - SubsonicAPI.ArtistAndArtistInfo("C", "D"), + SubsonicAPI.ArtistAndArtistInfo(id="A", name="B"), + SubsonicAPI.ArtistAndArtistInfo(id="C", name="D"), ] assert artist.albums and len(artist.albums) == 1 - assert cast(SelectQuery, artist.albums).dicts() == [SubsonicAPI.Album("1", "Foo")] + assert cast(SelectQuery, artist.albums).dicts() == [ + SubsonicAPI.Album(id="1", name="Foo") + ] # Simulate "force refreshing" the artist details being retrieved from Subsonic. cache_adapter.ingest_new_data( KEYS.ARTIST, "1", SubsonicAPI.ArtistAndArtistInfo( - "1", - "Foo", + id="1", + name="Foo", album_count=2, artist_image_url="image2", similar_artists=[ - SubsonicAPI.ArtistAndArtistInfo("A", "B"), - SubsonicAPI.ArtistAndArtistInfo("E", "F"), + SubsonicAPI.ArtistAndArtistInfo(id="A", name="B"), + SubsonicAPI.ArtistAndArtistInfo(id="E", name="F"), ], biography="this is a bio2", music_brainz_id="mbid2", albums=[ - SubsonicAPI.Album("1", "Foo", artist_id="1"), - SubsonicAPI.Album("2", "Bar", artist_id="1"), + SubsonicAPI.Album(id="1", name="Foo", artist_id="1"), + SubsonicAPI.Album(id="2", name="Bar", artist_id="1"), ], ), ) @@ -713,13 +782,13 @@ def test_caching_get_artist(cache_adapter: FilesystemAdapter): artist.music_brainz_id, ) == ("1", "Foo", 2, "image2", "this is a bio2", "mbid2") assert artist.similar_artists == [ - SubsonicAPI.ArtistAndArtistInfo("A", "B"), - SubsonicAPI.ArtistAndArtistInfo("E", "F"), + SubsonicAPI.ArtistAndArtistInfo(id="A", name="B"), + SubsonicAPI.ArtistAndArtistInfo(id="E", name="F"), ] assert artist.albums and len(artist.albums) == 2 assert cast(SelectQuery, artist.albums).dicts() == [ - SubsonicAPI.Album("1", "Foo"), - SubsonicAPI.Album("2", "Bar"), + SubsonicAPI.Album(id="1", name="Foo"), + SubsonicAPI.Album(id="2", name="Bar"), ] @@ -732,8 +801,8 @@ def test_caching_get_album(cache_adapter: FilesystemAdapter): KEYS.ALBUM, "a1", SubsonicAPI.Album( - "a1", - "foo", + id="a1", + name="foo", cover_art="c", song_count=2, year=2020, @@ -767,31 +836,31 @@ def test_caching_invalidate_artist(cache_adapter: FilesystemAdapter): KEYS.ARTIST, "artist1", SubsonicAPI.ArtistAndArtistInfo( - "artist1", - "Bar", + id="artist1", + name="Bar", album_count=1, artist_image_url="image", similar_artists=[ - SubsonicAPI.ArtistAndArtistInfo("A", "B"), - SubsonicAPI.ArtistAndArtistInfo("C", "D"), + SubsonicAPI.ArtistAndArtistInfo(id="A", name="B"), + SubsonicAPI.ArtistAndArtistInfo(id="C", name="D"), ], biography="this is a bio", music_brainz_id="mbid", albums=[ - SubsonicAPI.Album("1", "Foo", artist_id="1"), - SubsonicAPI.Album("2", "Bar", artist_id="1"), + SubsonicAPI.Album(id="1", name="Foo", artist_id="1"), + SubsonicAPI.Album(id="2", name="Bar", artist_id="1"), ], ), ) cache_adapter.ingest_new_data( KEYS.ALBUM, "1", - SubsonicAPI.Album("1", "Foo", artist_id="artist1", cover_art="1"), + SubsonicAPI.Album(id="1", name="Foo", artist_id="artist1", cover_art="1"), ) cache_adapter.ingest_new_data( KEYS.ALBUM, "2", - SubsonicAPI.Album("2", "Bar", artist_id="artist1", cover_art="2"), + SubsonicAPI.Album(id="2", name="Bar", artist_id="artist1", cover_art="2"), ) cache_adapter.ingest_new_data(KEYS.COVER_ART_FILE, "image", MOCK_ALBUM_ART3) cache_adapter.ingest_new_data(KEYS.COVER_ART_FILE, "1", MOCK_ALBUM_ART) @@ -899,15 +968,21 @@ def test_search(cache_adapter: FilesystemAdapter): search_result.add_results( "albums", [ - SubsonicAPI.Album("album1", "Foo", artist_id="artist1", cover_art="cal1"), - SubsonicAPI.Album("album2", "Boo", artist_id="artist1", cover_art="cal2"), + SubsonicAPI.Album( + id="album1", name="Foo", artist_id="artist1", cover_art="cal1" + ), + SubsonicAPI.Album( + id="album2", name="Boo", artist_id="artist1", cover_art="cal2" + ), ], ) search_result.add_results( "artists", [ - SubsonicAPI.ArtistAndArtistInfo("artist1", "foo", cover_art="car1"), - SubsonicAPI.ArtistAndArtistInfo("artist2", "better boo", cover_art="car2"), + SubsonicAPI.ArtistAndArtistInfo(id="artist1", name="foo", cover_art="car1"), + SubsonicAPI.ArtistAndArtistInfo( + id="artist2", name="better boo", cover_art="car2" + ), ], ) search_result.add_results( diff --git a/tests/adapter_tests/mock_data/get_song_details_no_albumid-airsonic.json b/tests/adapter_tests/mock_data/get_song_details_no_albumid-airsonic.json new file mode 100644 index 0000000..fdee341 --- /dev/null +++ b/tests/adapter_tests/mock_data/get_song_details_no_albumid-airsonic.json @@ -0,0 +1,30 @@ +{ + "subsonic-response": { + "status": "ok", + "version": "1.15.0", + "song": { + "id": "1", + "parent": "544", + "isDir": false, + "title": "Sweet Caroline", + "album": "50th Anniversary Collection", + "artist": "Neil Diamond", + "track": 16, + "year": 2017, + "genre": "Pop", + "coverArt": "544", + "size": 7437928, + "contentType": "audio/mpeg", + "suffix": "mp3", + "duration": 203, + "bitRate": 288, + "path": "Neil Diamond/50th Anniversary Collection/16 - Sweet Caroline.mp3", + "isVideo": false, + "playCount": 7, + "discNumber": 1, + "created": "2020-03-27T05:25:52.000Z", + "artistId": "60", + "type": "music" + } + } +} diff --git a/tests/adapter_tests/mock_data/get_song_details_no_artistid-airsonic.json b/tests/adapter_tests/mock_data/get_song_details_no_artistid-airsonic.json new file mode 100644 index 0000000..c4b6601 --- /dev/null +++ b/tests/adapter_tests/mock_data/get_song_details_no_artistid-airsonic.json @@ -0,0 +1,30 @@ +{ + "subsonic-response": { + "status": "ok", + "version": "1.15.0", + "song": { + "id": "1", + "parent": "544", + "isDir": false, + "title": "Sweet Caroline", + "album": "50th Anniversary Collection", + "artist": "Neil Diamond", + "track": 16, + "year": 2017, + "genre": "Pop", + "coverArt": "544", + "size": 7437928, + "contentType": "audio/mpeg", + "suffix": "mp3", + "duration": 203, + "bitRate": 288, + "path": "Neil Diamond/50th Anniversary Collection/16 - Sweet Caroline.mp3", + "isVideo": false, + "playCount": 7, + "discNumber": 1, + "created": "2020-03-27T05:25:52.000Z", + "albumId": "88", + "type": "music" + } + } +} diff --git a/tests/adapter_tests/subsonic_adapter_tests.py b/tests/adapter_tests/subsonic_adapter_tests.py index bb70dc5..b4a716b 100644 --- a/tests/adapter_tests/subsonic_adapter_tests.py +++ b/tests/adapter_tests/subsonic_adapter_tests.py @@ -253,6 +253,50 @@ def test_get_song_details(adapter: SubsonicAdapter): assert song.genre and song.genre.name == "Pop" +def test_get_song_details_missing_data(adapter: SubsonicAdapter): + for filename, data in mock_data_files("get_song_details_no_albumid"): + logging.info(filename) + logging.debug(data) + adapter._set_mock_data(data) + + song = adapter.get_song_details("1") + assert (song.id, song.title, song.year, song.cover_art, song.duration) == ( + "1", + "Sweet Caroline", + 2017, + "544", + timedelta(seconds=203), + ) + assert song.path and song.path.endswith("Sweet Caroline.mp3") + assert song.parent_id == "544" + assert song.artist + assert (song.artist.id, song.artist.name) == ("60", "Neil Diamond") + assert song.album + assert (song.album.id, song.album.name) == (None, "50th Anniversary Collection") + assert song.genre and song.genre.name == "Pop" + + for filename, data in mock_data_files("get_song_details_no_artistid"): + logging.info(filename) + logging.debug(data) + adapter._set_mock_data(data) + + song = adapter.get_song_details("1") + assert (song.id, song.title, song.year, song.cover_art, song.duration) == ( + "1", + "Sweet Caroline", + 2017, + "544", + timedelta(seconds=203), + ) + assert song.path and song.path.endswith("Sweet Caroline.mp3") + assert song.parent_id == "544" + assert song.artist + assert (song.artist.id, song.artist.name) == (None, "Neil Diamond") + assert song.album + assert (song.album.id, song.album.name) == ("88", "50th Anniversary Collection") + assert song.genre and song.genre.name == "Pop" + + def test_get_genres(adapter: SubsonicAdapter): for filename, data in mock_data_files("get_genres"): logging.info(filename)