[gnome-music/wip/mschraal/core-thumb-property: 1/16] coreartist: Rework retrieval

[Marinus Schraal <mschraal src gnome org>]

commit b13c8f924b1e6b59db4764c81c906ae691e78ded
Author: Marinus Schraal <mschraal gnome org>
Date:   Sun Apr 5 15:43:16 2020 +0200

    coreartist: Rework retrieval
    
    * Remove the use of the cached_thumbnail_uri property, just have this
    intermediate uri directly passed to StoreArtistArt.
    * Add StoreArtistArt class, which retrieves art from a remote uri and
    stores it in the media art cache.
    * Listen to thumbnail property changes directly to update
    ArtistArtStack.
    * Add error logic to TrackerWrapper Grilo resolve callback.

 gnomemusic/artistart.py                       | 108 ++-----------------
 gnomemusic/coreartist.py                      |  19 ++--
 gnomemusic/coregrilo.py                       |   4 +
 gnomemusic/grilowrappers/grltrackerwrapper.py |  20 +++-
 gnomemusic/storeartistart.py                  | 150 ++++++++++++++++++++++++++
 gnomemusic/widgets/artistartstack.py          |  27 +++--
 6 files changed, 206 insertions(+), 122 deletions(-)
---
diff --git a/gnomemusic/artistart.py b/gnomemusic/artistart.py
index 973c5369..7edfae42 100644
--- a/gnomemusic/artistart.py
+++ b/gnomemusic/artistart.py
@@ -141,9 +141,6 @@ class ArtistArt(GObject.GObject):
         if self._in_cache():
             return
 
-        self._coreartist.connect(
-            "notify::thumbnail", self._on_thumbnail_changed)
-
         application.props.coregrilo.get_artist_art(self._coreartist)
 
     def _in_cache(self):
@@ -151,103 +148,13 @@ class ArtistArt(GObject.GObject):
             self._artist, None, "artist")
         if (not success
                 or not thumb_file.query_exists()):
+            self._coreartist.props.thumbnail = "loading"
             return False
 
-        self._coreartist.props.cached_thumbnail_uri = thumb_file.get_path()
+        self._coreartist.props.thumbnail = thumb_file.get_path()
 
         return True
 
-    def _on_thumbnail_changed(self, coreartist, thumbnail):
-        uri = coreartist.props.thumbnail
-
-        if (uri is None
-                or uri == ""):
-            self._coreartist.props.cached_thumbnail_uri = ""
-            return
-
-        msg = Soup.Message.new("GET", uri)
-        self._soup_session.queue_message(msg, self._read_callback, None)
-
-    def _read_callback(self, src, result, data):
-        if result.props.status_code != 200:
-            self._log.debug(
-                "Failed to get remote art for the artist {} : {}".format(
-                    self._artist, result.props.reason_phrase))
-            return
-
-        try:
-            istream = Gio.MemoryInputStream.new_from_bytes(
-                result.props.response_body_data)
-        except GLib.Error as error:
-            self._log.warning(
-                "Error: {}, {}".format(error.domain, error.message))
-            self._coreartist.props.cached_thumbnail_uri = ""
-            return
-
-        try:
-            [tmp_file, iostream] = Gio.File.new_tmp()
-        except GLib.Error as error:
-            self._log.warning(
-                "Error: {}, {}".format(error.domain, error.message))
-            self._coreartist.props.cached_thumbnail_uri = ""
-            return
-
-        ostream = iostream.get_output_stream()
-        # FIXME: Passing the iostream here, otherwise it gets
-        # closed. PyGI specific issue?
-        ostream.splice_async(
-            istream, Gio.OutputStreamSpliceFlags.CLOSE_SOURCE
-            | Gio.OutputStreamSpliceFlags.CLOSE_TARGET, GLib.PRIORITY_LOW,
-            None, self._splice_callback, [tmp_file, iostream])
-
-    def _delete_callback(self, src, result, data):
-        try:
-            src.delete_finish(result)
-        except GLib.Error as error:
-            self._log.warning(
-                "Error: {}, {}".format(error.domain, error.message))
-
-    def _splice_callback(self, src, result, data):
-        tmp_file, iostream = data
-
-        iostream.close_async(
-            GLib.PRIORITY_LOW, None, self._close_iostream_callback, None)
-
-        try:
-            src.splice_finish(result)
-        except GLib.Error as error:
-            self._log.warning(
-                "Error: {}, {}".format(error.domain, error.message))
-            self._coreartist.props.cached_thumbnail_uri = ""
-            return
-
-        success, cache_path = MediaArt.get_path(self._artist, None, "artist")
-
-        if not success:
-            self._coreartist.props.cached_thumbnail_uri = ""
-            return
-
-        try:
-            # FIXME: I/O blocking
-            MediaArt.file_to_jpeg(tmp_file.get_path(), cache_path)
-        except GLib.Error as error:
-            self._log.warning(
-                "Error: {}, {}".format(error.domain, error.message))
-            self._coreartist.props.cached_thumbnail_uri = ""
-            return
-
-        self._in_cache()
-
-        tmp_file.delete_async(
-            GLib.PRIORITY_LOW, None, self._delete_callback, None)
-
-    def _close_iostream_callback(self, src, result, data):
-        try:
-            src.close_finish(result)
-        except GLib.Error as error:
-            self._log.warning(
-                "Error: {}, {}".format(error.domain, error.message))
-
 
 class ArtistCache(GObject.GObject):
     """Handles retrieval of MediaArt cache art
@@ -269,6 +176,8 @@ class ArtistCache(GObject.GObject):
         self._size = size
         self._scale = scale
 
+        self._loading_icon = DefaultIcon().get(
+            DefaultIcon.Type.LOADING, self._size, self._scale)
         self._default_icon = DefaultIcon().get(
             DefaultIcon.Type.ARTIST, self._size, self._scale)
 
@@ -296,11 +205,12 @@ class ArtistCache(GObject.GObject):
 
         :param CoreSong coresong: The CoreSong object to search art for
         """
