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.

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.

Created attachment 321640 [details] [review]
photos-pipeline: Attempt to recreate the graph using snapshot but failing

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_...

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.

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

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.

Created attachment 321717 [details] [review]
photos-pipeline: Attempt to recreate the graph using snapshot but failing

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.

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.

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? :)

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.

Created attachment 321940 [details] [review]
pipeline: Attempt to recreate the graph but failing

https://bugzilla.gnome.org/show_bug.cgi?id=762046

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.

Created attachment 321970 [details] [review]
pipeline, utils: Refactor removal of children into a separate function

Created attachment 321971 [details] [review]
pipeline: Don't destroy the graph and its proxies when deserializing

Created attachment 322003 [details] [review]
pipeline: Recreate the pipeline snapshot's graph when edit is cancelled

Created attachment 322005 [details] [review]
pipeline: Recreate the pipeline snapshot's graph when edit is cancelled

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.

Created attachment 322176 [details] [review]
Cancel should revert to the state prior to entering edit mode

Fixed and pushed.

Thanks for the patches!