GNOME Bugzilla – Bug 761683
tool-crop: Crop area goes out of bounds when crop tool is reactivated
Last modified: 2016-02-23 14:16:45 UTC
Steps to reproduce: * Crop a photo * Restart the app * Open the photo - You must see the cropped part only * Open the crop tool - you now must see, the full original image with crop area selected * Click "Done" / Click Reset (This will restore the original image i.e. without any crop) * Again activate Crop tool (You will see that the crop area is out of bounds)
This is because we temporarily disable the cropping for already cropped items by setting the bounds to the extents of the item. We never remove the gegl:crop on "reset". When the tool is re-activated, it mistakes this temporary gegl:crop as a real thing and stretches the crop area across the entire canvas. From a quick look, the easiest way would be to actually remove the gegl:crop when activating with a cropped item. There are multiple options - mark the node as passthrough, switch the operation with a gegl:nop, or completely remove it from the pipeline. Whatever we do, it should play well with serializing / deserializing the pipeline.
Created attachment 321415 [details] [review] Fix crop reset This is my attempt to fix the crop reset issue.
Created attachment 321416 [details] [review] Replace A3 and A4 for "A paper sizes" This patch removes the redundant A3 and A4 sizes, since both have the same aspect ratio.
Created attachment 321417 [details] [review] Add Letter as cropping size option Optional: add Letter as size cropping option. It might need designers' approval.
(In reply to Rafael Fonseca from comment #2) > Created attachment 321415 [details] [review] [review] > Fix crop reset > > This is my attempt to fix the crop reset issue. There is an error in this patch. I'll send a new version soon.
Review of attachment 321416 [details] [review]: Thanks, looks good to me. Pushed after adding the bug URL.
Review of attachment 321415 [details] [review]: Thanks for the patch, Rafael. It is in the right direction. Some comments: ::: src/photos-tool-crop.c @@ +958,3 @@ + NULL)) + { + photos_base_item_operation_undo (self->item); photos_base_item_operation_undo and photos_pipeline_undo are busted in their current form. The code was written while the UX was totally in flux, so it is one of those things that don't fit so well with our reality. The problem is that undo always removes the last node in the graph. So, if you have: a -> b -> c ... then undo will give you: a -> b However, every time we activate a tool, we don't add a new node to the end of the graph. If a node is already present in the graph, we just update its properties. See photos_pipeline_add - particularly the use of self->hash. Therefore, in this case, undo will only work if crop is the last node in the graph. That won't be the case, if you went from crop to colours and back to crop. I would suggest adding photos_base_item_operation_remove and photos_pipeline_remove, which are the opposite of add. Given a 'const gchar *operation', it would completely remove it from the graph, if found. Then you can replace the photos_base_item_operation_add call in photos_tool_crop_activate with photos_base_item_operation_remove. (We should nuke/simplify the undo code in subsequent patches/bugs)
Created attachment 321508 [details] [review] Remove pipeline history The idea for a pipeline history is not currently being used for anything and will probably be replaced by a snapshot in the future.
Created attachment 321509 [details] [review] Add pipeline revert method With the undo operation being replaced by snapshot, there is not much point in having it. Instead, a revert operation to remove all the operation nodes in the gegl graph is clearer and more useful.
Created attachment 321510 [details] [review] Add pipeline operation removal This method will be useful for reverting specific functions, like cropping.
Created attachment 321511 [details] [review] Fix crop resetting When resetting a cropping, nothing was actually done regarding the gegl:crop operation. Because of that, the crop area would go out of bounds when the user tried to crop the item again, making it impossible to crop and/or resize the area.
Review of attachment 321509 [details] [review]: Looks quite good. ::: src/photos-pipeline.c @@ +587,3 @@ + + gegl_node_get (node, "operation", &operation, NULL); + g_hash_table_remove (self->hash, operation); I think, the "operation" needs to be g_free-ed. However, you don't need to free the hash_table in the loop. Just g_hash_table_remove_all should be enough.
Review of attachment 321508 [details] [review]: Looking at the other patches, it appears as if you have everything to replace undo with revert. So, we can completely nuke undo, no?
(In reply to Debarshi Ray from comment #13) > Review of attachment 321508 [details] [review] [review]: > > Looking at the other patches, it appears as if you have everything to > replace undo with revert. So, we can completely nuke undo, no? It's still used by the preview-view.c. I'm not sure if revert is a direct replacement in this case.
(In reply to Debarshi Ray from comment #12) > Review of attachment 321509 [details] [review] [review]: > > Looks quite good. > > ::: src/photos-pipeline.c > @@ +587,3 @@ > + > + gegl_node_get (node, "operation", &operation, NULL); > + g_hash_table_remove (self->hash, operation); > > I think, the "operation" needs to be g_free-ed. However, you don't need to > free the hash_table in the loop. Just g_hash_table_remove_all should be > enough. I'm actually not sure. The API doesn't say anything other than it returns the location of the property. I'll have to look into the source code.
(In reply to Rafael Fonseca from comment #14) > (In reply to Debarshi Ray from comment #13) > > Review of attachment 321508 [details] [review] [review] [review]: > > > > Looking at the other patches, it appears as if you have everything to > > replace undo with revert. So, we can completely nuke undo, no? > > It's still used by the preview-view.c. I'm not sure if revert is a direct > replacement in this case. It isn't used by preview-view.c anymore. ;) Sorry about the confusion.
(In reply to Rafael Fonseca from comment #15) > (In reply to Debarshi Ray from comment #12) > > Review of attachment 321509 [details] [review] [review] [review]: > > > > Looks quite good. > > > > ::: src/photos-pipeline.c > > @@ +587,3 @@ > > + > > + gegl_node_get (node, "operation", &operation, NULL); > > + g_hash_table_remove (self->hash, operation); > > > > I think, the "operation" needs to be g_free-ed. However, you don't need to > > free the hash_table in the loop. Just g_hash_table_remove_all should be > > enough. > > I'm actually not sure. The API doesn't say anything other than it returns > the location of the property. I'll have to look into the source code. gegl_node_get is a wrapper around g_object_get that will not only let you access the properties of the GeglNode, but also the properties of the GeglOperation object inside the GeglNode. Generally, g_object_get returns a copy of the properties' value, but it, of course, depends on the implementation of the get_property vfunc of the class, and whether gegl_node_get has changed the semantics of g_object_get. I will be a bit surprised if it doesn't confirm to the usual behaviour, because I didn't notice anything unusual when I looked at it last time.
Created attachment 321585 [details] [review] Add pipeline revert method Fix raised issues.
Review of attachment 321585 [details] [review]: Thanks, Rafael. ::: src/photos-pipeline.c @@ +580,3 @@ + input = gegl_node_get_input_proxy (self->graph, "input"); + output = gegl_node_get_output_proxy (self->graph, "output"); + node = gegl_node_get_producer (output, "input", NULL); Nitpick: 'last' would be a slightly nicer name because for the sake of consistency. @@ +590,3 @@ + } + + gegl_node_link (input, output); Note that GEGL is stupid and doesn't check if input and output are already connected or not. So, currently, if the graph was already empty, it would disconnect input and output (invalidating all the internal caches as a result) and connect them again. I think it is a GEGL bug, but we don't end up fixing GEGL, we should skip this in our code. Not a blocker for this patch. Let's see how it goes. :)
Created attachment 321676 [details] [review] application, pipeline: Add API to revert a series of edits and use it Pushed after tweaking the variable name, and adding the bug URL to the commit message.
Review of attachment 321510 [details] [review]: Looks really good, apart from some cosmetics: ::: src/photos-pipeline.c @@ +457,2 @@ +gboolean +photos_pipeline_remove (PhotosPipeline *self, const gchar *operation) Nit pick: would be nice to preserve the rough alphabetical ordering of the functions. @@ +474,3 @@ + + g_hash_table_remove (self->hash, operation); + gegl_node_remove_child (self->graph, node); Would be nice to add some debug to display the state of the graph.
Review of attachment 321511 [details] [review]: ::: src/photos-tool-crop.c @@ +956,3 @@ { + if (self->item != NULL) + photos_base_item_operation_remove (self->item, "gegl:crop"); It would be easier to remove the operation in photos_tool_crop_activate itself instead of modifying it to cover the full dimensions of the image.
Created attachment 321679 [details] [review] base-item, pipeline: Add API to remove an operation Tweaked and pushed to master.
Created attachment 321680 [details] [review] tool-crop: Fix reset
Created attachment 321684 [details] [review] base-item, pipeline: Remove all remaining undo code
Thanks for the nice patches, Rafael!
Review of attachment 321417 [details] [review]: We can merge this if the designers want US Letter.
Created attachment 321968 [details] [review] pipeline: Ensure that all nodes are actually removed during revert Fixed a minor issue in the revert code.