Improve ergonomics of getting song URIs

sublime/adapters/adapter_base.py

@@ -448,16 +448,16 @@ class Adapter(abc.ABC):
         return False
 
     @property
-    def can_stream(self) -> bool:
+    def can_get_song_file_uri(self) -> bool:
         """
-        Whether or not the adapter can provide a stream URI.
+        Whether or not the adapter supports :class:`get_song_file_uri`.
         """
         return False
 
     @property
-    def can_get_song_uri(self) -> bool:
+    def can_get_song_stream_uri(self) -> bool:
         """
-        Whether or not the adapter supports :class:`get_song_uri`.
+        Whether or not the adapter supports :class:`get_song_stream_uri`.
         """
         return False
 
@@ -645,19 +645,26 @@ class Adapter(abc.ABC):
         """
         raise self._check_can_error("get_cover_art_uri")
 
-    def get_song_uri(self, song_id: str, scheme: str, stream: bool = False) -> str:
+    def get_song_file_uri(self, song_id: str, schemes: Iterable[str]) -> str:
         """
-        Get a URI for a given song.
+        Get a URI for a given song. This URI must give the full file.
 
         :param song_id: The ID of the song to get a URI for.
-        :param scheme: The URI scheme that should be returned. It is guaranteed that
+        :param schemes: A set of URI schemes that can be returned. It is guaranteed that
-            ``scheme`` will be one of the schemes returned by
+            all of the items in ``schemes`` will be one of the schemes returned by
             :class:`supported_schemes`.
-        :param stream: Whether or not the URI returned should be a stream URI. This will
-            only be ``True`` if :class:`supports_streaming` returns ``True``.
         :returns: The URI for the given song.
         """
-        raise self._check_can_error("get_song_uri")
+        raise self._check_can_error("get_song_file_uri")
+
+    def get_song_stream_uri(self, song_id: str) -> str:
+        """
+        Get a URI for streaming the given song.
+
+        :param song_id: The ID of the song to get the stream URI for.
+        :returns: the stream URI for the given song.
+        """
+        raise self._check_can_error("get_song_stream_uri")
 
     def get_song_details(self, song_id: str) -> Song:
         """

sublime/adapters/filesystem/adapter.py

@@ -101,7 +101,7 @@ class FilesystemAdapter(CachingAdapter):
 
     # TODO (#200) make these dependent on cache state. Need to do this kinda efficiently
     can_get_cover_art_uri = True
-    can_get_song_uri = True
+    can_get_song_file_uri = True
     can_get_song_details = True
     can_get_artist = True
     can_get_albums = True
@@ -286,7 +286,7 @@ class FilesystemAdapter(CachingAdapter):
 
         raise CacheMissError()
 
-    def get_song_uri(self, song_id: str, scheme: str, stream: bool = False) -> str:
+    def get_song_file_uri(self, song_id: str, schemes: Iterable[str]) -> str:
         song = models.Song.get_or_none(models.Song.id == song_id)
         if not song:
             if self.is_cache:

sublime/adapters/manager.py

@@ -655,13 +655,17 @@ class AdapterManager:
         return AdapterManager._ground_truth_can_do("delete_playlist")
 
     @staticmethod
-    def can_get_song_filename_or_stream() -> bool:
+    def can_get_song_file_uri() -> bool:
-        return AdapterManager._ground_truth_can_do("get_song_uri")
+        return AdapterManager._ground_truth_can_do("get_song_file_uri")
+
+    @staticmethod
+    def can_get_song_stream_uri() -> bool:
+        return AdapterManager._ground_truth_can_do("get_song_stream_uri")
 
     @staticmethod
     def can_batch_download_songs() -> bool:
         # We can only download from the ground truth adapter.
-        return AdapterManager._ground_truth_can_do("get_song_uri")
+        return AdapterManager._ground_truth_can_do("get_song_file_uri")
 
     @staticmethod
     def can_get_genres() -> bool:
