Merge lp:~evfool/pantheon-photos/lp1341812 into lp:~pantheon-photos/pantheon-photos/trunk

[Robert Roth]

Only delete files when emptying the Trash, in other cases (e.g. Remove from library) do not delete the files to avoid data loss.

Only delete files when emptying the Trash, in other cases (e.g. Remove from library) do not delete the files to avoid data loss.

[Corentin Noël (tintou)]

It's nice, I've made some comments to improve Photos

[xapantu (xapantu)]

After some discussion:
1. Put to trash should put the file into the real system trash
2. The trash view should show the images from the system trash (and maybe even only the ones that were in ~/Image before their deletion).

review: Needs Fixing

[Tom Beckmann (tombeckmann)]

It looks like the original idea for trashing in shotwell was very non-destructive. They don't even move an original file after it was trashed to some special place, but only flag the file as deleted in their database. So apparently they were aiming to keep the original file structure intact and limiting themselves to changes on their database. I haven't read much of the code yet, but my impression is that there's a vast number of different subclasses handling trashing differently or relying on some state of the files. My guess would be that it's relatively difficult to rework this structure, just because of the number of places where you have to make sure that the file is not accessed at its original spot, but I don't know if evfool already made progress on that? Maybe I missed something obvious. Otherwise I'd suggest to keep shortwell's non-destructive trashing system and go with the changes proposed by evfool so far.

[Robert Roth (evfool)]

@Tom: no, I haven't had the time to look into this yet, so I don't know how complicated it would be. Theoretically yes, several subclasses are overriding the same superclass method for "trashing files", but as far as I saw this so far, it shouldn't be that complicated to implement what xapantu suggests (to only show the images from trash that have been managed by photos exactly because they have an internal reference in -photos' database)

I will take a look at this and will check back with the results sometime this week.

[Robert Roth (evfool)]

This seems to be harder than I first thought: as there seems to be no way to find out the URL of a file in the trash (after trashing), thus restoring is not trivial. Will look into other ways (e.g. iterating through the files in the trash to see which ones appear in Photos' database)

[Jacob Parker (jacobparker1992)]

Is it fixing this bug?
https://bugs.launchpad.net/pantheon-photos/+bug/1345635

[Danielle Foré (danrabbit)]

It sounds like what Lucas is referring to is outside the scope of this particular merge. A separate merge should be made that addresses the bug that Jacob linked. This merge should only deal with correcting the data loss associated with the current "Remove from Library" menu item.

[Tom Beckmann (tombeckmann)]

Code looks great!!

review: Approve (code (not actually tested))

[David Gomes (davidgomes)]

Works.

review: Approve

[RabbitBot (rabbitbot-a)]

Attempt to merge into lp:pantheon-photos failed due to conflicts:

text conflict in src/Dialogs.vala
text conflict in src/PhotoPage.vala
text conflict in src/library/LibraryWindow.vala
text conflict in src/library/TrashPage.vala

[lp:~evfool/pantheon-photos/lp1341812]

2553. By Robert Roth on 2014-08-09

Merged from trunk

2554. By Robert Roth on 2014-08-09

Coding style fixes for styling broken at merge

[Robert Roth (evfool)]

Merged from trunk, reapproving.

=== modified file 'src/Dialogs.vala'
--- src/Dialogs.vala	2014-08-08 21:13:09 +0000
+++ src/Dialogs.vala	2014-08-09 14:07:50 +0000
@@ -2658,11 +2658,11 @@
 
 public void remove_photos_from_library (Gee.Collection<LibraryPhoto> photos) {
     remove_from_app (photos, _ ("Remove From Library"),
-                     (photos.size == 1) ? _ ("Removing Photo From Library") : _ ("Removing Photos From Library"));
+                     (photos.size == 1) ? _ ("Removing Photo From Library") : _ ("Removing Photos From Library"), false);
 }
 
 public void remove_from_app (Gee.Collection<MediaSource> sources, string dialog_title,
-                             string progress_dialog_text) {
+                             string progress_dialog_text, bool delete_files) {
     if (sources.size == 0)
         return;
 
@@ -2680,23 +2680,25 @@
     }
 
     // Remove and attempt to trash.
