Fixed a bunch of perf issues; handled null album/artist IDs; sort directories & albums in artist view & genres

Closes #181

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.
 

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.
         """

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]

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:

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)

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(
+        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)))
 
+        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:
-            return SongCacheStatus.DOWNLOADING
+                cache_statuses.append(SongCacheStatus.DOWNLOADING)
+            else:
+                cache_statuses.append(cache_status)
 
-        return AdapterManager._instance.caching_adapter.get_cached_status(song)
+        return cache_statuses

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)
 
 

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

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"<AlbumsGrid.AlbumModel {self.album}>"
+            return f"<AlbumsGrid._AlbumModel {self.album}>"
 
     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.

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.

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 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:
-                song = cast(API.Song, el)
-                new_songs_store.append(
+                    self.songs.append(cast(API.Song, el))
+
+            util.diff_model_store(
+                self.drilldown_directories_store, new_directories_store
+            )
+
+            new_songs_store = [
                 [
-                        util.get_cached_status_icon(song),
+                    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
                 )
-
-        # 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
+            ]
+        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
                 )
-        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:

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)

sublime/ui/main.py

@@ -395,7 +395,7 @@ class MainWindow(Gtk.ApplicationWindow):
                     f"<b>{util.esc(song.title)}</b>",
                     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"<b>{util.esc(album.name)}</b>",
                     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

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,
             )

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 = [
+        # 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)
+
+        if force:
+            self._current_song_ids = song_ids
+            self.songs = [cast(API.Song, s) for s in songs]
+
+            new_songs_store = [
                 [
-                util.get_cached_status_icon(song),
+                    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 song in playlist.songs
+                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_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')}"

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):

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
-        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)
+    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
+        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,

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(

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"
+    }
+  }
+}

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"
+    }
+  }
+}

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)