After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 763156 - Discard all edits functionality
Discard all edits functionality
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on: 764801
Blocks:
 
 
Reported: 2016-03-05 23:55 UTC by Rafael Fonseca
Modified: 2016-08-09 16:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add revert to original API (5.09 KB, patch)
2016-04-04 12:36 UTC, Rafael Fonseca
none Details | Review
properties-dialog: Add option to revert edits (6.22 KB, patch)
2016-04-04 12:38 UTC, Rafael Fonseca
none Details | Review
add revert to original API (5.18 KB, patch)
2016-04-15 18:21 UTC, Rafael Fonseca
none Details | Review
properties-dialog: add option to revert edits (6.22 KB, patch)
2016-04-15 18:22 UTC, Rafael Fonseca
none Details | Review
properties-dialog: merge width and height into dimensions. (4.11 KB, patch)
2016-04-15 18:26 UTC, Rafael Fonseca
committed Details | Review
properties-dialog: add option to revert all edits (5.75 KB, patch)
2016-04-15 18:28 UTC, Rafael Fonseca
none Details | Review
properties-dialog: Merge width and height into dimensions (4.05 KB, patch)
2016-08-04 12:47 UTC, Debarshi Ray
committed Details | Review
base-item, pipeline: Add is_edited and revert_to_original API (9.24 KB, patch)
2016-08-09 16:44 UTC, Debarshi Ray
committed Details | Review
application, properties-dialog: Add UI to revert all edits (10.83 KB, patch)
2016-08-09 16:45 UTC, Debarshi Ray
committed Details | Review

Description Rafael Fonseca 2016-03-05 23:55:55 UTC
We need a way to discard all the edits done through gnome-photos, thus reverting the photo to its original state.

This is the mockup provided for it https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/photos/wires-revert-to-original.png
Comment 1 Rafael Fonseca 2016-04-01 15:10:56 UTC
14:02 <  rfonseca> aday: regarding gphotos, issue #763156, what should happen after "Discard all edits" is clicked? Change the "Edited in Photos" string and hide the button? Remove the whole line?
16:25 <    jimmac> rfonseca: it should say something like "Untouched" or "Unmodified"
17:03 <  rfonseca> jimmac: what about the button?
17:05 <    jimmac> i was pondering an undo, but i'd probably just remove the button
Comment 2 Rafael Fonseca 2016-04-04 12:36:49 UTC
Created attachment 325329 [details] [review]
Add revert to original API
Comment 3 Rafael Fonseca 2016-04-04 12:38:03 UTC
Created attachment 325330 [details] [review]
properties-dialog: Add option to revert edits

For some reason, gtk_widget_hide is not working on the revert button.
Comment 4 Rafael Fonseca 2016-04-15 18:21:42 UTC
Created attachment 326127 [details] [review]
add revert to original API

Accounting for possible changes introduced by #764801.
Comment 5 Rafael Fonseca 2016-04-15 18:22:57 UTC
Created attachment 326128 [details] [review]
properties-dialog: add option to revert edits

In this new version we only disable the "Discard all edits" button instead of hiding it.
Comment 6 Rafael Fonseca 2016-04-15 18:26:14 UTC
Created attachment 326129 [details] [review]
properties-dialog: merge width and height into dimensions.
Comment 7 Rafael Fonseca 2016-04-15 18:28:20 UTC
Created attachment 326130 [details] [review]
properties-dialog: add option to revert all edits

Ops, this is the correct version.
Comment 8 Debarshi Ray 2016-08-04 10:17:38 UTC
Review of attachment 326127 [details] [review]:

Thanks, Rafael. Looks good, except:

::: src/photos-base-item.c
@@ +2485,3 @@
+photos_base_item_revert_to_original (PhotosBaseItem *self)
+{
+  photos_pipeline_revert_to_original (self->priv->pipeline);

We should implicitly call photos_base_item_process_async, as we did in bug 765105
Comment 9 Debarshi Ray 2016-08-04 10:26:39 UTC
Review of attachment 326127 [details] [review]:

::: src/photos-base-item.c
@@ +2005,3 @@
+photos_base_item_get_edited (PhotosBaseItem *self)
+{
+  return photos_pipeline_get_edited (self->priv->pipeline);

The pipeline can be NULL if the item hasn't been loaded, yet. Since we don't need the pixel data for this, we can try to only create the pipeline.
Comment 10 Debarshi Ray 2016-08-04 12:45:15 UTC
Review of attachment 326129 [details] [review]:

Looks fine, except one small detail:

::: src/photos-properties-dialog.c
@@ +502,3 @@
+      gchar *dims_str;
+
+      dims_str = g_strdup_printf ("%"G_GINT64_FORMAT" x %"G_GINT64_FORMAT" pixels", width, height);

Nitpick: would be nice to use the Unicode U+00D7 code point (ie. ×) instead of the 'X' character. See https://wiki.gnome.org/Design/OS/Typography
Comment 11 Debarshi Ray 2016-08-04 12:47:36 UTC
Created attachment 332729 [details] [review]
properties-dialog: Merge width and height into dimensions

Fixed and pushed.
Comment 12 Debarshi Ray 2016-08-09 16:37:27 UTC
Review of attachment 326130 [details] [review]:

::: src/photos-properties-dialog.c
@@ +222,3 @@
+
+  app = g_application_get_default ();
+  g_application_hold (app);

Even if we are holding the GApplication, running the async operations with 'self->cancellable' means that they will be cancelled if the dialog is closed before they are complete. Cancelling the process_async isn't so bad, but cancelling the pipeline_save_async means that the changes might not be saved to disk. That can be a bit surprising.

Therefore, the least we should do is not to pass a GCancellable.

However, not using a GCancellable means we can't rely on PropertiesDialog (ie. self) staying alive across the calls. It might be nicer to move the async code to photos-application.c in that case. The dialog can emit a signal to indicate when the button was clicked.
Comment 13 Debarshi Ray 2016-08-09 16:44:22 UTC
Created attachment 333004 [details] [review]
base-item, pipeline: Add is_edited and revert_to_original API
Comment 14 Debarshi Ray 2016-08-09 16:45:01 UTC
Created attachment 333006 [details] [review]
application, properties-dialog: Add UI to revert all edits
Comment 15 Debarshi Ray 2016-08-09 16:46:02 UTC
Fixed and pushed to master. Thanks for the patches, Rafael.