-        thumbnail_uri = coreartist.props.cached_thumbnail_uri
-        if thumbnail_uri == "":
-            self.emit("result", self._default_icon)
+        thumbnail_uri = coreartist.props.thumbnail
+        if thumbnail_uri == "loading":
+            self.emit("result", self._loading_icon)
             return
-        elif thumbnail_uri is None:
+        elif thumbnail_uri == "generic":
+            self.emit("result", self._default_icon)
             return
 
         thumb_file = Gio.File.new_for_path(thumbnail_uri)
diff --git a/gnomemusic/coreartist.py b/gnomemusic/coreartist.py
index 9aeccd68..a8018581 100644
--- a/gnomemusic/coreartist.py
+++ b/gnomemusic/coreartist.py
@@ -110,20 +110,21 @@ class CoreArtist(GObject.GObject):
 
     @GObject.Property(type=str, default=None)
     def thumbnail(self):
+        """Artist art thumbnail retrieval
+
+        :return: The artist art location or "generic" or "loading"
+        :rtype: string
+        """
         if self._thumbnail is None:
-            self._thumbnail = ""
+            self._thumbnail = "loading"
             ArtistArt(self._application, self)
 
         return self._thumbnail
 
     @thumbnail.setter
     def thumbnail(self, value):
-        self._thumbnail = value
+        """Artist art thumbnail setter
 
-    @GObject.Property(type=str, default=None)
-    def cached_thumbnail_uri(self):
-        return self._cached_thumbnail_uri
-
-    @cached_thumbnail_uri.setter
-    def cached_thumbnail_uri(self, value):
-        self._cached_thumbnail_uri = value
+        :param string value: path, "generic" or "loading"
+        """
+        self._thumbnail = value
diff --git a/gnomemusic/coregrilo.py b/gnomemusic/coregrilo.py
index a7a4936e..fa977ab1 100644
--- a/gnomemusic/coregrilo.py
+++ b/gnomemusic/coregrilo.py
@@ -205,6 +205,10 @@ class CoreGrilo(GObject.GObject):
                 coresong, callback)
 
     def get_artist_art(self, coreartist):
+        """Retrieve artist art for the given CoreArtist
+
+        :param CoreArtist coreartist: CoreArtist to retrieve art for
+        """
         if "grl-tracker-source" in self._wrappers:
             self._wrappers["grl-tracker-source"].get_artist_art(coreartist)
 
diff --git a/gnomemusic/grilowrappers/grltrackerwrapper.py b/gnomemusic/grilowrappers/grltrackerwrapper.py
index 0cde75e4..652e2269 100644
--- a/gnomemusic/grilowrappers/grltrackerwrapper.py
+++ b/gnomemusic/grilowrappers/grltrackerwrapper.py
@@ -31,6 +31,7 @@ from gnomemusic.coreartist import CoreArtist
 from gnomemusic.coredisc import CoreDisc
 from gnomemusic.coresong import CoreSong
 from gnomemusic.grilowrappers.grltrackerplaylists import GrlTrackerPlaylists
+from gnomemusic.storeartistart import StoreArtistArt
 
 
 class GrlTrackerWrapper(GObject.GObject):
@@ -982,22 +983,33 @@ class GrlTrackerWrapper(GObject.GObject):
         return query
 
     def get_artist_art(self, coreartist):
