GNOME Bugzilla – Bug 781134
Support changing the orientation of the crop rectangle
Last modified: 2017-07-19 10:18:46 UTC
It should be possible to change the orientation of the crop rectangle from portrait to landscape and vice versa. Currently it is locked to the orientation of the original image and there is no way to change it. Here is a mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/photos/wires-editing.png
Created attachment 350454 [details] [review] tool-crop: Rename variable
Created attachment 350455 [details] [review] preview-view: Don't expand the edit sidebar
Created attachment 350456 [details] [review] utils: Add get_rectangle_surface This will be used in order to create custom icons that represent different orientation and aspect ratio
Created attachment 350457 [details] [review] tool-crop: Add orientation buttons
Created attachment 350458 [details] [review] tool-crop: Add 'orientable' field This will be useful to know when we should reveal the orientation buttons
Created attachment 350459 [details] [review] tool-crop: Reveal orientation buttons when appropriate
Created attachment 350460 [details] [review] tool-crop: Handle when orientation buttons are pressed
Created attachment 350461 [details] [review] tool-crop: Add orientation buttons
Created attachment 350462 [details] [review] tool-crop: Add 'orientable' field This will be useful to know when we should reveal the orientation buttons
Created attachment 350463 [details] [review] tool-crop: Reveal orientation buttons when appropriate
Created attachment 350464 [details] [review] tool-crop: Handle when orientation buttons are pressed
Review of attachment 350454 [details] [review]: Thanks for these patches, Alessandro! Looks good to me.
Review of attachment 350462 [details] [review]: ++ ::: src/photos-tool-crop.c @@ +113,3 @@ guint basis_height; guint basis_width; + gboolean orientable; Nitpick: I'd move it above to retain the order.
Created attachment 354576 [details] [review] tool-crop: Add 'orientable' field
Review of attachment 350456 [details] [review]: ::: src/photos-utils.c @@ +1116,3 @@ + cr = cairo_create (surface); + + cairo_set_source_rgba (cr, 1, 1, 1, 1); I learnt from #gtk+ that gtk_style_context_get_color might be a better way to pick the colour. I am worried that hard coding it like this will make it look odd when the window's focus changes.
The mockups have been changed, now there is no need each orientation (and aspect ratio) option to have its own box icon.
Created attachment 354838 [details] [review] preview-view: Don't expand the edit sidebar
Created attachment 354839 [details] [review] tool-crop: Add orientation buttons
Created attachment 354840 [details] [review] tool-crop: Reveal orientation buttons when appropriate
Created attachment 354841 [details] [review] tool-crop: Handle when orientation buttons are pressed
Created attachment 354842 [details] [review] tool-crop: Select the orientation depending on the original ratio
Review of attachment 354838 [details] [review]: ::: src/photos-preview-view.c @@ +1055,3 @@ sw = gtk_scrolled_window_new (NULL, NULL); gtk_scrolled_window_set_policy (GTK_SCROLLED_WINDOW (sw), GTK_POLICY_NEVER, GTK_POLICY_AUTOMATIC); + gtk_widget_set_hexpand (GTK_WIDGET (sw), FALSE); It will be slightly better to set it on the EditPalette below.
Created attachment 355819 [details] [review] preview-view: Don't horizontally expand the EditPalette
Review of attachment 354576 [details] [review]: Thinking about this some more ... ::: src/photos-tool-crop.c @@ +107,3 @@ { PhotosToolCropAspectRatioType aspect_ratio_type; + gboolean orientable; Just a constant boolean value isn't enough. For example, if the original BaseItem is a square then it doesn't make sense to show the orientation buttons. Since we cannot determine that at compile time, a certain amount of run-time adjustment is needed.
Review of attachment 354839 [details] [review]: Thanks for these new set of patches! While the small-sized patches are very convenient for reviewing, I think we should squash the UI bits before merging. ::: src/photos-tool-crop.c @@ +1258,3 @@ + gtk_style_context_add_class (context, "linked"); + + orientation_size_group = gtk_size_group_new (GTK_SIZE_GROUP_HORIZONTAL); We need to unref the SizeGroup once we have added all the widgets to it. @@ +1274,3 @@ + gtk_size_group_add_widget (orientation_size_group, self->portrait_button); + + gtk_revealer_set_reveal_child (GTK_REVEALER (self->orientation_revealer), TRUE); It will be better to do this in photos_tool_crop_set_active so that visibility of the orientation buttons is updated whenever we manually set the "active ratio" ourselves. eg., on reset, when the tool is activated for a BaseItem with a pre-existing crop.
Review of attachment 354840 [details] [review]: Looks good. Should be squashed into the previous patch.
Review of attachment 354841 [details] [review]: ::: src/photos-tool-crop.c @@ +162,3 @@ + if ((ret_val > 1 && gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->portrait_button))) || + (ret_val < 1 && gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->landscape_button)))) + ret_val = 1.0 / ret_val; This is correct as long as we are interactively cropping a BaseItem. We also have to handle the case when the ToolCrop has been activated on an item that was cropped earlier. That's when photos_base_item_operation_get returns TRUE in photos_tool_crop_activate. In that case, we read the extents of the gegl:crop from the item, temporarily remove the crop to reveal all the pixels, and set up the tool's widgets to mimic the existing crop. This second code path uses _calculate_aspect_ratio via _find_constraint. I think we have to: (a) Split _calculate_aspect_ratio into 2 variants. One that returns the aspect ratio without applying the orientation, and one after applying it. Then, _find_constraint can continue use the former variant. (b) Change _find_constraint to look at both the "raw" aspect ration and its reciprocal to find out the constraint and its orientation.
Review of attachment 354842 [details] [review]: ::: src/photos-tool-crop.c @@ +976,3 @@ + original_aspect_ratio = (gdouble) self->bbox_source.width / self->bbox_source.height; + active_button = original_aspect_ratio > 1 ? self->landscape_button : self->portrait_button; + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (active_button), TRUE); I think we can only do this in the "else" branch below. For BaseItems with a pre-existing crop, we need to set up the orientation buttons to reflect the current state.
Created attachment 355857 [details] [review] tool-crop: Always express the "original" constraint as landscape
Created attachment 355858 [details] [review] tool-crop: Support changing the orientation of the crop rectangle I fixed up the above issues and squashed the patches together. It seems to work as far as I can see, but, Alessandro, could you try it out and see if it works for you?
Created attachment 355860 [details] [review] tool-crop: Support changing the orientation of the crop rectangle We need some row-spacing between the list of ratios and the orientation buttons.
Review of attachment 355860 [details] [review]: ::: src/photos-tool-crop.c @@ +883,3 @@ gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (self->lock_button), TRUE); + active = 1; + orientation_button = self->landscape_button; - open a portrait photo - open the crop tool - set the orientation to landscape - press reset - reopen the crop tool The selected button is the portrait one, but the crop area is in landscape. I guess it is because we activate the landscape button unconditionally on reset. Something like this should solve it: aspect_ratio = (gdouble) self->bbox_source.width / self->bbox_source.height; orientation_button = aspect_ratio > 1.0 ? self->landscape_button : self->portrait_button;
Created attachment 355913 [details] [review] tool-crop: Clear "reset" earlier in the activation sequence
Created attachment 355914 [details] [review] tool-crop: Support changing the orientation of the crop rectangle
Review of attachment 355913 [details] [review]: Looks ok to me.
Created attachment 355926 [details] [review] tool-crop: Support changing the orientation of the crop rectangle Extend the g_assert_null in photos_tool_crop_set_active to cover the 'active != -1' case. Pointed out by Alessandro on IRC.
Review of attachment 355926 [details] [review]: Looks ok to me.
(In reply to Debarshi Ray from comment #24) > Review of attachment 354576 [details] [review] [review]: > > Thinking about this some more ... > > ::: src/photos-tool-crop.c > @@ +107,3 @@ > { > PhotosToolCropAspectRatioType aspect_ratio_type; > + gboolean orientable; > > Just a constant boolean value isn't enough. For example, if the original > BaseItem is a square then it doesn't make sense to show the orientation > buttons. Since we cannot determine that at compile time, a certain amount of > run-time adjustment is needed. I filed bug 785115 for this.
Thanks for pushing this through, Alessandro.