@@ -848,10 +852,10 @@ class AdapterManager:
                 except CacheMissError as e:
                     if e.partial_data is not None:
                         existing_filename = cast(str, e.partial_data)
-                    logging.info(f'Cache Miss on {"get_cover_art_uri"}.')
+                    logging.info("Cache Miss on get_cover_art_uri.")
                 except Exception:
                     logging.exception(
-                        f'Error on {"get_cover_art_uri"} retrieving from cache.'
+                        "Error on get_cover_art_uri retrieving from cache."
                     )
 
             # If we are forcing, invalidate the existing cached data.
@@ -887,40 +891,40 @@ class AdapterManager:
 
         return Result("")
 
-    # TODO (#189): allow this to take a set of schemes
     @staticmethod
-    def get_song_filename_or_stream(
+    def get_song_file_uri(song: Song) -> str:
-        song: Song, format: str = None, force_stream: bool = False
-    ) -> str:
         assert AdapterManager._instance
         cached_song_filename = None
-        if AdapterManager._can_use_cache(force_stream, "get_song_uri"):
+        if AdapterManager._can_use_cache(False, "get_song_file_uri"):
-            assert AdapterManager._instance.caching_adapter
+            assert (caching_adapter := AdapterManager._instance.caching_adapter)
             try:
-                return AdapterManager._instance.caching_adapter.get_song_uri(
+                if "file" not in caching_adapter.supported_schemes:
-                    song.id, "file"
+                    raise Exception("file not a supported scheme")
-                )
+
+                return caching_adapter.get_song_file_uri(song.id, "file")
             except CacheMissError as e:
                 if e.partial_data is not None:
                     cached_song_filename = cast(str, e.partial_data)
-                logging.info(f'Cache Miss on {"get_song_filename_or_stream"}.')
+                logging.info("Cache Miss on get_song_file_uri.")
             except Exception:
-                logging.exception(
+                logging.exception("Error on get_song_file_uri retrieving from cache.")
-                    f'Error on {"get_song_filename_or_stream"} retrieving from cache.'
-                )
 
+        ground_truth_adapter = AdapterManager._instance.ground_truth_adapter
         if (
-            not AdapterManager._ground_truth_can_do("stream")
+            not AdapterManager._ground_truth_can_do("get_song_file_uri")
-            or not AdapterManager._ground_truth_can_do("get_song_uri")
+            or (ground_truth_adapter.is_networked and AdapterManager._offline_mode)
-            or (
+            or ("file" not in ground_truth_adapter.supported_schemes)
-                AdapterManager._instance.ground_truth_adapter.is_networked
-                and AdapterManager._offline_mode
-            )
         ):
             raise CacheMissError(partial_data=cached_song_filename)
 
