Make MetadataWriter more robust

src/DirectoryMonitor.vala

@@ -925,7 +925,7 @@ public class DirectoryMonitor : Object {
                 foreach (FileInfo info in infos) {
                     // we don't deal with hidden files or directories
                     if (info.get_is_hidden ()) {
-                        warning ("Skipping hidden file/directory %s",
+                        debug ("Skipping hidden file/directory %s",
                                  dir.get_child (info.get_name ()).get_path ());
 
                         continue;

src/MetadataWriter.vala

@@ -33,8 +33,8 @@ public class MetadataWriter : Object {
     private class CommitJob : BackgroundJob {
         public LibraryPhoto photo;
         public Gee.Set<string>? current_keywords;
-        public Photo.ReimportMasterState reimport_master_state = null;
-        public Photo.ReimportEditableState reimport_editable_state = null;
+        public Photo.ReimportMasterState? reimport_master_state = null;
+        public Photo.ReimportEditableState? reimport_editable_state = null;
         public Error? err = null;
 
         public CommitJob (MetadataWriter owner, LibraryPhoto photo, Gee.Set<string>? keywords) {
@@ -58,38 +58,22 @@ public class MetadataWriter : Object {
             // otherwise, we'll end up ruining the original, and as such, breaking the
             // ability to revert to it.
             bool skip_orientation = photo.has_editable ();
-
-            if (!photo.get_master_file_format ().can_write_metadata ())
-                return;
-
             PhotoMetadata metadata = photo.get_master_metadata ();
             if (update_metadata (metadata, skip_orientation)) {
-                LibraryMonitor.blacklist_file (photo.get_master_file (), "MetadataWriter.commit_master");
-                try {
-                    photo.persist_master_metadata (metadata, out reimport_master_state);
-                } finally {
-                    LibraryMonitor.unblacklist_file (photo.get_master_file ());
-                }
+                // Photo does necessary checks and may throw error
+                photo.persist_master_metadata (metadata, out reimport_master_state);
             }
         }
 
         private void commit_editable () throws Error {
-            if (!photo.has_editable () || !photo.get_editable_file_format ().can_write_metadata ())
-                return;
-
             PhotoMetadata? metadata = photo.get_editable_metadata ();
-            assert (metadata != null);
-
             if (update_metadata (metadata)) {
-                LibraryMonitor.blacklist_file (photo.get_editable_file (), "MetadataWriter.commit_editable");
-                try {
-                    photo.persist_editable_metadata (metadata, out reimport_editable_state);
-                } finally {
-                    LibraryMonitor.unblacklist_file (photo.get_editable_file ());
-                }
+                // Photo does necessary checks and may throw error
+                photo.persist_editable_metadata (metadata, out reimport_editable_state);
             }
         }
 
+        // Vala compiler returns false if metadata null
         private bool update_metadata (PhotoMetadata metadata, bool skip_orientation = false) {
             bool changed = false;
 
@@ -534,19 +518,21 @@ public class MetadataWriter : Object {
 
         // mark all the photos as dirty
         if (!already_marked) {
-            try {
-                LibraryPhoto.global.transaction_controller.begin ();
-
-                foreach (LibraryPhoto photo in photos)
+            // Ensure transaction committed by moving outside try/catch
+            LibraryPhoto.global.transaction_controller.begin ();
+            foreach (LibraryPhoto photo in photos) {
+                try {
                     photo.set_master_metadata_dirty (true);
-
-                LibraryPhoto.global.transaction_controller.commit ();
-            } catch (Error err) {
-                if (err is DatabaseError)
-                    AppWindow.database_error ((DatabaseError) err);
-                else
-                    error ("Unable to mark metadata as dirty: %s", err.message);
+                } catch (Error err) {
+                    if (err is DatabaseError) {
+                        AppWindow.database_error ((DatabaseError) err); // Causes panic
+                    } else {
+                        critical ("Unable to mark metadata as dirty: %s", err.message);
+                    }
+                }
             }
+
+            LibraryPhoto.global.transaction_controller.commit ();
         }
 
         // ok to drop this on the floor, now that they're marked dirty (will attempt to write them
@@ -558,12 +544,19 @@ public class MetadataWriter : Object {
         debug ("[%s] adding %d photos to dirty list", reason, photos.size);
 #endif
 
+        int queued = 0;
         foreach (LibraryPhoto photo in photos) {
-            bool enqueued = dirty.enqueue (photo);
-            assert (enqueued);
+            if (photo.is_master_metadata_dirty ()) {
+                bool enqueued = dirty.enqueue (photo);
+                if (!enqueued) {
+                    critical ("Failed to enqueue photo %s", photo.get_basename ());
+                } else {
+                    queued++;
+                }
+            }
         }
 
-        count_enqueued_work (photos.size, true);
+        count_enqueued_work (queued, true);
     }
 
     private void cancel_all (bool wait) {
@@ -588,9 +581,10 @@ public class MetadataWriter : Object {
 
         if (dirty.contains (photo)) {
             bool removed = dirty.remove_first (photo);
-            assert (removed);
-
-            assert (!dirty.contains (photo));
+            if (!removed || dirty.contains (photo)) {
+                // Either queue did not contain photo or there was more than one entry. Doesn't seem fatal?
+                critical ("Cancel job: failed to remove photo from dirty HashTimedQueue");
+            }
 
             count_cancelled_work (1, false);
             cancelled = true;
@@ -626,14 +620,21 @@ public class MetadataWriter : Object {
 
     private void on_update_completed (BackgroundJob j) {
         CommitJob job = (CommitJob) j;
+        if (ignore_photo_alteration == job.photo) { // Guard against re-entering
+            return;
+        }
 
         if (job.err != null)
             warning ("Unable to update metadata for %s: %s", job.photo.to_string (), job.err.message);
         else
             message ("Completed writing metadata for %s", job.photo.to_string ());
 
         bool removed = pending.unset (job.photo);
-        assert (removed);
+        if (!removed) {
+            critical ("Photo was not in pending - ignore update completed");
+            // Do we need to return or continue or terminate? Presumably the photo was not in the queue?
+            // Other errors continued so do same for now;
+        }
 
         // since there's potentially multiple state-change operations here, use the transaction
         // controller
@@ -642,7 +643,12 @@ public class MetadataWriter : Object {
         if (job.reimport_master_state != null || job.reimport_editable_state != null) {
             // finish_update_*_metadata are going to issue an "altered" signal, and we want to
             // ignore it
-            assert (ignore_photo_alteration == null);
+            if (ignore_photo_alteration != null) {
+                critical ("ignore_photo_alteration is not null ");
+                ///TODO If this happens in practice then we need a way of pausing the job and calling this function later
+                // when ignore_photo_alteration becomes null
+            }
+
             ignore_photo_alteration = job.photo;
             try {
                 if (job.reimport_master_state != null)
@@ -651,10 +657,8 @@ public class MetadataWriter : Object {
                 if (job.reimport_editable_state != null)
                     job.photo.finish_update_editable_metadata (job.reimport_editable_state);
             } catch (DatabaseError err) {
-                AppWindow.database_error (err);
+                AppWindow.database_error (err); // Causes panic
             } finally {
-                // this assertion guards against reentrancy
-                assert (ignore_photo_alteration == job.photo);
                 ignore_photo_alteration = null;
             }
         } else {
@@ -677,7 +681,9 @@ public class MetadataWriter : Object {
 
     private void on_update_cancelled (BackgroundJob j) {
         bool removed = pending.unset (((CommitJob) j).photo);
-        assert (removed);
+        if (!removed) {
+            critical ("Failed to remove photo from pending on update cancelled");
+        }
 
         count_cancelled_work (1, true);
     }

src/Photo.vala

@@ -248,7 +248,7 @@ public abstract class Photo : PhotoSource, Dateable {
     private PixelTransformer transformer = null;
     private PixelTransformationBundle adjustments = null;
     // because file_title is determined by data in row, it should only be accessed when row is locked
-    private string file_title = null;
+    private string file_title = "";
     private FileMonitor editable_monitor = null;
     private OneShotScheduler reimport_editable_scheduler = null;
     private OneShotScheduler update_editable_attributes_scheduler = null;
@@ -1336,7 +1336,7 @@ public abstract class Photo : PhotoSource, Dateable {
 
     // This method is thread-safe.  If returns false the photo should be marked offline (in the
     // main UI thread).
-    public bool prepare_for_reimport_master (out ReimportMasterState reimport_state) throws Error {
+    public bool prepare_for_reimport_master (out ReimportMasterState? reimport_state) throws Error {
         reimport_state = null;
 
         File file = get_master_reader ().get_file ();
@@ -1418,7 +1418,12 @@ public abstract class Photo : PhotoSource, Dateable {
     protected abstract void apply_user_metadata_for_reimport (PhotoMetadata metadata);
 
     // This method is not thread-safe and should be called in the main thread.
-    public void finish_reimport_master (ReimportMasterState state) throws DatabaseError {
+    public void finish_reimport_master (ReimportMasterState? state) throws DatabaseError {
+        if (state == null) {
+            warning ("finish_reimport_state called with null state - ignoring");
+            return;
+        }
+
         ReimportMasterStateImpl reimport_state = (ReimportMasterStateImpl) state;
 
         PhotoTable.get_instance ().reimport (reimport_state.row);
@@ -1460,9 +1465,10 @@ public abstract class Photo : PhotoSource, Dateable {
         }
     }
 
-    // Verifies a file for reimport.  Returns the file's detected photo info.
+    // Verifies a (non null) file for reimport.  Returns the file's detected photo info.
     private bool verify_file_for_reimport (File file, out BackingPhotoRow backing,
                                            out DetectedPhotoInformation detected) throws Error {
+
         backing = query_backing_photo_row (file, PhotoFileSniffer.Options.NO_MD5,
         out detected);
         if (backing == null) {
@@ -1504,7 +1510,7 @@ public abstract class Photo : PhotoSource, Dateable {
     // This method is not thread-safe.  It should be called by the main thread.
     public void finish_reimport_editable (ReimportEditableState state) throws DatabaseError {
         BackingPhotoID editable_id = get_editable_id ();
-        if (editable_id.is_invalid ()) {
+        if (state == null || editable_id.is_invalid ()) {
             return;
         }
 
@@ -2692,7 +2698,19 @@ public abstract class Photo : PhotoSource, Dateable {
             return false;
         }
 
-        master_reader.create_metadata_writer ().write_metadata (metadata);
+        File? master_file = get_master_file ();
+        LibraryMonitor.blacklist_file (master_file, "MetadataWriter.commit_master");
+        try {
+            master_reader.create_metadata_writer ().write_metadata (metadata);
+        } catch (Error e) {
+            warning (
+                "Error persisting master metadata %s. %s",
+                master_file != null ? master_file.get_uri () : "null",
+                e.message
+            );
+        } finally {
+            LibraryMonitor.unblacklist_file (master_file);
+        }
 
         if (!prepare_for_reimport_master (out state)) {
             return false;
@@ -2707,6 +2725,7 @@ public abstract class Photo : PhotoSource, Dateable {
         finish_reimport_master (state);
     }
 
+    // Only one caller - in MetadataWriter  TODO move there
     public bool persist_editable_metadata (PhotoMetadata metadata, out ReimportEditableState state)
     throws Error {
         state = null;
@@ -2720,7 +2739,20 @@ public abstract class Photo : PhotoSource, Dateable {
             return false;
         }
 
-        editable_reader.create_metadata_writer ().write_metadata (metadata);
+        File? editable_file = get_editable_file ();
+        LibraryMonitor.blacklist_file (editable_file, "MetadataWriter.commit_editable");
+        try {
+            editable_reader.create_metadata_writer ().write_metadata (metadata);
+        } catch (Error e) {
+            warning (
+                "Error writing metadata to %s. %s",
+                editable_file != null ? editable_file.get_uri () : "null",
+                e.message
+            );
+            throw e;
+        } finally {
+            LibraryMonitor.unblacklist_file (editable_file);
+        }
 
         if (!prepare_for_reimport_editable (out state)) {
             return false;
@@ -3609,7 +3641,7 @@ public abstract class Photo : PhotoSource, Dateable {
         // Build a destination file with the caller's name but the appropriate extension
         File dest_file = format_properties.convert_file_extension (file);
 
-        // Create a PhotoFileMetadataWriter that matches the PhotoFileReader's file format
+        // Create a PhotoFileMetadataWriter that matches the PhotoFileReader's file format (may throw error)
         PhotoFileMetadataWriter writer = export_reader.get_file_format ().create_metadata_writer (
                                              dest_file.get_path ());
 

src/photos/PhotoFileFormat.vala

@@ -112,6 +112,7 @@ public enum PhotoFileFormat {
             return UNKNOWN;
 
         foreach (PhotoFileFormat file_format in get_supported ()) {
+            // Supported formats must have driver with properties
             if (file_format.get_driver ().get_properties ().is_recognized_extension (ext))
                 return file_format;
         }
@@ -399,7 +400,7 @@ public abstract class PhotoFileFormatDriver {
 
     public abstract PhotoFileWriter? create_writer (string filepath);
 
-    public abstract PhotoFileMetadataWriter? create_metadata_writer (string filepath);
+    public abstract PhotoFileMetadataWriter? create_metadata_writer (string filepath);  // Note can be null
 
     public abstract PhotoFileSniffer create_sniffer (File file, PhotoFileSniffer.Options options);
 }

src/photos/PhotoMetadata.vala

@@ -41,6 +41,11 @@ public enum MetadataDomain {
     IPTC
 }
 
+errordomain PhotoMetadataError {
+    MISSING_DATA,
+    UNKNOWN_ERROR
+}
+
 public class HierarchicalKeywordField {
     public string field_name;
     public string path_separator;