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 762046 - edit: Cancel should revert to the previous set of edits
edit: Cancel should revert to the previous set of edits
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
3.19.x
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-02-14 20:37 UTC by Debarshi Ray
Modified: 2016-02-23 19:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
photos-pipeline: Move the logic of creating graph using xml to separate function (4.25 KB, patch)
2016-02-18 20:35 UTC, Umang Jain
none Details | Review
photos-pipeline: Move the logic of creating graph using xml to separate function (4.25 KB, patch)
2016-02-19 04:30 UTC, Umang Jain
none Details | Review
photos-pipeline: Attempt to recreate the graph using snapshot but failing (3.10 KB, patch)
2016-02-19 09:27 UTC, Umang Jain
none Details | Review
photos-pipeline: Move the logic of creating graph using xml to separate function (5.43 KB, patch)
2016-02-20 07:31 UTC, Umang Jain
committed Details | Review
photos-pipeline: Attempt to recreate the graph using snapshot but failing (2.35 KB, patch)
2016-02-20 11:08 UTC, Umang Jain
none Details | Review
pipeline: Refactor the XML deserialization into a separate function (5.31 KB, patch)
2016-02-22 11:42 UTC, Debarshi Ray
committed Details | Review
pipeline: Attempt to recreate the graph but failing (4.26 KB, patch)
2016-02-23 11:20 UTC, Umang Jain
needs-work Details | Review
pipeline, utils: Refactor removal of children into a separate function (3.46 KB, patch)
2016-02-23 14:18 UTC, Debarshi Ray
committed Details | Review
pipeline: Don't destroy the graph and its proxies when deserializing (1.44 KB, patch)
2016-02-23 14:19 UTC, Debarshi Ray
committed Details | Review
pipeline: Recreate the pipeline snapshot's graph when edit is cancelled (3.95 KB, patch)
2016-02-23 16:30 UTC, Umang Jain
none Details | Review
pipeline: Recreate the pipeline snapshot's graph when edit is cancelled (4.02 KB, patch)
2016-02-23 16:41 UTC, Umang Jain
committed Details | Review
Cancel should revert to the state prior to entering edit mode (4.85 KB, patch)
2016-02-23 19:56 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2016-02-14 20:37:05 UTC
Right now pressing "Cancel" in the edit mode blows away all the edits. Instead it should revert to the previous editing session.
Comment 1 Umang Jain 2016-02-18 20:35:26 UTC
Created attachment 321616 [details] [review]
photos-pipeline: Move the logic of creating graph using xml to separate function

We are planning to destroy the current graph and re-create
a new one from the snapshot of the pipeline. This will help
us to do that without introducing redunant code.
Comment 2 Umang Jain 2016-02-19 04:30:12 UTC
Created attachment 321631 [details] [review]
photos-pipeline: Move the logic of creating graph using xml to separate function

We are planning to destroy the current graph and re-create
a new one from the snapshot of the pipeline. This will help
us to do that without introducing redunant code.
Comment 3 Umang Jain 2016-02-19 09:27:39 UTC
Created attachment 321640 [details] [review]
photos-pipeline: Attempt to recreate the graph using snapshot but failing
Comment 4 Rafael Fonseca 2016-02-19 16:40:51 UTC
Review of attachment 321640 [details] [review]:

