[gnome-music/wip/jfelder/untangle-mpris-playertoolbar] mpris: Untangle MPRIS and playertoolbar

[Jean Felder <jfelder src gnome org>]

commit 61573a2dc674c5a1728959a1aa14cbec8fb77f44
Author: Jean Felder <jfelder src gnome org>
Date:   Wed Apr 10 19:45:27 2019 +0200

    mpris: Untangle MPRIS and playertoolbar
    
    MPRIS is tied with PlayerToolbar because the thumbnail updating logic
    takes place in the CoverStack from PlayerToolbar.
    In fact, on every song change the following thing happens:
    * PlayerToolbar looks up for the song's thumbnail (via its
      cover_stack) and emits a "thumbnail-updated" signal once the
      operation is finished.
    * MPRIS sends the song's metadata without the thumbnail url (it has
      not been updated yet). A bit later, once the "thumbnail-updated"
      signal is received, the metadata are sent again with the correct
      thumbnail url.
    This can result in a glitter on some MPRIS clients: the client is
    updated with an empty thumbnail, and then, very quickly, with the
    correct one.
    
    Fix that issue by factoring out the thumbnail cache lookup and
    directly looking up the song thumbnail from MPRIS. The disadvantage of
    this solution is that the lookup_art_file_from_cache function is
    called two times on a song change. One call from PlayerToolbar and the
    other one from mpris. However, this function call is less costy than
    sending two times the song's metadata.
    
    Related: #43, #172
    Closes: #101

 gnomemusic/albumartcache.py         | 41 +++++++++++++++++++------------------
 gnomemusic/mpris.py                 | 25 +++++++++++-----------
 gnomemusic/widgets/playertoolbar.py |  9 --------
 3 files changed, 34 insertions(+), 41 deletions(-)
---
diff --git a/gnomemusic/albumartcache.py b/gnomemusic/albumartcache.py
index 01561f7e..e9f60826 100644
--- a/gnomemusic/albumartcache.py
+++ b/gnomemusic/albumartcache.py
@@ -42,6 +42,25 @@ import gnomemusic.utils as utils
 logger = logging.getLogger(__name__)
 
 
+@log
+def lookup_art_file_from_cache(media):
+    """Lookup MediaArt cache art of an album or song.
+
+    :param Grl.Media media: song or album
+    :returns: a cache file
+    :rtype: Gio.File
+    """
+    album = utils.get_album_title(media)
+    artist = utils.get_artist_name(media)
+
+    success, thumb_file = MediaArt.get_file(artist, album, "album")
+    if (not success
+            or not thumb_file.query_exists()):
+        return None
+
+    return thumb_file
+
+
 @log
 def _make_icon_frame(icon_surface, art_size=None, scale=1, default_icon=False):
     border = 3
@@ -213,7 +232,6 @@ class Art(GObject.GObject):
             pixbuf, self._scale, None)
         surface = _make_icon_frame(surface, self._size, self._scale)
         self._surface = surface
-        self._set_grilo_thumbnail_path()
 
         self.emit('finished')
 
@@ -284,18 +302,6 @@ class Art(GObject.GObject):
 
         return False
 
-    def _set_grilo_thumbnail_path(self):
-        # TODO: This sets the thumbnail path for the Grilo Media object
-        # to be used by MPRIS. However, calling this by default for
-        # every cache hit is unnecessary.
-        album = utils.get_album_title(self._media)
-        artist = utils.get_artist_name(self._media)
-
-        success, thumb_file = MediaArt.get_file(artist, album, "album")
-        if success:
-            self._media.set_thumbnail(
-                GLib.filename_to_uri(thumb_file.get_path(), None))
-
     @GObject.Property
     def surface(self):
         if self._surface is None:
