GNOME Bugzilla – Bug 785051
Unedited photo are shown as edited
Last modified: 2018-01-23 10:13:32 UTC
How to reproduce: - select a photo - click the edit button - click "done" button (without actually editing the photo in any way) expected result: nothing happens and you get back to the original photo current result: you return to the original photo but get a "name-of-the-photo edited [undo]" pop up
That's a good point!
My suggestion would be disable the "done" button unless the item was actually edited. This should also take into consideration cases where some tools were temporarily used to do some edits, but their effects were undone afterwards.
Summarising the discussion in #photos on GIMPNet: I think the way forward is to add a "changed" signal to PhotosPipeline and enable/disable the GAction driving the "done" button depending on whether the pipeline is edited or not.
Created attachment 361732 [details] [review] Disables "done" button when photo is unedited.
Review of attachment 361732 [details] [review]: Thanks for the patch, Nidhi. I haven't tested it thoroughly, but some quick comments below: ::: src/photos-pipeline.c @@ +56,3 @@ +}; + +static guint signals[LAST_SIGNAL] = { 0 }; Nitpick: there should be a newline after this. @@ +222,3 @@ output = gegl_node_get_output_proxy (self->graph, "output"); gegl_node_link (input, output); + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); We shouldn't emit a signal here. The object is still getting constructed. ie., we are still inside photos_pipeline_new_*. So it is too early for any users of the Pipeline instance to connect to the signal. @@ +324,3 @@ + + signals[PIPELINE_CHANGED] = g_signal_new ("changed", + G_TYPE_FROM_CLASS (class), Nitpick: the alignment is off. @@ +688,3 @@ g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); We shouldn't emit a signal here. This merely saves the current state of the pipeline in a file by serializing it into XML. There is no change in its output. However, we are missing a signal emission in photos_pipeline_remove. @@ +739,3 @@ xml = gegl_node_to_xml_full (self->graph, self->graph, "/"); photos_debug (PHOTOS_DEBUG_GEGL, "Pipeline: %s", xml); + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); There is no guarantee that reverting will restore the pipeline to its unedited state. This will revert to the previous snapshot. ie., the state of the pipeline before we entered the EDIT mode. So, this won't work if someone is editing the same BaseItem twice. I think that we need to use photos_pipeline_is_edited to get the signal's parameter. @@ +781,3 @@ if (parent != NULL) gegl_node_add_child (parent, self->graph); + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, TRUE); We shouldn't emit a signal here. This merely changes the parent GeglNode of the pipeline's graph. It doesn't change the contents of the graph.
(In reply to Debarshi Ray from comment #5) > Review of attachment 361732 [details] [review] [review]: > > Thanks for the patch, Nidhi. I haven't tested it thoroughly, but some quick > comments below: > > ::: src/photos-pipeline.c > @@ +688,3 @@ > g_return_val_if_fail (error == NULL || *error == NULL, FALSE); > > + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); > > We shouldn't emit a signal here. This merely saves the current state of the > pipeline in a file by serializing it into XML. There is no change in its > output. Signal with false parameter is required here because when we save the edited photo, next time on opening edit panel done button should be disabled. I tested this case. > > However, we are missing a signal emission in photos_pipeline_remove. > In photos_pipeline_remove, we are removing one operation at a time but that doesn't ensure that overall photo is unedited. I think this will come into the picture when we take care of cases where some tools were temporarily used to do some edits, but their effects were undone afterwards. Correct me if this is wrong. > @@ +739,3 @@ > xml = gegl_node_to_xml_full (self->graph, self->graph, "/"); > photos_debug (PHOTOS_DEBUG_GEGL, "Pipeline: %s", xml); > + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); > > There is no guarantee that reverting will restore the pipeline to its > unedited state. This will revert to the previous snapshot. ie., the state of > the pipeline before we entered the EDIT mode. So, this won't work if someone > is editing the same BaseItem twice. This is called when we click cancel button, done button should be disabled so that when we enter edit section again,we find it disabled. I tested this case.
(In reply to Nidhi Gupta from comment #6) > (In reply to Debarshi Ray from comment #5) > > ::: src/photos-pipeline.c > > > @@ +688,3 @@ > > g_return_val_if_fail (error == NULL || *error == NULL, FALSE); > > > > + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); > > > > We shouldn't emit a signal here. This merely saves the current state of the > > pipeline in a file by serializing it into XML. There is no change in its > > output. > > Signal with false parameter is required here because when we save the edited > photo, next time on opening edit panel done button should be disabled. > I tested this case. That needs to be handled somewhere else. Possibly something more higher-level that Pipeline. The Pipeline::changed signal should have a clear meaning - it's emitted when the graph changes after the initial construction has happened, and the boolean parameter indicates whether there are any edits or not. > > However, we are missing a signal emission in photos_pipeline_remove. > > > > In photos_pipeline_remove, we are removing one operation at a time but that > doesn't ensure that overall photo is unedited. You can use photos_pipeline_is_edited to determine what value to pass with the signal. > > @@ +739,3 @@ > > xml = gegl_node_to_xml_full (self->graph, self->graph, "/"); > > photos_debug (PHOTOS_DEBUG_GEGL, "Pipeline: %s", xml); > > + g_signal_emit (self, signals[PIPELINE_CHANGED], 0, FALSE); > > > > There is no guarantee that reverting will restore the pipeline to its > > unedited state. This will revert to the previous snapshot. ie., the state of > > the pipeline before we entered the EDIT mode. So, this won't work if someone > > is editing the same BaseItem twice. > > This is called when we click cancel button, done button should be disabled > so that when we enter edit section again,we find it disabled. I tested this > case. That should be handled somewhere else. The patch might be working (I haven't tried it) in its current form, but in future the code will become confusing and unmanageable if things (especially interfaces) don't have clear semantics. That's why Pipeline::changed needs to have a clear meaning. See above.
Hi Nidhi, Can you re-work the patch according to the review comments given and try to get this merged? Thanks!
Hi Umang, I am stuck at the review comment "That needs to be handled somewhere else. Possibly something more higher-level that Pipeline.". I am not clear how to approach this bug correctly. Thanks for your time.
(In reply to Nidhi Gupta from comment #9) > Hi Umang, > I am stuck at the review comment "That needs to be handled somewhere else. > Possibly something more higher-level that Pipeline.". > I am not clear how to approach this bug correctly. > Thanks for your time. Hi Nidhi. Looks like that something can be discussed at IRC maybe? What do you think? :)
By "higher-level than Pipeline" I meant something that's conceptually more abstract than Pipeline in the application's architecture. The most abstract entity that we have is Application. Instantiating it, and invoking its "run" method is all that you need to start the application. Inside Application there is MainWindow, the various singletons (like ItemManager, SourceManager, etc.), and so on. MainWindow contains a hierarchy of more specialized widgets; the ItemManager contains all the BaseItems, and each of those can have a Pipeline. So, Pipeline is a few layers below the UI widgets, which is why things need to be wired through the different layers.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-photos/issues/68.