GNOME Bugzilla – Bug 765136
Changing the aspect ratio of the crop can make the selection larger than the image
Last modified: 2017-04-11 16:58:40 UTC
This is pretty easy to reproduce: Select the Original aspect ratio, make the crop area the entire image, now change the selected aspect ratio to another one. In some cases, this results in the crop area being larger (in one dimension) than the actual image, so the resize handles are invisible.
Created attachment 348961 [details] [review] gnome-photos: selection can be only within an image Problem occured when using crop. Changing the aspect ratio could make the selection larger then the image. To fix this, width and height of selection is controled. It cannot be bigger then width and height of the image. And then, position of selection is controled. https://bugzilla.gnome.org/show_bug.cgi?id=76536
Review of attachment 348961 [details] [review]: Thanks for the nice patch, Katerina. It works as advertised. Some minor comments below. It would be good to ensure that the commit message doesn't exceed 72 characters per line. See: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines ::: src/photos-tool-crop.c @@ +276,3 @@ self->crop_width = sqrt (old_area * self->crop_aspect_ratio); + + if (self->crop_width > self->bbox_source.width) This should be self->bbox_zoomed instead of self->bbox_source. Most of the time we are dealing with the zoomed out image because that is what we are drawing interactively. Only when we actually crop the image, we include the zoom and modify the image at its original size. @@ +280,3 @@ + self->crop_width = self->bbox_source.width; + self->crop_height = self->crop_width / self->crop_aspect_ratio; + self->crop_x = 0; Do we really need to set self->crop_x to 0 here? The rest of the code will take care of that. @@ +286,3 @@ + self->crop_height = self->bbox_source.height; + self->crop_width = self->crop_height * self->crop_aspect_ratio; + self->crop_y = 0; Ditto about self->crop_y. @@ +292,3 @@ + { + crop_center_x = self->crop_width /2; + } Could you please do these adjustments after self->crop_x and self->crop_y have been calculated? That will make them a bit more obvious. For example: if (self->crop_x < 0.0) self->crop_x = 0.0; if (self->crop_x + self->crop_width > self->bbox_zoomed.width) self->crop_x = self->bbox_zoomed.width - self->crop_width; ... same for the other axis. @@ +302,3 @@ + } + if (crop_center_y > self->bbox_source.height - self->crop_height / 2) + { Nitpick: The braces are not properly aligned, and we don't use tabs for indentation.
Created attachment 349243 [details] [review] gnome-photos: selection can be only within an image Problem occured when using crop. Changing the aspect ratio could make the selection larger then the image. To fix this, width and height of selection is controled. It cannot be bigger then width and height of the image. And then, position of selection is controled.
Review of attachment 349243 [details] [review]: This looks much better. Thanks. ::: src/photos-tool-crop.c @@ +293,3 @@ + self->crop_x = 0.0; + if (self->crop_x + self->crop_width > self->bbox_zoomed.width) + self->crop_x = self->bbox_zoomed.width - self->crop_width; I forgot to mention that we can use CLAMP instead of the if branches to make the code shorter.
Created attachment 349343 [details] [review] tool-crop: Restrain the selection when changing aspect ratios I tweaked the code to use CLAMP, fixed the commit message and pushed to master, gnome-3-24 and gnome-3-22.