[banshee/gtk3] ArtworkManager: prevent wrong inclusion of null artwork_id in cache

[Andrés Aragoneses <aaragoneses src gnome org>]

commit 3a72268f5cb31109b0414b198ca703c97174597d
Author: Andrés G. Aragoneses <knocte gmail com>
Date:   Mon Sep 9 03:54:38 2013 +0200

    ArtworkManager: prevent wrong inclusion of null artwork_id in cache
    
    The public method LookupScaleSurface(_,size,_) of ArtworkManager class
    calls another public method called LookupScalePixbuf(_,size). The latter
    was in charge of adding cached null artwork_ids to prevent subsequent
    slow lookups, however:
    
    1) If an invalid (as in, too small) size was specified, a null value was
    also returned by the callee LookupScalePixbuf().
    2) The caller LookupScaleSurface() was *also* inserting to null_artwork_ids
    when receiving a null value from LookupScalePixbuf().
    
    The consequence of the above was: if an invalid size ( < 10) was requested
    for a *valid* artwork_id, the cache would fill a null value for it, and all
    subsequent requests (even the ones with valid sizes) would receive null.
    
    The user-visible consequence of this was cover art not rendering in widgets
    such as ClassicTrackInfoDisplay or others. Surprisingly, this bug was more
    likely to be hit in the gtk3 branch when the NotificationArea extension was
    enabled, and not in the master branch (gtk2) or when this extension was
    disabled.
    
    The solution, implemented in this commit, is:
    - Stop querying the null_artwork_ids cache from the caller.
    - Stop adding elements to the null_artwork_ids cache in the caller.
    - Move from the caller the only scenario not covered by the callee,
    about checking if the pixbuf's Handle is IntPtr.Zero.
    - Add some comments clarifying some absences of null_artwork_ids
    manipulation.
    
    A unit test is also added for the bug (which failed before this fix,
    and passes now), as an extra layer of protection against future
    regressions. This unit test uses one of the embedded resources (PNG)
    that are already inside the Banshee.ThickClient assembly to provide
    dummy cover art.

 .../Banshee.Collection.Gui/ArtworkManager.cs       |   23 +++--
 .../Tests/ArtworkManagerTests.cs                   |  102 ++++++++++++++++++++
 .../Banshee.ThickClient/Banshee.ThickClient.csproj |    2 +
 src/Core/Banshee.ThickClient/Makefile.am           |    1 +
 4 files changed, 119 insertions(+), 9 deletions(-)
---
diff --git a/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/ArtworkManager.cs 
b/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/ArtworkManager.cs
index 6e1aa47..d3da931 100644
--- a/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/ArtworkManager.cs
+++ b/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/ArtworkManager.cs
@@ -1,10 +1,12 @@
 //
 // ArtworkManager.cs
 //
-// Author:
+// Authors:
 //   Aaron Bockover <abockover novell com>
+//   Andrés G. Aragoneses <knocte gmail com>
 //
 // Copyright (C) 2007-2008 Novell, Inc.
+// Copyright (C) 2013 Andrés G. Aragoneses
 //
 // Permission is hereby granted, free of charge, to any person obtaining
 // a copy of this software and associated documentation files (the
@@ -28,9 +30,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Text.RegularExpressions;
-
-using Mono.Unix;
 
 using Gdk;
 
@@ -68,6 +67,11 @@ namespace Banshee.Collection.Gui
 
         public ArtworkManager ()
         {
+            Init ();
+        }
+
+        protected virtual void Init ()
+        {
             AddCachedSize (36);
             AddCachedSize (40);
             AddCachedSize (42);
@@ -122,13 +126,9 @@ namespace Banshee.Collection.Gui
                 return surface;
             }
 
-            if (null_artwork_ids.Contains (id)) {
-                return null;
-            }
-
             Pixbuf pixbuf = LookupScalePixbuf (id, size);
             if (pixbuf == null || pixbuf.Handle == IntPtr.Zero) {
-                null_artwork_ids.Add (id);
+                // no need to add to null_artwork_ids here, LookupScalePixbuf already did it
                 return null;
             }
 
@@ -165,6 +165,8 @@ namespace Banshee.Collection.Gui
         public Pixbuf LookupScalePixbuf (string id, int size)
         {
             if (id == null || (size != 0 && size < 10)) {
+                // explicitly don't add this id into null_artwork_ids here,
+                // otherwise it would blacklist all other non-invalid sizes
                 return null;
             }
 
@@ -232,6 +234,9 @@ namespace Banshee.Collection.Gui
                     }
 
                     DisposePixbuf (pixbuf);
+                    if (scaled_pixbuf == null || scaled_pixbuf.Handle == IntPtr.Zero) {
+                        null_artwork_ids.Add (id);
+                    }
                     return scaled_pixbuf;
                 } catch {}
             }