@@ -339,13 +345,8 @@ class Cache(GObject.GObject):
 
         :param Grl.Media media: The media object to search art for
         """
-        album = utils.get_album_title(media)
-        artist = utils.get_artist_name(media)
-
-        success, thumb_file = MediaArt.get_file(artist, album, "album")
-
-        if (success
-                and thumb_file.query_exists()):
+        thumb_file = lookup_art_file_from_cache(media)
+        if thumb_file:
             thumb_file.read_async(
                 GLib.PRIORITY_LOW, None, self._open_stream, None)
             return
diff --git a/gnomemusic/mpris.py b/gnomemusic/mpris.py
index b74c4c53..4a8fcf15 100644
--- a/gnomemusic/mpris.py
+++ b/gnomemusic/mpris.py
@@ -23,6 +23,7 @@
 # code, but you are not obligated to do so.  If you do not wish to do so,
 # delete this exception statement from your version.
 
+from gnomemusic.albumartcache import lookup_art_file_from_cache
 from gnomemusic.gstplayer import Playback
 from gnomemusic.player import PlayerPlaylist, RepeatMode
 from gnomemusic.grilo import grilo
@@ -243,9 +244,6 @@ class MediaPlayer2Service(Server):
         self.player.connect('seek-finished', self._on_seek_finished)
         self.player.connect(
             'playlist-changed', self._on_player_playlist_changed)
-        self.player_toolbar = app.get_active_window()._player_toolbar
-        self.player_toolbar.connect(
-            'thumbnail-updated', self._on_thumbnail_updated)
         self._playlists = Playlists.get_default()
         self._playlists.connect(
             'playlist-created', self._on_playlists_count_changed)
@@ -315,8 +313,19 @@ class MediaPlayer2Service(Server):
             last_played_str = last_played.format("%FT%T%:z")
             metadata['xesam:lastUsed'] = GLib.Variant('s', last_played_str)
 
+        # If the media has already been part of an MPRIS playlist, its
+        # thumbnail is already set. Otherwise, try to look for it in the
+        # cache directory and set the media thumbnail for a future use.
+        # The search is only through the cache to prevent any delayed
+        # loading.
         art_url = media.get_thumbnail()
-        if art_url is not None:
+        if not art_url:
+            thumb_file = lookup_art_file_from_cache(media)
+            if thumb_file:
+                art_url = GLib.filename_to_uri(thumb_file.get_path())
+                media.set_thumbnail(art_url)
+
+        if art_url:
             metadata['mpris:artUrl'] = GLib.Variant('s', art_url)
 
         return metadata
@@ -451,14 +460,6 @@ class MediaPlayer2Service(Server):
                                },
                                [])
 
-    @log
-    def _on_thumbnail_updated(self, player, data=None):
-        self.PropertiesChanged(MediaPlayer2Service.MEDIA_PLAYER2_PLAYER_IFACE,
-                               {
-                                   'Metadata': GLib.Variant('a{sv}', self._get_metadata()),
-                               },
-                               [])
-
     @log
     def _on_player_state_changed(self, klass, args):
         playback_status = self._get_playback_status()
diff --git a/gnomemusic/widgets/playertoolbar.py b/gnomemusic/widgets/playertoolbar.py
index 311f74b4..de11eb93 100644
--- a/gnomemusic/widgets/playertoolbar.py
+++ b/gnomemusic/widgets/playertoolbar.py
@@ -42,10 +42,6 @@ class PlayerToolbar(Gtk.ActionBar):
     Contains the ui of playing a song with Music.
     """
 
-    __gsignals__ = {
-        'thumbnail-updated': (GObject.SignalFlags.RUN_FIRST, None, ()),
-    }
-
     __gtype_name__ = 'PlayerToolbar'
 
     _artist_label = Gtk.Template.Child()
@@ -79,7 +75,6 @@ class PlayerToolbar(Gtk.ActionBar):
         self._player = None
 
         self._cover_stack.props.size = Art.Size.XSMALL
-        self._cover_stack.connect('updated', self._on_cover_stack_updated)
 
         self._tooltip = TwoLineTip()
 
@@ -121,10 +116,6 @@ class PlayerToolbar(Gtk.ActionBar):
         seconds = int(progress_scale.get_value() / 60)
         self._progress_time_label.set_label(utils.seconds_to_string(seconds))
 
-    @log
-    def _on_cover_stack_updated(self, klass):
-        self.emit('thumbnail-updated')
-
     @Gtk.Template.Callback()
     @log
     def _on_prev_button_clicked(self, button):