Cancelling downloads works, and no longer blocks shutdown for forever

Closes #13

CHANGELOG.rst

@@ -17,6 +17,7 @@ Features
 * The Albums tab is now paginated with configurable page sizes.
 * You can sort the Albums tab ascending or descending.
 * Opening an closing an album on the Albums tab now has a nice animation.
+* "Go to Album" is much more reliable.
 
 **Player Controls**
 
@@ -26,7 +27,8 @@ Features
 
 **New Icons**
 
-* The Devices button now uses the Chromecast logo.
+* The Devices button now uses the Chromecast logo. It uses a different icon
+  depending on whether or not you are playing on a Chromecast.
 * Custom icons for "Add to play queue", and "Play next" buttons. Thanks to
   @samsartor for contributing the SVGs!
 * A new icon for indicating the connection state to the Subsonic server.
@@ -53,6 +55,9 @@ Features
 
 **Other Features**
 
+* You can now collapse the Artist details and the Playlist details so that you
+  have more room to view the actual content.
+
 .. * A man page has been added. Contributed by @baldurmen.
 
 Under The Hood

docs/index.rst

@@ -5,7 +5,7 @@
 Welcome to Sublime Music's documentation!
 =========================================
 
-Sublime Music is a GTK3
+Sublime Music is a native, GTK3
 `Subsonic`_/`Airsonic`_/`Revel`_/`Gonic`_/`Navidrome`_/\*sonic client for the
 Linux Desktop.
 
@@ -26,16 +26,17 @@ Linux Desktop.
 Features
 --------
 
-- Switch between multiple Subsonic-API-compliant servers.
+* Switch between multiple Subsonic-API-compliant servers.
-- Play music through Chromecast devices on the same LAN.
+* Play music through Chromecast devices on the same LAN.
-- DBus MPRIS interface integration for controlling Sublime Music via DBus MPRIS
+* Offline Mode where Sublime Music will not make any network requests.
-  clients such as ``playerctl``, ``i3status-rust``, KDE Connect, and many
+* DBus MPRIS interface integration for controlling Sublime Music via clients
-  commonly used desktop environments.
+  such as ``playerctl``, ``i3status-rust``, KDE Connect, and many commonly used
-- Browse songs by the sever-reported filesystem structure, or view them
+  desktop environments.
+* Browse songs by the sever-reported filesystem structure, or view them
   organized by ID3 tags in the Albums, Artists, and Playlists views.
-- Intuitive play queue.
+* Intuitive play queue.
-- Create/delete/edit playlists.
+* Create/delete/edit playlists.
-- Cache songs for offline listening.
+* Download songs for offline listening.
 
 Installation
 ------------

docs/settings.rst

@@ -2,14 +2,32 @@ Settings
 ########
 
 There are many settings available in Sublime Music. Some are application-wide
-settings while others are are configurable at a per-server basis.
+settings while others are are configurable on a per-music-provider basis.
 
 Application Settings
 --------------------
 
-The following settings can be changed for the entire application.
+The following settings can be changed for the entire application. They can be
+accessed via the gear icon in the header.
 
-Port Number : (int)
+Enable Song Notifications : (``bool``)
+    If toggled on, a notification will be shown whenever a new song starts to
+    play. The notification will contain the title, artist name, album name, and
+    cover art of the song.
+
+Replay Gain : (Disabled | Track | Album)
+    Configures the replay gain setting for the MPV player. You can disable this
+    setting, or configure it to work on a track or album basis.
+
+Serve Local Files to Chromecasts on the LAN : (``bool``)
+    If checked, a local server will be started on your computer which will serve
+    your locally cached music files to the Chromecast. If not checked, the
+    Chromecast will always stream from the server.
+
+    If you are having trouble playing on your Chromecast, try disabling this
+    feature.
+
+LAN Server Port Number : (int)
     The port number to use for streaming to Chromecast devices on the same
     LAN.
 
@@ -17,39 +35,24 @@ Port Number : (int)
     already cached locally, the Chromecast will connect to your computer and
     stream from it instead of from the internet.
 
-    This will not take effect until the application is restarted.
+Allow Song Downloads : (bool)
+    If toggled on, this will allow songs to be downloaded and cached locally.
 