+        """Retrieve artist art for the given CoreArtist
+
+        This retrieves art through Grilo online services only.
+
+        :param CoreArtist coreartist: CoreArtist to get art for
+        """
         media = coreartist.props.media
 
-        def _resolve_cb(source, op_id, resolved_media, data, error):
+        def art_resolved_cb(source, op_id, resolved_media, data, error):
+            if error:
+                self._log.warning("Error: {}".format(error))
+                coreartist.props.thumbnail = "generic"
+                return
+
             if resolved_media.get_thumbnail() is None:
-                coreartist.props.thumbnail = ""
+                coreartist.props.thumbnail = "generic"
                 return
 
             media.set_thumbnail(resolved_media.get_thumbnail())
-            coreartist.props.thumbnail = media.get_thumbnail()
+            StoreArtistArt(coreartist)
 
         full_options = Grl.OperationOptions()
         full_options.set_resolution_flags(
             Grl.ResolutionFlags.FULL | Grl.ResolutionFlags.IDLE_RELAY)
 
         self.props.source.resolve(
-            media, [Grl.METADATA_KEY_THUMBNAIL], full_options, _resolve_cb,
+            media, [Grl.METADATA_KEY_THUMBNAIL], full_options, art_resolved_cb,
             None)
 
     def stage_playlist_deletion(self, playlist):
diff --git a/gnomemusic/storeartistart.py b/gnomemusic/storeartistart.py
new file mode 100644
index 00000000..813355a2
--- /dev/null
+++ b/gnomemusic/storeartistart.py
@@ -0,0 +1,150 @@
+# Copyright 2020 The GNOME Music developers
+#
+# GNOME Music is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# GNOME Music is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with GNOME Music; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# The GNOME Music authors hereby grant permission for non-GPL compatible
+# GStreamer plugins to be used and distributed together with GStreamer
+# and GNOME Music.  This permission is above and beyond the permissions
+# granted by the GPL license by which GNOME Music is covered.  If you
+# modify this code, you may extend this exception to your version of the
+# code, but you are not obligated to do so.  If you do not wish to do so,
+# delete this exception statement from your version.
+
+import gi
+gi.require_versions({"MediaArt": "2.0", "Soup": "2.4"})
+from gi.repository import Gio, GLib, GObject, MediaArt, Soup
+
+from gnomemusic.musiclogger import MusicLogger
+
+
+class StoreArtistArt(GObject.Object):
+    """Stores Art in the MediaArt cache.
+    """
+
+    def __init__(self, coreartist):
+        """Initialize StoreArtistArt
+
+        :param CoreArtist coreartist: The core artist to store art for
+        """
+        self._coreartist = coreartist
+        self._log = MusicLogger()
+        self._soup_session = Soup.Session.new()
+
+        uri = coreartist.props.media.get_thumbnail()
+        if (uri is None
+                or uri == ""):
+            self._coreartist.props.thumbnail = "generic"
+
+            return
+
+        cache_dir = GLib.build_filenamev(
+            [GLib.get_user_cache_dir(), "media-art"])
+        cache_dir_file = Gio.File.new_for_path(cache_dir)
+        cache_dir_file.query_info_async(
+            Gio.FILE_ATTRIBUTE_ACCESS_CAN_READ, Gio.FileQueryInfoFlags.NONE,
+            GLib.PRIORITY_LOW, None, self._cache_dir_info_read, uri)
+
+    def _cache_dir_info_read(self, cache_dir_file, res, uri):
+        try:
+            cache_dir_file.query_info_finish(res)
+        except GLib.Error:
+            # directory does not exist yet
+            try:
+                cache_dir_file.make_directory(None)
+            except GLib.Error as error:
+                self._log.warning(
+                    "Error: {}, {}".format(error.domain, error.message))
+                self._coreartist.props.thumbnail = "generic"
+
+                return
+
+        msg = Soup.Message.new("GET", uri)
+        self._soup_session.queue_message(msg, self._read_callback, None)
+
+    def _read_callback(self, src, result, data):
+        if result.props.status_code != 200:
+            self._log.debug(
+                "Failed to get remote art: {}".format(
+                    result.props.reason_phrase))
+
+            return
+
+        try:
+            [tmp_file, iostream] = Gio.File.new_tmp()
+        except GLib.Error as error:
+            self._log.warning(
+                "Error: {}, {}".format(error.domain, error.message))
+            self._coreartist.props.thumbnail = "generic"
+            return
+
+        istream = Gio.MemoryInputStream.new_from_bytes(
+            result.props.response_body_data)
+        ostream = iostream.get_output_stream()
+        # FIXME: Passing the iostream here, otherwise it gets
+        # closed. PyGI specific issue?
+        ostream.splice_async(
+            istream, Gio.OutputStreamSpliceFlags.CLOSE_SOURCE
+            | Gio.OutputStreamSpliceFlags.CLOSE_TARGET, GLib.PRIORITY_LOW,
+            None, self._splice_callback, [tmp_file, iostream])
+
+    def _delete_callback(self, src, result, data):
+        try:
+            src.delete_finish(result)
+        except GLib.Error as error:
+            self._log.warning(
+                "Error: {}, {}".format(error.domain, error.message))
+
+    def _splice_callback(self, src, result, data):
+        tmp_file, iostream = data
+
+        iostream.close_async(
+            GLib.PRIORITY_LOW, None, self._close_iostream_callback, None)
+
+        try:
+            src.splice_finish(result)
+        except GLib.Error as error:
+            self._log.warning(
+                "Error: {}, {}".format(error.domain, error.message))
+            self._coreartist.props.thumbnail = "generic"
+            return
+
+        success, cache_path = MediaArt.get_path(
+            self._coreartist.props.artist, None, "artist")
+
+        if not success:
+            self._coreartist.props.thumbnail = "generic"
+            return
+
+        try:
+            # FIXME: I/O blocking
+            MediaArt.file_to_jpeg(tmp_file.get_path(), cache_path)
+        except GLib.Error as error:
+            self._log.warning(
+                "Error: {}, {}".format(error.domain, error.message))
+            self._coreartist.props.thumbnail = "generic"
+            return
+
+        self._coreartist.props.media.set_thumbnail(cache_path)
+        self._coreartist.props.thumbnail = cache_path
+
+        tmp_file.delete_async(
+            GLib.PRIORITY_LOW, None, self._delete_callback, None)
+
+    def _close_iostream_callback(self, src, result, data):
+        try:
+            src.close_finish(result)
+        except GLib.Error as error:
+            self._log.warning(
+                "Error: {}, {}".format(error.domain, error.message))
diff --git a/gnomemusic/widgets/artistartstack.py b/gnomemusic/widgets/artistartstack.py
index 5cf3bb9a..94b0208a 100644
--- a/gnomemusic/widgets/artistartstack.py
+++ b/gnomemusic/widgets/artistartstack.py
@@ -42,13 +42,13 @@ class ArtistArtStack(Gtk.Stack):
     _default_icon = DefaultIcon()
 
     def __init__(self, size=Art.Size.MEDIUM):
