Created attachment 359367 [details] [review]
tool-crop: Make constraints as a part of instance

An attempt to fix the issue. Thoughts ?

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.

> 
> 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.

Created attachment 359393 [details] [review]
tool-crop: Make constraints as a part of instance

Created attachment 359394 [details] [review]
tool-crop: Toggle the orientable field based on image's aspect ratio

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.

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.

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.

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. :)

Created attachment 359879 [details] [review]
tool-crop: Update the orientable field based on image's original size

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.

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.

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.

Created attachment 359991 [details] [review]
tool-crop: Hide orientation controls for square items & original ratio

Thanks for all the effort, Umang!