-        return AdapterManager._instance.ground_truth_adapter.get_song_uri(
+        return ground_truth_adapter.get_song_file_uri(song.id, "file")
-            song.id, AdapterManager._get_networked_scheme(), stream=True,
+
+    @staticmethod
+    def get_song_stream_uri(song: Song) -> str:
+        assert AdapterManager._instance
+        # TODO
+        return AdapterManager._instance.ground_truth_adapter.get_song_stream_uri(
+            song.id
         )
 
     @staticmethod
@@ -968,7 +972,9 @@ class AdapterManager:
             # Download the actual song file.
             try:
                 # If the song file is already cached, just indicate done immediately.
-                AdapterManager._instance.caching_adapter.get_song_uri(song_id, "file")
+                AdapterManager._instance.caching_adapter.get_song_file_uri(
+                    song_id, "file"
+                )
                 AdapterManager._instance.download_limiter_semaphore.release()
                 AdapterManager._instance.song_download_progress(
                     song_id, DownloadProgress(DownloadProgress.Type.DONE),
@@ -984,7 +990,7 @@ class AdapterManager:
                 song_tmp_filename_result: Result[
                     str
                 ] = AdapterManager._create_download_result(
-                    AdapterManager._instance.ground_truth_adapter.get_song_uri(
+                    AdapterManager._instance.ground_truth_adapter.get_song_file_uri(
                         song_id, AdapterManager._get_networked_scheme()
                     ),
                     song_id,

sublime/adapters/subsonic/adapter.py

@@ -281,7 +281,8 @@ class SubsonicAdapter(Adapter):
     can_get_playlist_details = True
     can_get_playlists = True
     can_get_song_details = True
-    can_get_song_uri = True
+    can_get_song_file_uri = True
+    can_get_song_stream_uri = True
     can_scrobble_song = True
     can_search = True
     can_stream = True
@@ -537,10 +538,14 @@ class SubsonicAdapter(Adapter):
         params = {"id": cover_art_id, "size": size, **self._get_params()}
         return self._make_url("getCoverArt") + "?" + urlencode(params)
 
-    def get_song_uri(self, song_id: str, scheme: str, stream: bool = False) -> str:
+    def get_song_file_uri(self, song_id: str, schemes: Iterable[str]) -> str:
+        assert any(s in schemes for s in self.supported_schemes)
         params = {"id": song_id, **self._get_params()}
-        endpoint = "stream" if stream else "download"
+        return self._make_url("download") + "?" + urlencode(params)
-        return self._make_url(endpoint) + "?" + urlencode(params)
+
+    def get_song_stream_uri(self, song_id: str) -> str:
+        params = {"id": song_id, **self._get_params()}
+        return self._make_url("stream") + "?" + urlencode(params)
 
     def get_song_details(self, song_id: str) -> API.Song:
         song = self._get_json(self._make_url("getSong"), id=song_id).song

sublime/app.py

@@ -7,6 +7,7 @@ from datetime import timedelta
 from functools import partial
 from pathlib import Path
 from typing import Any, Callable, Dict, Iterable, List, Optional, Set, Tuple
+from urllib.parse import urlparse
 
 try:
     import osxmmkeys
@@ -35,6 +36,7 @@ except Exception:
 from .adapters import (
     AdapterManager,
     AlbumSearchQuery,
+    CacheMissError,
     DownloadProgress,
     Result,
     SongCacheStatus,
@@ -1112,9 +1114,27 @@ class SublimeMusicApp(Gtk.Application):
             if order_token != self.song_playing_order_token:
                 return
 
-            # TODO (#189): make this actually use the player's allowed list of schemes
+            uri = None
-            # to play.
+            try:
-            uri = AdapterManager.get_song_filename_or_stream(song)
+                if "file" in self.player_manager.supported_schemes:
+                    uri = AdapterManager.get_song_file_uri(song)
+            except CacheMissError:
+                logging.debug("Couldn't find the file, will attempt to stream.")
+
+            if not uri:
+                try:
+                    uri = AdapterManager.get_song_stream_uri(song)
+                except Exception:
+                    pass
+                if (
+                    not uri
+                    or urlparse(uri).scheme not in self.player_manager.supported_schemes
+                ):
+                    self.app_config.state.current_notification = UIState.UINotification(
+                        markup=f"<b>Unable to play {song.title}.</b>",
+                        icon="dialog-error",
+                    )
+                    return
 
             # Prevent it from doing the thing where it continually loads
             # songs when it has to download.
@@ -1190,7 +1210,7 @@ class SublimeMusicApp(Gtk.Application):
 
                         os.system(f"osascript -e '{' '.join(osascript_command)}'")
                 except Exception:
-                    logging.exception(
+                    logging.warning(
                         "Unable to display notification. Is a notification daemon running?"  # noqa: E501
                     )
 
@@ -1215,7 +1235,7 @@ class SublimeMusicApp(Gtk.Application):
                     assert self.player_manager
                     if self.player_manager.can_start_playing_with_no_latency:
                         self.player_manager.play_media(
-                            AdapterManager.get_song_filename_or_stream(song),
+                            AdapterManager.get_song_file_uri(song),
                             self.app_config.state.song_progress,
                             song,
                         )

sublime/players/chromecast.py

@@ -237,8 +237,7 @@ class ChromecastPlayer(Player):
             song = AdapterManager.get_song_details(
                 self._serving_song_id.value.decode()
             ).result()
-            filename = AdapterManager.get_song_filename_or_stream(song)
+            filename = AdapterManager.get_song_file_uri(song)
-            assert filename.startswith("file://")
             with open(filename[7:], "rb") as fin:
                 song_buffer = io.BytesIO(fin.read())
 

sublime/players/manager.py

@@ -1,6 +1,6 @@
 import logging
 from datetime import timedelta
-from typing import Any, Callable, Dict, List, Optional, Tuple, Type, Union
+from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type, Union
 
 from sublime.adapters.api_objects import Song
 
@@ -89,6 +89,12 @@ class PlayerManager:
         if current_player_type := self._get_current_player_type():
             return self.players.get(current_player_type)
 
+    @property
+    def supported_schemes(self) -> Set[str]:
+        if cp := self._get_current_player():
+            return cp.supported_schemes
+        return set()
+
     @property
     def can_start_playing_with_no_latency(self) -> bool:
         if self._current_device_id:

tests/adapter_tests/filesystem_adapter_tests.py

@@ -371,13 +371,13 @@ def test_invalidate_song_file(cache_adapter: FilesystemAdapter):
     cache_adapter.invalidate_data(KEYS.COVER_ART_FILE, "s1")
 
     with pytest.raises(CacheMissError):
-        cache_adapter.get_song_uri("1", "file")
+        cache_adapter.get_song_file_uri("1", "file")
 
     with pytest.raises(CacheMissError):
         cache_adapter.get_cover_art_uri("s1", "file", size=300)
 
     # Make sure it didn't delete the other song.
-    assert cache_adapter.get_song_uri("2", "file").endswith("song2.mp3")
+    assert cache_adapter.get_song_file_uri("2", "file").endswith("song2.mp3")
 
 
 def test_malformed_song_path(cache_adapter: FilesystemAdapter):
@@ -390,10 +390,10 @@ def test_malformed_song_path(cache_adapter: FilesystemAdapter):
         KEYS.SONG_FILE, "2", ("fine/path/song2.mp3", MOCK_SONG_FILE2, None)
     )
 
-    song_uri = cache_adapter.get_song_uri("1", "file")
+    song_uri = cache_adapter.get_song_file_uri("1", "file")
     assert song_uri.endswith(f"/music/{MOCK_SONG_FILE_HASH}")
 
-    song_uri2 = cache_adapter.get_song_uri("2", "file")
+    song_uri2 = cache_adapter.get_song_file_uri("2", "file")
     assert song_uri2.endswith("fine/path/song2.mp3")
 
 
@@ -467,7 +467,7 @@ def test_delete_song_data(cache_adapter: FilesystemAdapter):
         KEYS.COVER_ART_FILE, "s1", MOCK_ALBUM_ART,
     )
 
-    music_file_path = cache_adapter.get_song_uri("1", "file")
+    music_file_path = cache_adapter.get_song_file_uri("1", "file")
     cover_art_path = cache_adapter.get_cover_art_uri("s1", "file", size=300)
 
     cache_adapter.delete_data(KEYS.SONG_FILE, "1")
@@ -477,7 +477,7 @@ def test_delete_song_data(cache_adapter: FilesystemAdapter):
     assert not Path(cover_art_path).exists()
 
     try:
-        cache_adapter.get_song_uri("1", "file")
+        cache_adapter.get_song_file_uri("1", "file")
         assert 0, "DID NOT raise CacheMissError"
     except CacheMissError as e:
         assert e.partial_data is None