After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785115 - tool-crop: Don't show the orientation controls if it doesn't make sense for the "original" aspect ratio
tool-crop: Don't show the orientation controls if it doesn't make sense for t...
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-07-19 10:16 UTC by Debarshi Ray
Modified: 2017-09-18 15:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tool-crop: Make constraints as a part of instance (10.91 KB, patch)
2017-09-07 16:42 UTC, Umang Jain
none Details | Review
tool-crop: Make constraints as a part of instance (7.65 KB, patch)
2017-09-08 11:13 UTC, Umang Jain
none Details | Review
tool-crop: Toggle the orientable field based on image's aspect ratio (1.49 KB, patch)
2017-09-08 11:13 UTC, Umang Jain
none Details | Review
tool-crop: Make constraints as a part of instance (8.76 KB, patch)
2017-09-15 12:52 UTC, Umang Jain
committed Details | Review
tool-crop: Update the orientable field based on image's original size (1.45 KB, patch)
2017-09-16 02:44 UTC, Umang Jain
committed Details | Review
tool-crop: Make "constraints" an instance variable (9.37 KB, patch)
2017-09-18 15:12 UTC, Debarshi Ray
committed Details | Review
tool-crop: Hide orientation controls for square items & original ratio (1.44 KB, patch)
2017-09-18 15:12 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2017-07-19 10:16:59 UTC
The orientation controls added in bug 781134 are always shown for the "original" aspect ratio constraint, regardless of whether the input BaseItem was a square or not. We should take that into consideration.
Comment 1 Umang Jain 2017-09-07 16:42:00 UTC
Created attachment 359367 [details] [review]
tool-crop: Make constraints as a part of instance

An attempt to fix the issue. Thoughts ?
Comment 2 Debarshi Ray 2017-09-07 23:02:11 UTC
Review of attachment 359367 [details] [review]:

Thanks for working on this, Umang. I didn't get the chance to run it, yet. A few quick comments from merely looking at it:

::: src/photos-tool-crop.c
@@ +1319,3 @@
 
+  for (i = 0; i < G_N_ELEMENTS (CONSTRAINTS); i++)
+    photos_tool_crop_constraint_free(self->constraints[i]);

Better to do this in _finalize. The _dispose method is for things that can possibly have a reference cycle, whereas there is no reference counting for the constraints.

Also _dispose can be invoked multiple times, so we need to guard against that. Otherwise this will crash if it's run more than once.

@@ +1368,3 @@
 
