GNOME Bugzilla – Bug 760931
Cropping not being applied after clicking "Done"
Last modified: 2016-02-17 16:50:41 UTC
If you try to only crop a photo, clicking Done won't work. These are the steps to reproduce the problem: Open gnome-photos -> Select Picture -> Click edit -> Click crop -> Select crop area -> Click Done ==> no cropping To make it work: Open gnome-photos -> Select Picture -> Click edit -> Click crop -> Select crop area -> Change to Colors ==> cropping done
Created attachment 320312 [details] [review] edit: Move "edit-done" implementation to photos-preview-view.c
Created attachment 320314 [details] [review] edit: Deactivate current tool before saving the pipeline
Review of attachment 320312 [details] [review]: Thanks, Umang. This is in the right direction. ::: src/photos-preview-view.c @@ +511,3 @@ + item = PHOTOS_BASE_ITEM (photos_base_manager_get_active_object (self->item_mngr)); + if (item == NULL) + return; Umm... why did you not use 'g_return_if_fail (item != NULL)' as before? If item is NULL, then it should be marked as a CRITICAL because it would indicate a programming error, not a runtime condition that we should cleanly handle. It would be programming error because logic dictates that there must be an active item if we activated 'edit-done'.
Review of attachment 320314 [details] [review]: ::: src/photos-preview-view.c @@ +514,3 @@ + /* Deactivate the current tool before saving the pipeline. */ + photos_preview_view_tool_changed (self, NULL); I think it would be better to retract the tool also. You can use photos_edit_palette_hide_details, and that will take care of calling photos_preview_view_tool_changed. If this affects the hiding animation too much, then we can think of sliding the entire palette upwards, like we do when entering the edit mode, as the final step after the pipeline is saved.
Created attachment 320521 [details] [review] edit: Move "edit-done" implementation to photos-preview-view.c
(In reply to Debarshi Ray from comment #4) > > If this affects the hiding animation too much, then we can think of sliding > the entire palette upwards, like we do when entering the edit mode, as the > final step after the pipeline is saved. Sorry but I didn't get exactly what you are trying to say, like I don't see palette sliding upwards when we "ENTER" the Editing mode. But I do see, palette row sliding upwards while flipping them. Basically, I am going to change : + photos_preview_view_tool_changed (self, NULL); with photos_edit_palette_hide_details (PHOTOS_EDIT_PALETTE (self->palette)); It looks good to me. Please review it and I'll see what part I am not able to understand.
Created attachment 320528 [details] [review] edit: Retract the currently activated tool before saving the pipeline
Review of attachment 320528 [details] [review]: This looks nice. ::: src/photos-preview-view.c @@ +511,3 @@ g_return_if_fail (item != NULL); + /* Retract the currently activated tool before saving the pipeline. */ Nitpick: the comment is not necessary. :)
Review of attachment 320521 [details] [review]: Thanks, Umang. This is much better. ::: src/photos-preview-view.c @@ +513,3 @@ + app = g_application_get_default (); + g_application_hold (app); + photos_base_item_pipeline_save_async (item, NULL, photos_preview_view_edit_done_pipeline_save, self); Thinking about it, we should take a ref on self during the lifetime of the pipeline_save_async operation. It was not necessary when this lived inside PhotosApplication because the hold would take care of it.
Created attachment 320628 [details] [review] application, preview-view: Move the "edit-done" implementation
Created attachment 320629 [details] [review] preview-view: Deactivate the current tool before saving the pipeline
Thanks for the nice patches, Umang.