Merge branch 'master' of ssh://git.debian.org/git/collab-maint/shotwell

12 files changed, 210 insertions, 93 deletions

diff --git a/Makefile b/Makefile
index bc1d5de..e3db565 100644
--- a/Makefile
+++ b/Makefile
@@ -2,7 +2,7 @@ PROGRAM = shotwell
 PROGRAM_THUMBNAILER = shotwell-video-thumbnailer
 PROGRAM_MIGRATOR = shotwell-settings-migrator
 
-VERSION = 0.20.0
+VERSION = 0.20.1
 GITVER := $(shell git log -n 1 2>/dev/null | head -n 1 | awk '{print $$2}')
 GETTEXT_PACKAGE = $(PROGRAM)
 BUILD_ROOT = 1
diff --git a/NEWS b/NEWS
index 6c8b8e9..baa165d 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,11 @@
+Shotwell 0.20.1 - 2 October 2014
+--------------------------------
+
+  * Corrects problems with navigating photos in full-screen mode (#737092)
+  * Better memory utilization via more conservative pixbuf cache (#715198)
+  * Fixes minor bugs detected by better Vala code analysis
+
+
 Shotwell 0.20.0 - 16 September 2014
 -----------------------------------
 
diff --git a/plugins/common/RESTSupport.vala b/plugins/common/RESTSupport.vala
index 6a64025..7334de6 100644
--- a/plugins/common/RESTSupport.vala
+++ b/plugins/common/RESTSupport.vala
@@ -463,7 +463,7 @@ public class XmlDocument {
         document = doc;
     }
 
-    ~RESTXmlDocument() {
+    ~XmlDocument() {
         delete document;
     }
 
diff --git a/src/AppWindow.vala b/src/AppWindow.vala
index 782f953..5a8e9c4 100644
--- a/src/AppWindow.vala
+++ b/src/AppWindow.vala
@@ -146,7 +146,11 @@ public class FullscreenWindow : PageWindow {
             return true;
         }
         
-        // Make sure this event gets propagated to the underlying window...
+        // propagate to this (fullscreen) window respecting "stop propagation" result...
+        if (base.key_press_event != null && base.key_press_event(event))
+            return true;
+        
+        // ... then propagate to the underlying window hidden behind this fullscreen one
         return AppWindow.get_instance().key_press_event(event);
     }
     
diff --git a/src/Commands.vala b/src/Commands.vala
index 0ad8ecb..04b771c 100644
--- a/src/Commands.vala
+++ b/src/Commands.vala
@@ -211,7 +211,7 @@ public abstract class GenericPhotoTransformationCommand : SingleDataSourceComman
         base(photo, name, explanation);
     }
     
-    ~GenericPhotoTransformationState() {
+    ~GenericPhotoTransformationCommand() {
         if (original_state != null)
             original_state.broken.disconnect(on_state_broken);
         
@@ -795,8 +795,16 @@ public class StraightenCommand : GenericPhotoTransformationCommand {
     }
     
     public override void execute_on_photo(Photo photo) {
+        // thaw collection so both alterations are signalled at the same time
+        DataCollection? collection = photo.get_membership();
+        if (collection != null)
+            collection.freeze_notifications();
+        
         photo.set_straighten(theta);
         photo.set_crop(crop);
+        
+        if (collection != null)
+            collection.thaw_notifications();
     }
 }
 
diff --git a/src/Photo.vala b/src/Photo.vala
index 34b2676..98a3175 100644
--- a/src/Photo.vala
+++ b/src/Photo.vala
@@ -199,7 +199,11 @@ public abstract class Photo : PhotoSource, Dateable {
 
     // The number of seconds we should hold onto a precached copy of the original image; if
     // it hasn't been accessed in this many seconds, discard it to conserve memory.
-    private const int PRECACHE_TIME_TO_LIVE = 180;
+    private const int SOURCE_PIXBUF_TIME_TO_LIVE_SEC = 10;
+    
+    // min and max size of source pixbuf cache LRU
+    private const int SOURCE_PIXBUF_MIN_LRU_COUNT = 1;
+    private const int SOURCE_PIXBUF_MAX_LRU_COUNT = 3;
     
     // Minimum raw embedded preview size we're willing to accept; any smaller than this, and 
     // it's probably intended primarily for use only as a thumbnail and won't look good on the
@@ -293,6 +297,35 @@ public abstract class Photo : PhotoSource, Dateable {
         public PhotoFileReader editable;
     }
     
+    private class CachedPixbuf {
+        public Photo photo;
+        public Gdk.Pixbuf pixbuf;
+        public Timer last_touched = new Timer();
+        
+        public CachedPixbuf(Photo photo, Gdk.Pixbuf pixbuf) {
+            this.photo = photo;
+            this.pixbuf = pixbuf;
+        }
+    }
+    
+    // The first time we have to run the pipeline on an image, we'll precache
+    // a copy of the unscaled, unmodified version; this allows us to operate
+    // directly on the image data quickly without re-fetching it at the top
+    // of the pipeline, which can cause significant lag with larger images.
+    //
+    // This adds a small amount of (automatically garbage-collected) memory
+    // overhead, but greatly simplifies the pipeline, since scaling can now
+    // be blithely ignored, and most of the pixel operations are fast enough
+    // that the app remains responsive, even with 10MP images.
+    //
+    // In order to make sure we discard unneeded precaches in a timely fashion,
+    // we spawn a timer when the unmodified pixbuf is first precached; if the
+    // timer elapses and the pixbuf hasn't been needed again since then, we'll
+    // discard it and free up the memory.  The cache also has an LRU to prevent
+    // runaway amounts of memory from being stored (see SOURCE_PIXBUF_LRU_COUNT)
+    private static Gee.LinkedList<CachedPixbuf>? source_pixbuf_cache = null;
+    private static uint discard_source_id = 0;
+    
     // because fetching individual items from the database is high-overhead, store all of
     // the photo row in memory
     protected PhotoRow row;
@@ -308,23 +341,6 @@ public abstract class Photo : PhotoSource, Dateable {
     private OneShotScheduler remove_editable_scheduler = null;
     
     protected bool can_rotate_now = true;
-
-    // The first time we have to run the pipeline on an image, we'll precache
-    // a copy of the unscaled, unmodified version; this allows us to operate
-    // directly on the image data quickly without re-fetching it at the top
-    // of the pipeline, which can cause significant lag with larger images.
-    //
-    // This adds a small amount of (automatically garbage-collected) memory
-    // overhead, but greatly simplifies the pipeline, since scaling can now
-    // be blithely ignored, and most of the pixel operations are fast enough
-    // that the app remains responsive, even with 10MP images.
-    //
-    // In order to make sure we discard unneeded precaches in a timely fashion,
-    // we spawn a timer when the unmodified pixbuf is first precached; if the
-    // timer elapses and the pixbuf hasn't been needed again since then, we'll
-    // discard it and free up the memory.
-    private Gdk.Pixbuf unmodified_precached = null;
-    private GLib.Timer secs_since_access = null;
     
     // RAW only: developed backing photos.
     private Gee.HashMap<RawDeveloper, BackingPhotoRow?>? developments = null;
@@ -455,6 +471,19 @@ public abstract class Photo : PhotoSource, Dateable {
         cached_exposure_time = this.row.exposure_time;
     }
     
+    protected static void init_photo() {
+        source_pixbuf_cache = new Gee.LinkedList<CachedPixbuf>();
+    }
+    
+    protected static void terminate_photo() {
+        source_pixbuf_cache = null;
+        
+        if (discard_source_id != 0) {
+            Source.remove(discard_source_id);
+            discard_source_id = 0;
+        }
+    }
+    
     protected virtual void notify_editable_replaced(File? old_file, File? new_file) {
         editable_replaced(old_file, new_file);
     }
@@ -785,7 +814,7 @@ public abstract class Photo : PhotoSource, Dateable {
             if (!is_raw_developer_complete(d)) {
                 develop_photo(d);
                 try {
-                    populate_prefetched();
+                    get_prefetched_copy();
                 } catch (Error e) {
                     // couldn't reload the freshly-developed image, nothing to display
                     return;
@@ -826,7 +855,7 @@ public abstract class Photo : PhotoSource, Dateable {
         }
         
         notify_altered(new Alteration("image", "developer"));
-        discard_prefetched(true);
+        discard_prefetched();
     }
     
     public RawDeveloper get_raw_developer() {
@@ -3204,64 +3233,123 @@ public abstract class Photo : PhotoSource, Dateable {
     public override Gdk.Pixbuf get_pixbuf(Scaling scaling) throws Error {
         return get_pixbuf_with_options(scaling);
     }
-
+    
     /**
-     * @brief Populates the cached version of the unmodified image.
+     * One-stop shopping for the source pixbuf cache.
+     *
+     * The source pixbuf cache holds untransformed, unscaled (full-sized) pixbufs of Photo objects.
+     * These can be rather large and shouldn't be held in memory for too long, nor should many be
+     * allowed to stack up.
+     *
+     * If locate is non-null, a source pixbuf is returned for the Photo.  If keep is true, the
+     * pixbuf is stored in the cache.  (Thus, passing a Photo w/ keep == false will drop the cached
+     * pixbuf.)  If Photo is non-null but keep is false, null is returned.
+     *
+     * Whether locate is null or not, the cache is walked in its entirety, dropping expired pixbufs
+     * and dropping excessive pixbufs from the LRU.  Locating a Photo "touches" the pixbuf, i.e.
+     * it moves to the head of the LRU.
      */
-    public void populate_prefetched() throws Error {
-        lock (unmodified_precached) {
-            // If we don't have it already, precache the original...
-            if (unmodified_precached == null) {
-                unmodified_precached = load_raw_pixbuf(Scaling.for_original(), Exception.ALL, BackingFetchMode.SOURCE);
-                secs_since_access = new GLib.Timer();
-                GLib.Timeout.add_seconds(5, (GLib.SourceFunc)discard_prefetched);
-                debug("spawning new precache timeout for %s", this.to_string()); 
+    private static Gdk.Pixbuf? run_source_pixbuf_cache(Photo? locate, bool keep) throws Error {
+        lock (source_pixbuf_cache) {
+            CachedPixbuf? found = null;
+            
+            // walk list looking for photo to locate (if specified), dropping expired and LRU'd
+            // pixbufs along the way
+            double min_elapsed = double.MAX;
+            int count = 0;
+            Gee.Iterator<CachedPixbuf> iter = source_pixbuf_cache.iterator();
+            while (iter.next()) {
+                CachedPixbuf cached_pixbuf = iter.get();
+                
+                double elapsed = Math.trunc(cached_pixbuf.last_touched.elapsed()) + 1;
+                
+                if (locate != null && cached_pixbuf.photo.equals(locate)) {
+                    // found it, remove and reinsert at head of LRU (below)...
+                    iter.remove();
+                    found = cached_pixbuf;
+                    
+                    // ...that's why the counter is incremented
+                    count++;
+                } else if (elapsed >= SOURCE_PIXBUF_TIME_TO_LIVE_SEC) {
+                    iter.remove();
+                } else if (count >= SOURCE_PIXBUF_MAX_LRU_COUNT) {
+                    iter.remove();
+                } else {
+                    // find the item with the least elapsed time to reschedule a cache trim (to
+                    // prevent onesy-twosy reschedules)
+                    min_elapsed = double.min(elapsed, min_elapsed);
+                    count++;
+                }
+            }
+            
+            // if not found and trying to locate one and keep it, generate now
+            if (found == null && locate != null && keep) {
+                found = new CachedPixbuf(locate,
+                    locate.load_raw_pixbuf(Scaling.for_original(), Exception.ALL, BackingFetchMode.SOURCE));
+            } else if (found != null) {
+                // since it was located, touch it so it doesn't expire
+                found.last_touched.start();
+            }
+            
+            // if keeping it, insert at head of LRU
+            if (found != null && keep) {
+                source_pixbuf_cache.insert(0, found);
+                
+                // since this is (re-)inserted, count its elapsed time too ... w/ min_elapsed, this
+                // is almost guaranteed to be the min, since it was was touched mere clock cycles
+                // ago...
+                min_elapsed = double.min(found.last_touched.elapsed(), min_elapsed);
+                
+                // ...which means don't need to readjust the min_elapsed when trimming off excess
+                // due to adding back an element
+                while(source_pixbuf_cache.size > SOURCE_PIXBUF_MAX_LRU_COUNT)
+                    source_pixbuf_cache.poll_tail();
             }
+            
+            // drop expiration timer...
+            if (discard_source_id != 0) {
+                Source.remove(discard_source_id);
+                discard_source_id = 0;
+            }
+            
+            // ...only reschedule if there's something to expire
+            if (source_pixbuf_cache.size > SOURCE_PIXBUF_MIN_LRU_COUNT) {
+                assert(min_elapsed >= 0.0);
+                
+                // round-up to avoid a bunch of zero-second timeouts
+                uint retry_sec = SOURCE_PIXBUF_TIME_TO_LIVE_SEC - ((uint) Math.trunc(min_elapsed));
+                discard_source_id = Timeout.add_seconds(retry_sec, trim_source_pixbuf_cache, Priority.LOW);
+            }
+            
+            return found != null ? found.pixbuf : null;
         }
     }
-
+    
+    private static bool trim_source_pixbuf_cache() {
+        try {
+            run_source_pixbuf_cache(null, false);
+        } catch (Error err) {
+        }
+        
+        return false;
+    }
+    
     /**
      * @brief Get a copy of what's in the cache.
      *
-     * @return A Pixbuf with the image data from unmodified_precached.
+     * @return A copy of the Pixbuf with the image data from unmodified_precached.
      */
     public Gdk.Pixbuf get_prefetched_copy() throws Error {
-        lock (unmodified_precached) {
-            if (unmodified_precached == null) {
-                try {
-                    populate_prefetched();
-                } catch (Error e) {
-                    message("pixbuf for %s could not be loaded: %s", to_string(), e.message);
-                    
-                    throw e;
-                }
-            }
-
-            return unmodified_precached.copy();
-        }
+        return run_source_pixbuf_cache(this, true).copy();
     }
 
     /**
      * @brief Discards the cached version of the unmodified image.
-     *
-     * @param immed Whether the cached version should be discarded now, or not.
      */
-    public bool discard_prefetched(bool immed = false) {
-        lock (unmodified_precached) {
-            if (secs_since_access == null)
-                return false;
-            
-            double tmp;
-            if ((secs_since_access.elapsed(out tmp) > PRECACHE_TIME_TO_LIVE) || (immed)) {
-                debug("pipeline not run in over %d seconds or got immediate command, discarding " + 
-                    "cached original for %s",
-                    PRECACHE_TIME_TO_LIVE, to_string());
-                unmodified_precached = null;
-                secs_since_access = null;
-                return false;
-            }
-
-            return true;
+    public void discard_prefetched() {
+        try {
+            run_source_pixbuf_cache(this, false);
+        } catch (Error err) {
         }
     }
     
@@ -3283,7 +3371,7 @@ public abstract class Photo : PhotoSource, Dateable {
         Timer timer = new Timer();
         Timer total_timer = new Timer();
         double redeye_time = 0.0, crop_time = 0.0, adjustment_time = 0.0, orientation_time = 0.0,
-            straighten_time = 0.0;
+            straighten_time = 0.0, scale_time = 0.0;
 
         total_timer.start();
 #endif
@@ -3329,13 +3417,8 @@ public abstract class Photo : PhotoSource, Dateable {
         //
         // Image load-and-decode
         //
-        populate_prefetched();
-
-        Gdk.Pixbuf pixbuf = get_prefetched_copy();
         
-        // remember to delete the cached copy if it isn't being used.
-        secs_since_access.start();
-        debug("pipeline being run against %s, timer restarted.", this.to_string());
+        Gdk.Pixbuf pixbuf = get_prefetched_copy();
         
         //
         // Image transformation pipeline
@@ -3405,15 +3488,15 @@ public abstract class Photo : PhotoSource, Dateable {
 #endif
         }
         
-#if MEASURE_PIPELINE
-        debug("PIPELINE %s (%s): redeye=%lf crop=%lf adjustment=%lf orientation=%lf total=%lf",
-            to_string(), scaling.to_string(), redeye_time, crop_time, adjustment_time, 
-            orientation_time, total_timer.elapsed());
-#endif
-
         // scale the scratch image, as needed.
         if (is_scaled) {
+#if MEASURE_PIPELINE
+            timer.start();
+#endif
             pixbuf = pixbuf.scale_simple(scaled_to_viewport.width, scaled_to_viewport.height, Gdk.InterpType.BILINEAR);
+#if MEASURE_PIPELINE
+            scale_time = timer.elapsed();
+#endif
         }
 
         // color adjustment; we do this dead last, since, if an image has been scaled down,
@@ -3434,7 +3517,13 @@ public abstract class Photo : PhotoSource, Dateable {
         // the pixbuf, and must be accounted for the test to be valid.
         if ((is_scaled) && (!is_straightened))
             assert(scaled_to_viewport.approx_equals(Dimensions.for_pixbuf(pixbuf), SCALING_FUDGE));
-
+        
+#if MEASURE_PIPELINE
+        debug("PIPELINE %s (%s): redeye=%lf crop=%lf adjustment=%lf orientation=%lf straighten=%lf scale=%lf total=%lf",
+            to_string(), scaling.to_string(), redeye_time, crop_time, adjustment_time,
+            orientation_time, straighten_time, scale_time, total_timer.elapsed());
+#endif
+        
         return pixbuf;
     }
 
@@ -4032,12 +4121,12 @@ public abstract class Photo : PhotoSource, Dateable {
 
         // at this point, any image date we have cached is stale,
         // so delete it and force the pipeline to re-fetch it
-        discard_prefetched(true);
+        discard_prefetched();
     }
     
     private void on_reimport_editable() {
         // delete old image data and force the pipeline to load new from file.
-        discard_prefetched(true);
+        discard_prefetched();
         
         debug("Reimporting editable for %s", to_string());
         try {
@@ -4890,6 +4979,8 @@ public class LibraryPhoto : Photo, Flaggable, Monitorable {
     }
     
     public static void init(ProgressMonitor? monitor = null) {
+        init_photo();
+        
         global = new LibraryPhotoSourceCollection();
         
         // prefetch all the photos from the database and add them to the global collection ...
@@ -4921,6 +5012,7 @@ public class LibraryPhoto : Photo, Flaggable, Monitorable {
     }
     
     public static void terminate() {
+        terminate_photo();
     }
     
     // This accepts a PhotoRow that was prepared with Photo.prepare_for_import and
diff --git a/src/PixbufCache.vala b/src/PixbufCache.vala
index 8b8f276..0708f5e 100644
--- a/src/PixbufCache.vala
+++ b/src/PixbufCache.vala
@@ -265,6 +265,10 @@ public class PixbufCache : Object {
             return;
         }
         
+#if TRACE_PIXBUF_CACHE
+        debug("%s %s fetched into pixbuf cache", type.to_string(), job.photo.to_string());
+#endif
+        
         encache(job.photo, job.pixbuf);
         
         // fire signal
@@ -279,16 +283,14 @@ public class PixbufCache : Object {
             Photo photo = (Photo) object;
             
             if (in_progress.has_key(photo)) {
-                // Load is in progress, must cancel.
+                // Load is in progress, must cancel, but consider in-cache (since it was decached
+                // before being put into progress)
                 in_progress.get(photo).cancel();
                 in_progress.unset(photo);
+            } else if (!cache.has_key(photo)) {
                 continue;
             }
             
-            // only interested if in this cache
-            if (!cache.has_key(photo))
-                continue;
-            
             decache(photo);
             
 #if TRACE_PIXBUF_CACHE
diff --git a/src/SearchFilter.vala b/src/SearchFilter.vala
index 9769195..e8f0986 100644
--- a/src/SearchFilter.vala
+++ b/src/SearchFilter.vala
@@ -694,7 +694,7 @@ public class SearchFilterToolbar : Gtk.Toolbar {
             this.add(button);
         }
         
-        ~ToggleActionButton() {
+        ~ToggleActionToolButton() {
             button.clicked.disconnect(on_button_activate);
         }
         
diff --git a/src/Tag.vala b/src/Tag.vala
index 1ae76ba..450af20 100644
--- a/src/Tag.vala
+++ b/src/Tag.vala
@@ -626,7 +626,7 @@ public class Tag : DataSource, ContainerSource, Proxyable, Indexable {
         string built = builder.str;
         
         if (built.length >= separator.length)
-            if (built.substring(built.length - separator.length, separator.length) == separator);
+            if (built.substring(built.length - separator.length, separator.length) == separator)
                 built = built.substring(0, built.length - separator.length);
         
         if (end != null)
diff --git a/src/direct/DirectPhoto.vala b/src/direct/DirectPhoto.vala
index 98886ba..f7271ba 100644
--- a/src/direct/DirectPhoto.vala
+++ b/src/direct/DirectPhoto.vala
@@ -38,6 +38,8 @@ public class DirectPhoto : Photo {
     }
 
     public static void init(File initial_file) {
+        init_photo();
+        
         global = new DirectPhotoSourceCollection(initial_file);
         DirectPhoto photo;
         string? reason = global.fetch(initial_file, out photo, false);
@@ -47,6 +49,7 @@ public class DirectPhoto : Photo {
     }
     
     public static void terminate() {
+        terminate_photo();
     }
 
     // Gets the dimensions of this photo's pixbuf when scaled to original
@@ -257,7 +260,7 @@ public class DirectPhotoSourceCollection : DatabaseSourceCollection {
     }
     
     public void reimport_photo(DirectPhoto photo) {
-        photo.discard_prefetched(true);
+        photo.discard_prefetched();
         DirectPhoto reimported_photo;
         fetch(photo.get_file(), out reimported_photo, true);
     }
diff --git a/src/searches/SavedSearchDialog.vala b/src/searches/SavedSearchDialog.vala
index b425cfb..da7f7db 100644
--- a/src/searches/SavedSearchDialog.vala
+++ b/src/searches/SavedSearchDialog.vala
@@ -522,7 +522,7 @@ public class SavedSearchDialog {
             update_date_labels();
         }
         
-        ~SearchRowRating() {
+        ~SearchRowDate() {
             context.changed.disconnect(on_changed);
         }
         
diff --git a/src/tags/HierarchicalTagUtilities.vala b/src/tags/HierarchicalTagUtilities.vala
index 985491d..dbb7165 100644
--- a/src/tags/HierarchicalTagUtilities.vala
+++ b/src/tags/HierarchicalTagUtilities.vala
@@ -173,7 +173,7 @@ class HierarchicalTagUtilities {
                 return;
 
             Tag? t = null;
-            if (Tag.global.exists(actual_path));
+            if (Tag.global.exists(actual_path))
                 t = Tag.for_path(actual_path);
             
             if (t != null && t.get_hierarchical_children().size == 0)