diff --git a/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/Tests/ArtworkManagerTests.cs 
b/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/Tests/ArtworkManagerTests.cs
new file mode 100644
index 0000000..e29658e
--- /dev/null
+++ b/src/Core/Banshee.ThickClient/Banshee.Collection.Gui/Tests/ArtworkManagerTests.cs
@@ -0,0 +1,102 @@
+//
+// ArtworkManagerTests.cs
+//
+// Author:
+//   Andrés G. Aragoneses <knocte gmail com>
+//
+// Copyright 2013 Andrés G. Aragoneses
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+#if ENABLE_TESTS
+
+using Banshee.Base;
+using Hyena.Tests;
+
+using NUnit.Framework;
+using System;
+using System.Linq;
+using System.Reflection;
+using System.IO;
+using Gdk;
+
+namespace Banshee.Collection.Gui.Tests
+{
+    class CustomArtworkManager : ArtworkManager
+    {
+        internal static int SizeTest = 36;
+        protected override void Init ()
+        {
+            AddCachedSize (SizeTest);
+        }
+    }
+
+    [TestFixture]
+    public class ArtworkManagerTests : TestBase
+    {
+        static string ExtractPngFromResource ()
+        {
+            var first_image = Assembly.GetExecutingAssembly ().GetManifestResourceNames ().Where (n => 
n.EndsWith (".png")).First ();
+            var temp_png = Path.Combine (Path.GetTempPath (), first_image);
+            Stream s = Assembly.GetExecutingAssembly ().GetManifestResourceStream (first_image);
+            using (FileStream file = new FileStream (temp_png, FileMode.Create)) {
+                byte[] b = new byte[s.Length + 1];
+                s.Read (b, 0, Convert.ToInt32 (s.Length));
+                file.Write (b, 0, Convert.ToInt32 (b.Length - 1));
+                file.Flush ();
+            }
+            return temp_png;
+        }
+
+        static ArtworkManagerTests ()
+        {
+            GLib.GType.Init ();
+            Mono.Addins.AddinManager.Initialize (BinDir);
+            Banshee.IO.Provider.SetProvider (new Banshee.IO.SystemIO.Provider ());
+        }
+
+        [Test]
+        public void TestSizePath ()
+        {
+            var png_file_path = ExtractPngFromResource ();
+            string jpg_file_path = null;
+
+            try {
+                var artist_album_id = CoverArtSpec.CreateArtistAlbumId ("Metallica", "Master Of Puppets");
+                jpg_file_path = CoverArtSpec.GetPathForSize (artist_album_id, 
CustomArtworkManager.SizeTest); // i.e.: 
/home/knocte/.cache/media-art/36/album-d33f25dbd7dfb4817a7e99f6bc2de49e.jpg"
+                var pixbuf = new Pixbuf (png_file_path);
+                pixbuf.Save (jpg_file_path, "jpeg");
+
+                var artwork_manager = new CustomArtworkManager ();
+                Assert.IsNull (artwork_manager.LookupScaleSurface (artist_album_id, 1, false),
+                               "Should have got null at the first request, with an invalid size");
+                Assert.IsNotNull (artwork_manager.LookupScaleSurface (artist_album_id, 
CustomArtworkManager.SizeTest, false),
+                                  "Null at the second request, was null cached incorrectly?");
+
+            } finally {
+                File.Delete (png_file_path);
+                if (File.Exists (jpg_file_path)) {
+                    File.Delete (jpg_file_path);
+                }
+            }
+        }
+    }
+}
+
+#endif
\ No newline at end of file
diff --git a/src/Core/Banshee.ThickClient/Banshee.ThickClient.csproj 
b/src/Core/Banshee.ThickClient/Banshee.ThickClient.csproj
index 3e89d79..55a8677 100644
--- a/src/Core/Banshee.ThickClient/Banshee.ThickClient.csproj
+++ b/src/Core/Banshee.ThickClient/Banshee.ThickClient.csproj
@@ -152,6 +152,7 @@
     </EmbeddedResource>
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="Banshee.Collection.Gui\Tests\ArtworkManagerTests.cs" />
     <Compile Include="Banshee.Collection.Gui\TrackListView.cs" />
     <Compile Include="Banshee.Collection.Gui\ColumnCellAlbum.cs" />
     <Compile Include="Banshee.Collection.Gui\ArtworkManager.cs" />
@@ -330,5 +331,6 @@
   </ProjectExtensions>
   <ItemGroup>
     <Folder Include="Banshee.Gui.Widgets\Tests\" />
+    <Folder Include="Banshee.Collection.Gui\Tests\" />
   </ItemGroup>
 </Project>
diff --git a/src/Core/Banshee.ThickClient/Makefile.am b/src/Core/Banshee.ThickClient/Makefile.am
index 6e54ae7..cdfbd4e 100644
--- a/src/Core/Banshee.ThickClient/Makefile.am
+++ b/src/Core/Banshee.ThickClient/Makefile.am
@@ -31,6 +31,7 @@ SOURCES =  \
        Banshee.Collection.Gui/QueryFilterView.cs \
        Banshee.Collection.Gui/SearchableListView.cs \
        Banshee.Collection.Gui/TerseTrackListView.cs \
+       Banshee.Collection.Gui/Tests/ArtworkManagerTests.cs \
        Banshee.Collection.Gui/TrackFilterListView.cs \
        Banshee.Collection.Gui/TrackListView.cs \
        Banshee.Collection.Gui/XmlColumnController.cs \