-Replay Gain : (Disabled | Track | Album)
+When Streaming, Also Download Song : (bool)
-    Configures the replay gain setting for the MPV player. You can disable this
+    If toggled on, when a song is streamed, it will also be downloaded. Once the
-    setting, or configure it to work on a track or album basis.
-
-Always stream songs : (bool)
-    If checked, this will disable using the local song cache.
-
-When streaming, also download song : (bool)
-    If checked, when a song is streamed, it will also be downloaded. Once the
     download is complete, Sublime Music will stop streaming and switch to the
-    downloaded version.
+    downloaded version (if playing locally with MPV).
 
-Show notification when song begins to play : (bool)
+Number of Songs to Prefetch : (int)
-    If checked, a notification containing the new song's title, artist, album,
-    and album art will be shown through your notification daemon.
-
-Serve locally cached files over the LAN to Chromecast devices : (bool)
-    If checked, a local server will be started on your computer which will serve
-    your locally cached music files to the Chromecast. If not checked, the
-    Chromecast will always stream from the server.
-
-How many songs in the play queue do you want to prefetch? : (int)
     If the next :math:`n` songs in the play queue are not already downloaded,
-    they will be downloaded. (This has no effect if *Always stream songs* is
+    they will be downloaded.
-    checked.)
 
-How many song downloads do you want to allow concurrently? : (int)
+Maximum Concurrent Downloads : (int)
-    Specifies how many songs can be downloaded at the same time.
+    Specifies the maximum number of songs that should be downloaded at the same
+    time.
 
-Server Settings
+Subsonic Server Settings
----------------
+------------------------
 
 Each server has the following configuration options:
 

sublime-music.metainfo.xml

@@ -5,7 +5,7 @@
     <metadata_license>FSFAP</metadata_license>
     <project_license>GPL-3.0+</project_license>
     <name>Sublime Music</name>
-    <summary>A native GTK music player with *sonic support</summary>
+    <summary>Native Subsonic client for Linux</summary>
 
     <description>
         <p>

sublime/adapters/manager.py