+  self->constraints = g_new0 (PhotosToolCropConstraint*, G_N_ELEMENTS (CONSTRAINTS));
+  self->constraints[0] = photos_tool_crop_constraint_new (CONSTRAINTS[0].aspect_ratio_type,

Is there a particular reason why the 0th element is assigned separate from the rest? I understand that it is ANY, so it's unconstrained, but will there be any problem if we kept them together?

I think it's better to move the entire array here, and toggle the orientable field when we get a new BaseItem in _activate. I think, it will reduce the delta a bit and make it more readable.
Comment 3 Umang Jain 2017-09-08 11:12:39 UTC
> 
> Is there a particular reason why the 0th element is assigned separate from
> the rest? I understand that it is ANY, so it's unconstrained, but will there
> be any problem if we kept them together?

Actually the approach took 2-3 tries to get it working. In one of the cases, photos_tool_crop_set_active (self, -1, NULL) was giving segfault, hence I figured that 0th element is not being a part of instance, so I quickly assigned it (As you see in the patch). 

> I think it's better to move the entire array here, and toggle the orientable
> field when we get a new BaseItem in _activate. I think, it will reduce the
> delta a bit and make it more readable.

This is done in the following patches.
Comment 4 Umang Jain 2017-09-08 11:13:19 UTC
Created attachment 359393 [details] [review]
tool-crop: Make constraints as a part of instance
Comment 5 Umang Jain 2017-09-08 11:13:47 UTC
Created attachment 359394 [details] [review]
tool-crop: Toggle the orientable field based on image's aspect ratio
Comment 6 Debarshi Ray 2017-09-12 10:02:56 UTC
Review of attachment 359394 [details] [review]:

Looks better, thanks.

::: src/photos-tool-crop.c
@@ +1035,2 @@
 static void
+photos_tool_crop_find_real_orientable (PhotosToolCrop *self)

Since the method doesn't return anything "update" might be a better name than "find". eg., _update_original_orientable.

@@ +1041,3 @@
+    {
+      /* Handle the case when image is a square */
+      if (photos_utils_equal_double (self->bbox_source.width, self->bbox_source.height) &&

self->bbox_source is a GeglRectangle. The width and height fields are integers, not floating points. So we should use a simple '==' comparison.

@@ +1042,3 @@
+      /* Handle the case when image is a square */
+      if (photos_utils_equal_double (self->bbox_source.width, self->bbox_source.height) &&
+          CONSTRAINTS[i].basis_height == CONSTRAINTS[i].basis_width)

I'd compare the aspect_ratio_type field against PHOTOS_TOOL_CROP_ASPECT_RATIO_ORIGINAL. That'll make the code more obvious, and make the previous comment redundant.

We assume that the "square" constraint already has its orientable set to FALSE. It would be a programming error if it's not. If you want you could add an assertion when building the constraints array.

@@ +1097,3 @@
       gdouble aspect_ratio;
 
+      photos_tool_crop_find_real_orientable (self);

Doesn't this need to happen outside the if block? Otherwise, the "orientable" won't be updated when revisiting a cropped item.
Comment 7 Debarshi Ray 2017-09-12 10:25:26 UTC
Review of attachment 359393 [details] [review]:

Thanks for splitting up the patches!

::: src/photos-tool-crop.c
@@ +62,3 @@
+  PHOTOS_TOOL_CROP_ASPECT_RATIO_BASIS,
+  PHOTOS_TOOL_CROP_ASPECT_RATIO_ORIGINAL
+} PhotosToolCropAspectRatioType;

Nitpick: it should be moved up to retain the alphabetical order.

@@ +105,3 @@
   gint list_box_active;
   gulong size_allocate_id;
+  PhotosToolCropConstraint **constraints;

Ditto.

@@ +1318,3 @@
                             self);
 
+  self->constraints = g_new0 (PhotosToolCropConstraint*, G_N_ELEMENTS (CONSTRAINTS));

Nitpick: g_malloc0_n is slightly preferred than g_new0 for consistency with the rest of the code.

How about having an array of ToolCropConstraint elements? Instead of an array of pointers to ToolCropConstraints. That will simplify the code. You wouldn't need tool_crop_constraint_new/free. You could just use g_malloc0_n and g_free.

Secondly, it will be good to not have CONSTRAINTS as a global array anymore. Otherwise, code might start using self->constraints and CONSTRAINTS interchangeably (like a few lines below :) and lead to needless bugs and confusion. I'd create a small static method, say _create_constraints, that has a local constraints array which is used to create self->constraints.
Comment 8 Umang Jain 2017-09-15 12:52:45 UTC
Created attachment 359845 [details] [review]
tool-crop: Make constraints as a part of instance

I don't know why the diff of initial parts is a bit screwed up.
Comment 9 Debarshi Ray 2017-09-15 19:54:24 UTC
Review of attachment 359845 [details] [review]:

Better, much better. I took a very quick glance.

::: src/photos-tool-crop.c
@@ +1237,3 @@
+  PhotosToolCrop *self = PHOTOS_TOOL_CROP (object);
+
+  g_free (self->constraints);

Forgot to chain up to the parent. :)
Comment 10 Umang Jain 2017-09-16 02:44:31 UTC
Created attachment 359879 [details] [review]
tool-crop: Update the orientable field based on image's original size
Comment 11 Debarshi Ray 2017-09-18 15:04:12 UTC
Review of attachment 359845 [details] [review]:

I took a closer look. Couldn't spot anything wrong other than the not chaining up in _finalize, and a few nitpicky details.

::: src/photos-tool-crop.c
@@ +207,3 @@
   guint ret_val = 0; /* ANY */
 
+  for (i = 0; self->constraints[i].aspect_ratio_type != 0; i++)

Better to use ASPECT_RATIO_NONE, instead 0, since we added that constant. Consistent use makes the code easier to read.

@@ +1303,3 @@
+  photos_tool_crop_create_constraints (self);
+
+  for (i = 1; self->constraints[i].aspect_ratio_type != 0; i++)

Ditto about ASPECT_RATIO_NONE instead of 0.
Comment 12 Debarshi Ray 2017-09-18 15:11:12 UTC
Review of attachment 359879 [details] [review]:

Nice patch.

Generally, it is better to mention the user visible change in the commit's subject. eg., in this case this patch hides the orientation controls for square items when the original aspect ratio is being used. The actual code changes are already part of the patch, so it is more important to mention things that are not obvious from reading the diff. eg., what they achieve, why it was done this way, trade-offs, etc..

::: src/photos-tool-crop.c
@@ +1028,3 @@
+      if (self->constraints[i].aspect_ratio_type == PHOTOS_TOOL_CROP_ASPECT_RATIO_ORIGINAL &&
+          self->bbox_source.width == self->bbox_source.height)
+        self->constraints[i].orientable = FALSE;

It'll be better to set both TRUE and FALSE depending on the dimensions. Otherwise, if the same ToolCrop instance is first used with a square BaseItem, followed by a non-square one, the orientable field won't get reset to TRUE.

The current layout of the code ensures a new set of tool instances when the BaseItem changes, so this isn't a problem in practice. However, it's trivial to ensure correctness, so let's do that.
Comment 13 Debarshi Ray 2017-09-18 15:12:20 UTC
Created attachment 359990 [details] [review]
tool-crop: Make "constraints" an instance variable

Fixed and pushed. Also updated the commit message to mention why this change is needed, since it doesn't do anything on its own.
Comment 14 Debarshi Ray 2017-09-18 15:12:34 UTC
Created attachment 359991 [details] [review]
tool-crop: Hide orientation controls for square items & original ratio
Comment 15 Debarshi Ray 2017-09-18 15:12:59 UTC
Thanks for all the effort, Umang!