From 916e190bacdbd4cf990f1e7bc528a421db21f337 Mon Sep 17 00:00:00 2001 From: Sumner Evans Date: Thu, 20 Feb 2020 13:44:37 -0700 Subject: [PATCH] Added order token infrastructure and fixed most issues with late-arriving results --- sublime/app.py | 24 +++++++-- sublime/cache_manager.py | 6 ++- sublime/server/server.py | 47 +++++++++-------- sublime/ui/artists.py | 47 +++++++++++++---- sublime/ui/browse.py | 48 ++++++++++++++--- sublime/ui/common/album_with_songs.py | 8 +-- sublime/ui/player_controls.py | 19 ++++++- sublime/ui/playlists.py | 74 +++++++++++++++++++++------ sublime/ui/util.py | 19 ++++++- 9 files changed, 222 insertions(+), 70 deletions(-) diff --git a/sublime/app.py b/sublime/app.py index 16b78f9..b0ad646 100644 --- a/sublime/app.py +++ b/sublime/app.py @@ -798,6 +798,8 @@ class SublimeMusicApp(Gtk.Application): play_queue_future.add_done_callback( lambda f: GLib.idle_add(do_update, f)) + song_playing_order_token = 0 + def play_song( self, song_index: int, @@ -845,17 +847,31 @@ class SublimeMusicApp(Gtk.Application): ) song_notification.show() - def on_cover_art_download_complete(cover_art_filename): + def on_cover_art_download_complete( + cover_art_filename, + order_token, + ): + if order_token != self.song_playing_order_token: + return + # Add the image to the notification, and re-draw the # notification. song_notification.set_image_from_pixbuf( GdkPixbuf.Pixbuf.new_from_file(cover_art_filename)) song_notification.show() - cover_art_future = CacheManager.get_cover_art_filename( - song.coverArt, size=70) + def get_cover_art_filename(order_token): + cover_art_future = CacheManager.get_cover_art_filename( + song.coverArt, size=70) + return cover_art_future.result(), order_token + + self.song_playing_order_token += 1 + cover_art_future = CacheManager.create_future( + get_cover_art_filename, + self.song_playing_order_token, + ) cover_art_future.add_done_callback( - lambda f: on_cover_art_download_complete(f.result())) + lambda f: on_cover_art_download_complete(*f.result())) except Exception: logging.warning( 'Unable to display notification. Is a notification ' diff --git a/sublime/cache_manager.py b/sublime/cache_manager.py index 9e9ff93..2531543 100644 --- a/sublime/cache_manager.py +++ b/sublime/cache_manager.py @@ -446,7 +446,11 @@ class CacheManager(metaclass=Singleton): logging.info(f'{abs_path} not found. Downloading...') os.makedirs(download_path.parent, exist_ok=True) - self.save_file(download_path, download_fn()) + try: + self.save_file(download_path, download_fn()) + except requests.exceptions.ConnectionError: + with self.download_set_lock: + self.current_downloads.discard(abs_path) # Move the file to its cache download location. os.makedirs(abs_path.parent, exist_ok=True) diff --git a/sublime/server/server.py b/sublime/server/server.py index 7206550..de105ac 100644 --- a/sublime/server/server.py +++ b/sublime/server/server.py @@ -62,12 +62,12 @@ class Server: the module's user to query the \\*sonic server via the API. """ def __init__( - self, - name: str, - hostname: str, - username: str, - password: str, - disable_cert_verify: bool, + self, + name: str, + hostname: str, + username: str, + password: str, + disable_cert_verify: bool, ): self.name: str = name self.hostname: str = hostname @@ -98,9 +98,8 @@ class Server: if os.environ.get('SUBLIME_MUSIC_DEBUG_DELAY'): logging.info( - "SUBLIME_MUSIC_DEBUG_DELAY enabled. Pausing for", - f"{os.environ['SUBLIME_MUSIC_DEBUG_DELAY']} seconds.", - ) + "SUBLIME_MUSIC_DEBUG_DELAY enabled. Pausing for " + f"{os.environ['SUBLIME_MUSIC_DEBUG_DELAY']} seconds.") sleep(int(os.environ['SUBLIME_MUSIC_DEBUG_DELAY'])) # Deal with datetime parameters (convert to milliseconds since 1970) @@ -122,9 +121,9 @@ class Server: return result def _get_json( - self, - url: str, - **params: Union[None, str, datetime, int, List[int]], + self, + url: str, + **params: Union[None, str, datetime, int, List[int]], ) -> Response: """ Make a get request to a *Sonic REST API. Handle all types of errors @@ -714,10 +713,10 @@ class Server: return result.playlist def create_playlist( - self, - playlist_id: int = None, - name: str = None, - song_id: Union[int, List[int]] = None, + self, + playlist_id: int = None, + name: str = None, + song_id: Union[int, List[int]] = None, ) -> Union[PlaylistWithSongs, Response]: """ Creates (or updates) a playlist. @@ -780,14 +779,14 @@ class Server: return self._get_json(self._make_url('deletePlaylist'), id=id) def get_stream_url( - self, - id: str, - max_bit_rate: int = None, - format: str = None, - time_offset: int = None, - size: int = None, - estimate_content_length: bool = False, - converted: bool = False, + self, + id: str, + max_bit_rate: int = None, + format: str = None, + time_offset: int = None, + size: int = None, + estimate_content_length: bool = False, + converted: bool = False, ): """ Gets the URL to streams a given file. diff --git a/sublime/ui/artists.py b/sublime/ui/artists.py index a13c3b6..eba151d 100644 --- a/sublime/ui/artists.py +++ b/sublime/ui/artists.py @@ -122,7 +122,7 @@ class ArtistList(Gtk.Box): before_download=lambda self: self.loading_indicator.show_all(), on_failure=lambda self, e: self.loading_indicator.hide(), ) - def update(self, artists, state: ApplicationState): + def update(self, artists, state: ApplicationState, **kwargs): new_store = [] selected_idx = None for i, artist in enumerate(artists): @@ -157,6 +157,8 @@ class ArtistDetailPanel(Gtk.Box): ), } + update_order_token = 0 + def __init__(self, *args, **kwargs): super().__init__( *args, @@ -264,9 +266,9 @@ class ArtistDetailPanel(Gtk.Box): self.add(artist_info_box) def update(self, state: ApplicationState): + self.artist_id = state.selected_artist_id if state.selected_artist_id is None: self.artist_action_buttons.hide() - self.artist_id = None self.artist_indicator.set_text('') self.artist_name.set_markup('') self.artist_stats.set_markup('') @@ -280,8 +282,13 @@ class ArtistDetailPanel(Gtk.Box): self.albums = cast(List[Child], []) self.albums_list.update(None) else: + self.update_order_token += 1 self.artist_action_buttons.show() - self.update_artist_view(state.selected_artist_id, state=state) + self.update_artist_view( + state.selected_artist_id, + state=state, + order_token=self.update_order_token, + ) # TODO need to handle when this is force updated. Need to delete a bunch of # stuff and un-cache things. @@ -295,14 +302,17 @@ class ArtistDetailPanel(Gtk.Box): artist: ArtistWithAlbumsID3, state: ApplicationState, force=False, + order_token=None, ): - self.artist_id = artist.id + if order_token != self.update_order_token: + return + self.artist_indicator.set_text('ARTIST') self.artist_name.set_markup(util.esc(f'{artist.name}')) self.artist_stats.set_markup(self.format_stats(artist)) - self.update_artist_info(artist.id) - self.update_artist_artwork(artist) + self.update_artist_info(artist.id, order_token=order_token) + self.update_artist_artwork(artist, order_token=order_token) self.albums = artist.get('album', artist.get('child', [])) self.albums_list.update(artist) @@ -314,7 +324,12 @@ class ArtistDetailPanel(Gtk.Box): self, artist_info: ArtistInfo2, state: ApplicationState, + force=False, + order_token=None, ): + if order_token != self.update_order_token: + return + self.artist_bio.set_markup(util.esc(''.join(artist_info.biography))) self.play_shuffle_buttons.show_all() @@ -344,21 +359,35 @@ class ArtistDetailPanel(Gtk.Box): self, cover_art_filename, state: ApplicationState, + force=False, + order_token=None, ): + if order_token != self.update_order_token: + return + self.artist_artwork.set_from_file(cover_art_filename) self.artist_artwork.set_loading(False) # Event Handlers # ========================================================================= def on_view_refresh_click(self, *args): - self.update_artist_view(self.artist_id, force=True) + self.update_artist_view( + self.artist_id, + force=True, + update_order_token=self.update_order_token, + ) def on_download_all_click(self, btn): CacheManager.batch_download_songs( self.get_artist_songs(), - before_download=lambda: self.update_artist_view(self.artist_id), + before_download=lambda: self.update_artist_view( + self.artist_id, + update_order_token=self.update_order_token, + ), on_song_download_complete=lambda i: self.update_artist_view( - self.artist_id), + self.artist_id, + update_order_token=self.update_order_token, + ), ) def on_play_all_clicked(self, btn): diff --git a/sublime/ui/browse.py b/sublime/ui/browse.py index d206ee9..000c42a 100644 --- a/sublime/ui/browse.py +++ b/sublime/ui/browse.py @@ -30,6 +30,7 @@ class BrowsePanel(Gtk.Overlay): } id_stack = None + update_order_token = 0 def __init__(self): super().__init__() @@ -60,15 +61,20 @@ class BrowsePanel(Gtk.Overlay): if not CacheManager.ready: return - def do_update(id_stack): + self.update_order_token += 1 + + def do_update(id_stack, update_order_token): + if self.update_order_token != update_order_token: + return + self.root_directory_listing.update( - id_stack.result(), + id_stack, state=state, force=force, ) self.spinner.hide() - def calculate_path(): + def calculate_path(update_order_token): if state.selected_browse_element_id is None: return [] @@ -83,10 +89,14 @@ class BrowsePanel(Gtk.Overlay): id_stack.append(directory.id) current_dir_id = directory.parent - return id_stack + return id_stack, update_order_token - path_fut = CacheManager.create_future(calculate_path) - path_fut.add_done_callback(lambda f: GLib.idle_add(do_update, f)) + path_fut = CacheManager.create_future( + calculate_path, + self.update_order_token, + ) + path_fut.add_done_callback( + lambda f: GLib.idle_add(do_update, *f.result())) class ListAndDrilldown(Gtk.Paned): @@ -384,6 +394,8 @@ class DrilldownList(Gtk.Box): class IndexList(DrilldownList): + update_order_token = 0 + def update( self, selected_id, @@ -391,8 +403,13 @@ class IndexList(DrilldownList): force=False, **kwargs, ): + self.update_order_token += 1 self.selected_id = selected_id - self.update_store(force=force, state=state) + self.update_store( + force=force, + state=state, + order_token=self.update_order_token, + ) def on_refresh_clicked(self, _): self.update(self.selected_id, force=True) @@ -407,7 +424,11 @@ class IndexList(DrilldownList): artists, state: ApplicationState = None, force=False, + order_token=None, ): + if order_token != self.update_order_token: + return + self.do_update_store(artists) def on_download_state_change(self, song_id=None): @@ -415,6 +436,8 @@ class IndexList(DrilldownList): class MusicDirectoryList(DrilldownList): + update_order_token = 0 + def update( self, selected_id, @@ -424,7 +447,12 @@ class MusicDirectoryList(DrilldownList): ): self.directory_id = directory_id self.selected_id = selected_id - self.update_store(directory_id, force=force, state=state) + self.update_store( + directory_id, + force=force, + state=state, + order_token=self.update_order_token, + ) def on_refresh_clicked(self, _): self.update( @@ -440,7 +468,11 @@ class MusicDirectoryList(DrilldownList): directory, state: ApplicationState = None, force=False, + order_token=None, ): + if order_token != self.update_order_token: + return + self.do_update_store(directory.child) def on_download_state_change(self, song_id=None): diff --git a/sublime/ui/common/album_with_songs.py b/sublime/ui/common/album_with_songs.py index 228bbea..b969e5a 100644 --- a/sublime/ui/common/album_with_songs.py +++ b/sublime/ui/common/album_with_songs.py @@ -279,9 +279,11 @@ class AlbumWithSongs(Gtk.Box): on_failure=lambda self, e: self.set_loading(False), ) def update_album_songs( - self, - album: Union[AlbumWithSongsID3, Child, Directory], - state: ApplicationState, + self, + album: Union[AlbumWithSongsID3, Child, Directory], + state: ApplicationState, + force=False, + order_token=None, ): new_store = [ [ diff --git a/sublime/ui/player_controls.py b/sublime/ui/player_controls.py index f42f030..8828c00 100644 --- a/sublime/ui/player_controls.py +++ b/sublime/ui/player_controls.py @@ -57,6 +57,7 @@ class PlayerControls(Gtk.ActionBar): current_song = None current_device = None chromecasts: List[ChromecastPlayer] = [] + cover_art_update_order_token = 0 def __init__(self): Gtk.ActionBar.__init__(self) @@ -130,7 +131,12 @@ class PlayerControls(Gtk.ActionBar): # Update the current song information. # TODO add popup of bigger cover art photo here if state.current_song is not None: - self.update_cover_art(state.current_song.coverArt, size='70') + self.cover_art_update_order_token += 1 + self.update_cover_art( + state.current_song.coverArt, + size='70', + order_token=self.cover_art_update_order_token, + ) self.song_title.set_markup(util.esc(state.current_song.title)) self.album_name.set_markup(util.esc(state.current_song.album)) @@ -245,7 +251,16 @@ class PlayerControls(Gtk.ActionBar): before_download=lambda self: self.album_art.set_loading(True), on_failure=lambda self, e: self.album_art.set_loading(False), ) - def update_cover_art(self, cover_art_filename: str, state): + def update_cover_art( + self, + cover_art_filename: str, + state, + force=False, + order_token=None, + ): + if order_token != self.cover_art_update_order_token: + return + self.album_art.set_from_file(cover_art_filename) self.album_art.set_loading(False) diff --git a/sublime/ui/playlists.py b/sublime/ui/playlists.py index f3c1073..156e97e 100644 --- a/sublime/ui/playlists.py +++ b/sublime/ui/playlists.py @@ -177,9 +177,11 @@ class PlaylistList(Gtk.Box): on_failure=lambda self, e: self.loading_indicator.hide(), ) def update_list( - self, - playlists: List[PlaylistWithSongs], - state: ApplicationState, + self, + playlists: List[PlaylistWithSongs], + state: ApplicationState, + force=False, + order_token=None, ): new_store = [] selected_idx = None @@ -429,6 +431,8 @@ class PlaylistDetailPanel(Gtk.Overlay): self.playlist_view_loading_box.add(playlist_view_spinner) self.add_overlay(self.playlist_view_loading_box) + update_playlist_view_order_token = 0 + def update(self, state: ApplicationState, force=False): if state.selected_playlist_id is None: self.playlist_artwork.set_from_file(None) @@ -441,10 +445,12 @@ class PlaylistDetailPanel(Gtk.Overlay): self.playlist_view_loading_box.hide() self.playlist_artwork.set_loading(False) else: + self.update_playlist_view_order_token += 1 self.update_playlist_view( state.selected_playlist_id, state=state, force=force, + order_token=self.update_playlist_view_order_token, ) @util.async_callback( @@ -453,11 +459,17 @@ class PlaylistDetailPanel(Gtk.Overlay): on_failure=lambda self, e: self.playlist_view_loading_box.hide(), ) def update_playlist_view( - self, - playlist, - state: ApplicationState = None, - force=False, + self, + playlist, + state: ApplicationState = None, + force=False, + order_token=None, ): + if self.update_playlist_view_order_token != order_token: + return + + # If the selected playlist has changed, then clear the selections in + # the song list. if self.playlist_id != playlist.id: self.playlist_songs.get_selection().unselect_all() @@ -474,7 +486,10 @@ class PlaylistDetailPanel(Gtk.Overlay): self.playlist_stats.set_markup(self.format_stats(playlist)) # Update the artwork. - self.update_playlist_artwork(playlist.coverArt) + self.update_playlist_artwork( + playlist.coverArt, + order_token=order_token, + ) # Update the song list model. This requires some fancy diffing to # update the list. @@ -506,17 +521,26 @@ class PlaylistDetailPanel(Gtk.Overlay): on_failure=lambda self, e: self.playlist_artwork.set_loading(False), ) def update_playlist_artwork( - self, - cover_art_filename, - state: ApplicationState, + self, + cover_art_filename, + state: ApplicationState, + force=False, + order_token=None, ): + if self.update_playlist_view_order_token != order_token: + return + self.playlist_artwork.set_from_file(cover_art_filename) self.playlist_artwork.set_loading(False) # Event Handlers # ========================================================================= def on_view_refresh_click(self, button): - self.update_playlist_view(self.playlist_id, force=True) + self.update_playlist_view( + self.playlist_id, + force=True, + order_token=self.update_playlist_view_order_token, + ) def on_playlist_edit_button_click(self, button): dialog = EditPlaylistDialog( @@ -555,7 +579,11 @@ class PlaylistDetailPanel(Gtk.Overlay): def on_playlist_list_download_all_button_click(self, button): def download_state_change(*args): - GLib.idle_add(self.update_playlist_view, self.playlist_id) + GLib.idle_add( + lambda: self.update_playlist_view( + self.playlist_id, + order_token=self.update_playlist_view_order_token, + )) song_ids = [s[-1] for s in self.playlist_song_store] CacheManager.batch_download_songs( @@ -608,7 +636,11 @@ class PlaylistDetailPanel(Gtk.Overlay): allow_deselect = False def on_download_state_change(song_id=None): - GLib.idle_add(self.update_playlist_view, self.playlist_id) + GLib.idle_add( + lambda: self.update_playlist_view( + self.playlist_id, + order_token=self.update_playlist_view_order_token, + )) # Use the new selection instead of the old one for calculating what # to do the right click on. @@ -629,7 +661,11 @@ class PlaylistDetailPanel(Gtk.Overlay): playlist_id=self.playlist_id, song_index_to_remove=[p.get_indices()[0] for p in paths], ) - self.update_playlist_view(self.playlist_id, force=True) + self.update_playlist_view( + self.playlist_id, + force=True, + order_token=self.update_playlist_view_order_token, + ) remove_text = ( 'Remove ' + util.pluralize('song', len(song_ids)) @@ -678,7 +714,7 @@ class PlaylistDetailPanel(Gtk.Overlay): ) @util.async_callback(lambda *a, **k: CacheManager.get_playlist(*a, **k)) - def update_playlist_order(self, playlist, state: ApplicationState): + def update_playlist_order(self, playlist, state, **kwargs): self.playlist_view_loading_box.show_all() update_playlist_future = CacheManager.update_playlist( playlist_id=playlist.id, @@ -688,7 +724,11 @@ class PlaylistDetailPanel(Gtk.Overlay): update_playlist_future.add_done_callback( lambda f: GLib.idle_add( - lambda: self.update_playlist_view(playlist.id, force=True))) + lambda: self.update_playlist_view( + playlist.id, + force=True, + order_token=self.update_playlist_view_order_token, + ))) def format_stats(self, playlist): created_date = playlist.created.strftime('%B %d, %Y') diff --git a/sublime/ui/util.py b/sublime/ui/util.py index 0cd1abb..cd744ba 100644 --- a/sublime/ui/util.py +++ b/sublime/ui/util.py @@ -319,7 +319,14 @@ def async_callback( """ def decorator(callback_fn): @functools.wraps(callback_fn) - def wrapper(self, *args, state=None, **kwargs): + def wrapper( + self, + *args, + state=None, + order_token=None, + force=False, + **kwargs, + ): if before_download: on_before_download = ( lambda: GLib.idle_add(before_download, self)) @@ -334,11 +341,19 @@ def async_callback( on_failure(self, e) return - return GLib.idle_add(callback_fn, self, result, state) + return GLib.idle_add( + lambda: callback_fn( + self, + result, + state=state, + force=force, + order_token=order_token, + )) future: Future = future_fn( *args, before_download=on_before_download, + force=force, **kwargs, ) future.add_done_callback(future_callback)