alternative implantation of "Forbid too small crop region"

[w-e-w]

Description

• this is an alternative implantation add "Forbid too small crop region" option #16602

these two are functional equivalent to yours just one is shorter and coded in a different style
@light-and-ray and I are at odds of believing which version of code is more readable

my opinion is that light-and-ray's is confusing, as during review I've rewrote in order to understand it

after I propose my implementation, light-and-ray believe my implementation is unreadable

---

sorry for wasting time on this situation

Checklist:

• I have read contributing wiki page
• I have performed a self-review of my own code
• My code follows the style guidelines
• My code passes tests

[light-and-ray]

Also I still don't think that we really need to move similar code in a function. They don't require it. Do you want to add a 3rd dimension in the future?

[light-and-ray]

:= is less readable

[light-and-ray]

Awful naming. They are not dimensions, they are lengths. And your function is _expand_segment, how segment can have processing and image. processing_dimension -> desirable_length, image_dimension -> maximal_coordinate can be better

[w-e-w]

not to happy with the names myself
as always bad at naming things

[w-e-w]

I'm pretty sure that dimensions is used to refer to the collective of width and height
dimension is refers to any side
width and height are specific sides

the variable names is a bit strange length seem to be better
maximal_coordinate not too sure

[light-and-ray]

Why = 0 and when c2 -= c1; c1 -= c1 is more readable. Also if is better then min in this case

[w-e-w]

Why = 0

imo c1 = 0 you read and see that something is just 0
while c1 -= c1 you need to think about it for a sec
not important in the grand scheme of things

Also if is better then min in this case

can't agree, I think min an max makes code cleaner

[light-and-ray]

Why = 0

imo c1 = 0 you read and see that something is just 0
while c1 -= c1 you need to think about it for a sec

Imo c1 -= c1 doesn't require any thoughts that it's 0, and without min function c2 -= c1 in pair with c1 -= c1 tells you that you're shifting the segment

[w-e-w]

still not convinced if is more readable

def expand_too_small_crop_region(crop_region, processing_width, processing_height, image_width, image_height):
    """expands a crop_region to not have dimensions smaller than processing_dimensions"""

    def _expand_segment(c1, c2, desirable_length, maximal_coordinate):
        """expands the segment given by c1 c2 to the desired_dimension but not exceeding the boundaries of the maximal_coordinate"""
        diff = desirable_length + c1 - c2
        if diff > 0:
            # if the region is smaller than desirable_length, extend both sides equally
            diff_l = diff // 2
            c1 -= diff_l
            c2 += diff - diff_l
            if c1 < 0:  # shift the region to the right by c1
                c2 -= c1
                c1 = 0
                if c2 > maximal_coordinate:
                    c2 = maximal_coordinate
            elif c2 >= maximal_coordinate:  # shift the region to the left by (c2 - maximal_coordinate)
                c1 -= c2 - maximal_coordinate
                c2 = maximal_coordinate
                if c1 < 0:
                    c1 = 0
        return c1, c2

    x1, y1, x2, y2 = crop_region

[light-and-ray]

I don't think this new data is useful in the console output. Crop region is an internal variable. For user could be more useful new padding value, but in this case it can be different for w and h. I thought about it

[w-e-w]

to be honest I wasn't considering if it was useful to the user
since you already added the print I just put my debug coordinate outputs also in ther
and didn't remove it afterwards

do you think is notified user about the adjustment of crop region even useful in the first place?

[light-and-ray]

I think it's useful because a person can not understand why their generation in the preview looks like padding is too big although they set it small, and so it can confuse them, so this message should be. But with no cropping region debugging info

[w-e-w]

Also I still don't think that we really need to move similar code in a function. They don't require it. Do you want to add a 3rd dimension in the future?

it's not about adding a third dimension

with a function you read it once and you know it behaves the same
on the other hand two blocks of code that serves the same purpose but have different variables that you have to individually keep track of is more confusing