-    LibraryPhoto.global.remove_from_app (photos, true, monitor, null);
-    Video.global.remove_from_app (videos, true, monitor, null);
-
-    // Attempt to delete the files.
-    Gee.ArrayList<LibraryPhoto> not_deleted_photos = new Gee.ArrayList<LibraryPhoto> ();
-    Gee.ArrayList<Video> not_deleted_videos = new Gee.ArrayList<Video> ();
-    LibraryPhoto.global.delete_backing_files (photos, monitor, not_deleted_photos);
-    Video.global.delete_backing_files (videos, monitor, not_deleted_videos);
-
-    int num_not_deleted = not_deleted_photos.size + not_deleted_videos.size;
-    if (num_not_deleted > 0) {
-        // Alert the user that the files were not removed.
-        string delete_failed_message =
-            ngettext ("The photo or video cannot be deleted.",
-                      "%d photos/videos cannot be deleted.",
-                      num_not_deleted).printf (num_not_deleted);
-        AppWindow.error_message_with_title (dialog_title, delete_failed_message, AppWindow.get_instance ());
+    LibraryPhoto.global.remove_from_app (photos, delete_files, monitor, null);
+    Video.global.remove_from_app (videos, delete_files, monitor, null);
+
+    if (delete_files) {
+        // Attempt to delete the files.
+        Gee.ArrayList<LibraryPhoto> not_deleted_photos = new Gee.ArrayList<LibraryPhoto> ();
+        Gee.ArrayList<Video> not_deleted_videos = new Gee.ArrayList<Video> ();
+        LibraryPhoto.global.delete_backing_files (photos, monitor, not_deleted_photos);
+        Video.global.delete_backing_files (videos, monitor, not_deleted_videos);
+
+        int num_not_deleted = not_deleted_photos.size + not_deleted_videos.size;
+        if (num_not_deleted > 0) {
+            // Alert the user that the files were not removed.
+            string delete_failed_message = 
+                ngettext ("The photo or video cannot be deleted.",
+                          "%d photos/videos cannot be deleted.",
+                          num_not_deleted).printf (num_not_deleted);
+            AppWindow.error_message_with_title (dialog_title, delete_failed_message, AppWindow.get_instance ());
+        }
     }
 
     if (progress != null)

=== modified file 'src/PhotoPage.vala'
--- src/PhotoPage.vala	2014-08-08 21:13:09 +0000
+++ src/PhotoPage.vala	2014-08-09 14:07:50 +0000
@@ -3117,7 +3117,7 @@
         Gee.Collection<LibraryPhoto> photos = new Gee.ArrayList<LibraryPhoto> ();
         photos.add (photo);
 
-        remove_from_app (photos, _ ("Remove From Library"), _ ("Removing Photo From Library"));
+        remove_from_app (photos, _ ("Remove From Library"), _ ("Removing Photo From Library"), false);
     }
 
     private void on_move_to_trash () {

=== modified file 'src/library/LibraryWindow.vala'
--- src/library/LibraryWindow.vala	2014-08-08 21:13:09 +0000
+++ src/library/LibraryWindow.vala	2014-08-09 14:07:50 +0000
@@ -737,7 +737,7 @@
         to_remove.add_all (LibraryPhoto.global.get_trashcan_contents ());
         to_remove.add_all (Video.global.get_trashcan_contents ());
 
-        remove_from_app (to_remove, _ ("Empty Trash"),  _ ("Emptying Trash..."));
+        remove_from_app (to_remove, _ ("Empty Trash"),  _ ("Emptying Trash..."), true);
 
         AppWindow.get_command_manager ().reset ();
     }

=== modified file 'src/library/TrashPage.vala'
--- src/library/TrashPage.vala	2014-08-08 21:13:09 +0000
+++ src/library/TrashPage.vala	2014-08-09 14:07:50 +0000
@@ -113,7 +113,7 @@
 
     private void on_delete () {
         remove_from_app ((Gee.Collection<MediaSource>) get_view ().get_selected_sources (), _ ("Delete"),
-                         (get_view ().get_selected_count () == 1) ? ("Deleting a Photo") : _ ("Deleting Photos"));
+                         ngettext("Deleting a Photo", "Deleting Photos", get_view().get_selected_count()), true);
     }
 
     public override SearchViewFilter get_search_view_filter () {