-        """Initialize the CoverStack
+        """Initialize the ArtStack
 
         :param Art.Size size: The size of the art used for the cover
         """
         super().__init__()
 
-        self._art = None
+        self._cache = None
         self._handler_id = None
         self._size = None
         self._timeout = None
@@ -67,11 +67,9 @@ class ArtistArtStack(Gtk.Stack):
         self.props.transition_type = Gtk.StackTransitionType.CROSSFADE
         self.props.visible_child_name = "loading"
 
-        self._loading_cover.props.visible = True
-        self._cover_a.props.visible = True
-        self._cover_b.props.visible = True
+        self.connect("destroy", self._on_destroy)
 
-        self.props.visible = True
+        self.show_all()
 
     @GObject.Property(type=object, flags=GObject.ParamFlags.READWRITE)
     def size(self):
@@ -103,16 +101,16 @@ class ArtistArtStack(Gtk.Stack):
         self._coreartist = coreartist
 
         self._coreartist.connect(
-            "notify::cached-thumbnail-uri", self._on_thumbnail_changed)
+            "notify::thumbnail", self._on_thumbnail_changed)
 
         if self._coreartist.props.thumbnail is not None:
             self._on_thumbnail_changed(self._coreartist, None)
 
     def _on_thumbnail_changed(self, coreartist, uri):
-        cache = ArtistCache(self.props.size, self.props.scale_factor)
-        cache.connect("result", self._on_cache_result)
+        self._cache = ArtistCache(self.props.size, self.props.scale_factor)
+        self._handler_id = self._cache.connect("result", self._on_cache_result)
 
-        cache.query(coreartist)
+        self._cache.query(coreartist)
 
     def _on_cache_result(self, cache, surface):
         if self._active_child == "B":
@@ -121,3 +119,12 @@ class ArtistArtStack(Gtk.Stack):
         else:
             self._cover_b.props.surface = surface
             self.props.visible_child_name = "B"
+
+    def _on_destroy(self, widget):
+        # If the stacm is destroyed while the art is updated, an error
+        # can occur once the art is retrieved because the CoverStack
+        # does not have children anymore.
+        if (self._cache is not None
+                and self._handler_id is not None):
+            self._cache.disconnect(self._handler_id)
+            self._handler_id = None