@@ -16,6 +16,7 @@ from typing import (
     Any,
     Callable,
     cast,
+    Dict,
     Generic,
     Iterable,
     List,
@@ -80,6 +81,7 @@ class Result(Generic[T]):
     _future: Optional[Future] = None
     _default_value: Optional[T] = None
     _on_cancel: Optional[Callable[[], None]] = None
+    _cancelled = False
 
     def __init__(
         self,
@@ -152,8 +154,12 @@ class Result(Generic[T]):
             self._on_cancel()
         if self._future is not None:
             return self._future.cancel()
+        self._cancelled = True
         return True
 
+    def cancelled(self) -> bool:
+        return self._cancelled
+
     @property
     def data_is_available(self) -> bool:
         """
@@ -194,6 +200,9 @@ class AdapterManager:
     is_shutting_down: bool = False
     _offline_mode: bool = False
 
+    _song_download_jobs: Dict[str, Result[str]] = {}
+    _cancelled_song_ids: Set[str] = set()
+
     @dataclass
     class _AdapterManagerInternal:
         ground_truth_adapter: Adapter
@@ -253,10 +262,16 @@ class AdapterManager:
     def shutdown():
         logging.info("AdapterManager shutdown start")
         AdapterManager.is_shutting_down = True
+        for _, job in AdapterManager._song_download_jobs.items():
+            job.cancel()
+
         AdapterManager.executor.shutdown()
+        print("2")
         AdapterManager.download_executor.shutdown()
+        print("3")
         if AdapterManager._instance:
             AdapterManager._instance.shutdown()
+        print("4")
 
         logging.info("AdapterManager shutdown complete")
 
@@ -377,17 +392,19 @@ class AdapterManager:
         return Result(future_fn)
 
     @staticmethod
-    def _create_download_fn(
+    def _create_download_result(
         uri: str,
         id: str,
         before_download: Callable[[], None] = None,
         expected_size: int = None,
-    ) -> Callable[[], str]:
+        **result_args,
+    ) -> Result[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
         downloaded to prevent multiple requests for the same download.
         """
+        download_cancelled = False
 
         def download_fn() -> str:
             assert AdapterManager._instance
@@ -463,6 +480,13 @@ class AdapterManager:
                             total_consumed += len(data)
                             f.write(data)
 
+                            if download_cancelled:
+                                AdapterManager._instance.song_download_progress(
+                                    id,
+                                    DownloadProgress(DownloadProgress.Type.CANCELLED),
+                                )
+                                raise Exception("Download Cancelled")
+
                             if i % 100 == 0:
                                 # Only delay (if configured) and update the progress UI
                                 # every 100 KiB.
@@ -485,7 +509,7 @@ class AdapterManager:
                             id, DownloadProgress(DownloadProgress.Type.DONE),
                         )
                 except Exception as e:
-                    if expected_size_exists:
+                    if expected_size_exists and not download_cancelled:
                         # Something failed. Post an error.
                         AdapterManager._instance.song_download_progress(
                             id,
@@ -501,7 +525,13 @@ class AdapterManager:
             logging.info(f"{uri} downloaded. Returning.")
             return str(download_tmp_filename)
 
-        return download_fn
+        def on_download_cancel():
+            nonlocal download_cancelled
+            download_cancelled = True
+
+        return Result(
+            download_fn, is_download=True, on_cancel=on_download_cancel, **result_args
+        )
 
     @staticmethod
     def _create_caching_done_callback(
@@ -862,15 +892,12 @@ class AdapterManager:
         ):
             return Result(existing_cover_art_filename)
 
-        future: Result[str] = Result(
+        future: Result[str] = AdapterManager._create_download_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,
         )
 
@@ -941,27 +968,31 @@ class AdapterManager:
             return Result(None)
 
         cancelled = False
+        AdapterManager._cancelled_song_ids -= set(song_ids)
 
         def do_download_song(song_id: str):
+            assert AdapterManager._instance
+            assert AdapterManager._instance.caching_adapter
+
             if (
                 AdapterManager.is_shutting_down
                 or AdapterManager._offline_mode
                 or cancelled
+                or song_id in AdapterManager._cancelled_song_ids
             ):
+                AdapterManager._instance.download_limiter_semaphore.release()
+                AdapterManager._instance.song_download_progress(
+                    song_id, DownloadProgress(DownloadProgress.Type.CANCELLED),
+                )
                 return
 
-            assert AdapterManager._instance
-            assert AdapterManager._instance.caching_adapter
-
             logging.info(f"Downloading {song_id}")
 
-            try:
             # Download the actual song file.
             try:
-                    # If the song file is already cached, return immediately.
+                # If the song file is already cached, just indicate done immediately.
-                    AdapterManager._instance.caching_adapter.get_song_uri(
+                AdapterManager._instance.caching_adapter.get_song_uri(song_id, "file")
-                        song_id, "file"
+                AdapterManager._instance.download_limiter_semaphore.release()
-                    )
                 AdapterManager._instance.song_download_progress(
                     song_id, DownloadProgress(DownloadProgress.Type.DONE),
                 )
@@ -973,27 +1004,36 @@ class AdapterManager:
                 song = AdapterManager.get_song_details(song_id).result()
 
                 # Download the song.
-                    # TODO figure out how to cancel
+                song_tmp_filename_result: Result[
-                    song_tmp_filename = AdapterManager._create_download_fn(
+                    str
+                ] = AdapterManager._create_download_result(
                     AdapterManager._instance.ground_truth_adapter.get_song_uri(
                         song_id, AdapterManager._get_scheme()
                     ),
                     song_id,
                     lambda: before_download(song_id),
                     expected_size=song.size,
-                    )()
+                )
+
+                def on_download_done(f: Result):
+                    assert AdapterManager._instance
+                    assert AdapterManager._instance.caching_adapter
+                    AdapterManager._instance.download_limiter_semaphore.release()
+
+                    if AdapterManager._song_download_jobs.get(song_id):
+                        del AdapterManager._song_download_jobs[song_id]
+
+                    try:
                         AdapterManager._instance.caching_adapter.ingest_new_data(
                             CachingAdapter.CachedDataKey.SONG_FILE,
                             song_id,
-                        (None, song_tmp_filename, None),
+                            (None, f.result(), None),
                         )
+                    except Exception:
                         on_song_download_complete(song_id)
-            finally:
+
-                # Release the semaphore lock. This will allow the next song in the queue
+                song_tmp_filename_result.add_done_callback(on_download_done)
-                # to be downloaded. I'm doing this in the finally block so that it
+                AdapterManager._song_download_jobs[song_id] = song_tmp_filename_result
-                # always runs, regardless of whether an exception is thrown or the
-                # function returns.
-                AdapterManager._instance.download_limiter_semaphore.release()
 
         def do_batch_download_songs():
             sleep(delay)
@@ -1012,12 +1052,6 @@ class AdapterManager:
                 )
 
             for song_id in song_ids:
-                if (
-                    AdapterManager.is_shutting_down
-                    or AdapterManager._offline_mode
-                    or cancelled
-                ):
-                    return
                 # Only allow a certain number of songs to be downloaded
                 # simultaneously.
                 AdapterManager._instance.download_limiter_semaphore.acquire()
@@ -1032,9 +1066,11 @@ class AdapterManager:
             nonlocal cancelled
             cancelled = True
 
+            # Cancel the individual song downloads
+            AdapterManager.cancel_download_songs(song_ids)
+
             # Alert the UI that the downloads are cancelled.
             for song_id in song_ids:
-                # Everything succeeded.
                 AdapterManager._instance.song_download_progress(
                     song_id, DownloadProgress(DownloadProgress.Type.CANCELLED),
                 )
@@ -1043,7 +1079,17 @@ class AdapterManager:
 
     @staticmethod
     def cancel_download_songs(song_ids: Sequence[str]):
-        print("cancel songs!!!!!", song_ids)
+        assert AdapterManager._instance
+        AdapterManager._cancelled_song_ids = AdapterManager._cancelled_song_ids.union(
+            set(song_ids)
+        )
+        for song_id in song_ids:
+            AdapterManager._instance.song_download_progress(
+                song_id, DownloadProgress(DownloadProgress.Type.CANCELLED),
+            )
+            if AdapterManager._song_download_jobs.get(song_id):
+                AdapterManager._song_download_jobs[song_id].cancel()
+                del AdapterManager._song_download_jobs[song_id]
 
     @staticmethod
     def batch_permanently_cache_songs(
@@ -1369,7 +1415,10 @@ class AdapterManager:
         )
         return [
             SongCacheStatus.DOWNLOADING
-            if song_id in AdapterManager.current_download_ids
+            if (
+                song_id in AdapterManager.current_download_ids
+                and song_id not in AdapterManager._cancelled_song_ids
+            )
             else cached_statuses[song_id]
             for song_id in song_ids
         ]

sublime/ui/main.py

@@ -165,13 +165,20 @@ class MainWindow(Gtk.ApplicationWindow):
 
         self._updating_settings = True
 
-        # Main Settings
+        # Offline Mode Settings
         offline_mode = app_config.offline_mode
         self.offline_mode_switch.set_active(offline_mode)
+
+        # Main Settings
         self.notification_switch.set_active(app_config.song_play_notification)
+
+        # MPV Settings
         self.replay_gain_options.set_active_id(app_config.replay_gain.as_string())
+
+        # Chromecast Settings
         self.serve_over_lan_switch.set_active(app_config.serve_over_lan)
         self.port_number_entry.set_value(app_config.port_number)
+        self.port_number_entry.set_sensitive(app_config.serve_over_lan)
 
         # Download Settings
         allow_song_downloads = app_config.allow_song_downloads
@@ -318,6 +325,7 @@ class MainWindow(Gtk.ApplicationWindow):
         AdapterManager.cancel_download_songs(
             {*self._pending_downloads, *self._current_download_boxes.keys()}
         )
+        self.emit("refresh-window", {}, False)
 
     def _on_download_box_cancel_click(self, _, song_id: str):
         AdapterManager.cancel_download_songs([song_id])

sublime/ui/util.py

@@ -247,7 +247,12 @@ def show_song_popover(
         ):
             download_song_button.set_sensitive(True)
         if any(
-            status in (SongCacheStatus.CACHED, SongCacheStatus.PERMANENTLY_CACHED)
+            status
+            in (
+                SongCacheStatus.CACHED,
+                SongCacheStatus.PERMANENTLY_CACHED,
+                SongCacheStatus.DOWNLOADING,
+            )
             for status in song_cache_statuses
         ):
             remove_download_button.set_sensitive(True)