::: src/photos-pipeline.c
@@ +293,3 @@
+{
+  self->snapshot = gegl_node_to_xml (self->graph, "/");
+

self->snapshot is leaking. If photos_pipeline_snapshot is called more than once for the same pipeline, the previous content needs to be freed.

@@ +572,2 @@
 photos_pipeline_undo (PhotosPipeline *self)
 {

You're leaking the whole current gegl graph and the operations saved in the hash. More than that, you are also keeping invalid operations in the hash for when the previous graph is restored.

@@ +578,1 @@
   return ret_val;

Why not just
    return photos_pipeline_...
Comment 5 Debarshi Ray 2016-02-19 19:01:43 UTC
Review of attachment 321631 [details] [review]:

One quick comment:

::: src/photos-pipeline.c
@@ +321,3 @@
+   * Therefore, we are going to re-construct a proper graph
+   * ourselves.
+   */

It will be better to keep this comment close to the gegl_node_new_from_xml call because that is the buggy code we are working around.
Comment 6 Debarshi Ray 2016-02-19 19:07:49 UTC
Review of attachment 321640 [details] [review]:

Thanks for the patch, Umang. Can you try to rebase it on top of master? It looks as if it conflicts with the changes from bug 761683

::: src/photos-pipeline.c
@@ +571,2 @@
 gboolean
 photos_pipeline_undo (PhotosPipeline *self)

Also, there is no photos_pipeline_undo anymore. We have photos_pipeline_revert now. See bug 761683
Comment 7 Umang Jain 2016-02-20 07:31:01 UTC
Created attachment 321707 [details] [review]
photos-pipeline: Move the logic of creating graph using xml to separate function

We are planning to destroy the current graph and
re-create a new one from the snapshot of the pipeline.
This will help us to do that without introducing
redunant code.
Comment 8 Umang Jain 2016-02-20 11:08:13 UTC
Created attachment 321717 [details] [review]
photos-pipeline: Attempt to recreate the graph using snapshot but failing
Comment 9 Debarshi Ray 2016-02-21 19:29:16 UTC
Review of attachment 321717 [details] [review]:

I haven't tried it out, yet.

::: src/photos-pipeline.c
@@ +38,3 @@
   GHashTable *hash;
   GeglNode *graph;
+  gchar *snapshot;

It needs to be g_free-ed during destruction (ie. in finalize).

@@ +294,2 @@
 static void
+photos_pipeline_set_snapshot (PhotosPipeline *self)

Nitpick: we are not setting a variable, but taking a snapshot. So photos_pipeline_snapshot sounds slightly better.
Comment 10 Debarshi Ray 2016-02-21 19:41:34 UTC
Review of attachment 321717 [details] [review]:

::: src/photos-pipeline.c
@@ +294,2 @@
 static void
+photos_pipeline_set_snapshot (PhotosPipeline *self)

More importantly, photos_pipeline_snapshot needs to be public, and it needs a corresponding photos_base_item_pipeline_snapshot wrapper. See the revert methods that Rafael added for an example.

@@ +335,3 @@
     }
 
+  photos_pipeline_set_snapshot (self);

The idea is to create a new snapshot each we enter the EDIT mode. The pipeline is created when we enter PREVIEW, or when we export from the selection mode. Therefore, this needs to happen in photos_application_edit_current, which is the implementation of the 'edit-current' GAction.
Comment 11 Debarshi Ray 2016-02-22 11:40:23 UTC
Review of attachment 321707 [details] [review]:

The patch looks perfect apart from a typo:

::: src/photos-pipeline.c
@@ +105,3 @@
+    {
+      ret_val = FALSE;
+      goto out;

It will slightly more idiomatic to initialize it to FALSE at the top.

@@ +123,3 @@
+      gegl_node_link (input, output);
+      return TRUE;
+      goto out;

Typo? :)
Comment 12 Debarshi Ray 2016-02-22 11:42:09 UTC
Created attachment 321823 [details] [review]
pipeline: Refactor the XML deserialization into a separate function

Fixed the typo and adjusted the commit message to not exceed 72 characters per line.
Comment 13 Umang Jain 2016-02-23 11:20:19 UTC
Created attachment 321940 [details] [review]
pipeline: Attempt to recreate the graph but failing

https://bugzilla.gnome.org/show_bug.cgi?id=762046
Comment 14 Debarshi Ray 2016-02-23 11:41:56 UTC
Review of attachment 321940 [details] [review]:

::: src/photos-pipeline.c
@@ +598,3 @@
 photos_pipeline_revert (PhotosPipeline *self)
 {
+  if (self->snapshot)

I would use:
  g_return_if_fail (self->snapshot != NULL);

... because it would be a programmer error to call revert without a snapshot. It simply should not happen.

@@ -594,3 @@
-
-  input = gegl_node_get_input_proxy (self->graph, "input");
-  output = gegl_node_get_output_proxy (self->graph, "output");

It is not going to be enough to just re-create the graph. When you do that, the old graph is destroyed. That means the input is disconnected from its producer and the output is disconnected from its consumers. So, once you have the new graph, you need to get its proxies (they are different from the old proxies) and connect them up.

@@ +601,2 @@
     {
+      g_hash_table_remove_all (self->hash);

I would suggest moving the g_hash_table_remove_all to create_graph_from_xml - we are destroying and recreating a new graph there, and also repopulating the hash. So, it makes sense to clear the hash table there too.
Comment 15 Debarshi Ray 2016-02-23 14:18:58 UTC
Created attachment 321970 [details] [review]
pipeline, utils: Refactor removal of children into a separate function
Comment 16 Debarshi Ray 2016-02-23 14:19:25 UTC
Created attachment 321971 [details] [review]
pipeline: Don't destroy the graph and its proxies when deserializing
Comment 17 Umang Jain 2016-02-23 16:30:24 UTC
Created attachment 322003 [details] [review]
pipeline: Recreate the pipeline snapshot's graph when edit is cancelled
Comment 18 Umang Jain 2016-02-23 16:41:00 UTC
Created attachment 322005 [details] [review]
pipeline: Recreate the pipeline snapshot's graph when edit is cancelled
Comment 19 Debarshi Ray 2016-02-23 19:55:23 UTC
Review of attachment 322005 [details] [review]:

Looking good, Umang.

::: src/photos-pipeline.c
@@ +39,3 @@
   GHashTable *hash;
   GeglNode *graph;
+  gchar *snapshot;

It is not being g_free-ed during destruction.

@@ +590,1 @@
+  photos_pipeline_create_graph_from_xml (self, self->snapshot);

Would be good to check the return value. Just in case, you know, there was a bug in the serialization/deserialization we failed to create the graph.

Also, we need to g_free self->snapshot and set it to NULL so that our assertion at the beginning of the function can work. ie., you can only revert once a snapshot has been defined, and you can not revert twice to the same snapshot.

@@ +601,3 @@
+{
+  g_free (self->snapshot);
+  self->snapshot = gegl_node_to_xml_full (self->graph, self->graph, "/");

Would be nice to add some debug here.
Comment 20 Debarshi Ray 2016-02-23 19:56:11 UTC
Created attachment 322176 [details] [review]
Cancel should revert to the state prior to entering edit mode

Fixed and pushed.
Comment 21 Debarshi Ray 2016-02-23 19:56:32 UTC
Thanks for the patches!