Image Squaring Stategy

[andrewdmontgomery]

Closes #364

Description

Testing Steps

[wpmobilebot]

📲 You can test the changes from this Pull Request in Gravatar UIKit Prototype Build by scanning the QR code below to install the corresponding build.

	App Name	Gravatar UIKit Prototype Build
	Build Number	1660
	Version	1.0
	Bundle ID	com.automattic.gravatar-sdk-demo-uikit.prototype-build
	Commit	fb166a8293788048d67d5838bd388b99ff60a03b
	App Center Build	Gravatar SDK Demo - UIKit #348

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Gravatar SwiftUI Prototype Build by scanning the QR code below to install the corresponding build.

	App Name	Gravatar SwiftUI Prototype Build
	Build Number	1660
	Version	1.0
	Bundle ID	com.automattic.gravatar-sdk-demo-swiftui.prototype-build
	Commit	fb166a8293788048d67d5838bd388b99ff60a03b
	App Center Build	Gravatar SDK Demo - SwiftUI #343

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[pinarol]

I like this idea. I think 3rd parties can really benefit from the small squaring touch we do here. Because the iOS method cropping(to: ...) can return slightly uneven edges and I think it is the "go to" method when it comes to cropping. Our default strategy cuts the longer edge only if the diff is very small.

[pinarol]

Also cc: @etoledom

[pinarol]

We only need the ImageSquaringStrategy in the AvatarService.upload(...) method. Not in the other UI parts. Our cropper already creates perfect squares. This is more for devs who use AvatarService directly.

Because most devs use either their own cropper with cropping(to: ...) or the UIImagePickerController's cropper and both can generate imperfect squares(I mean just a few pixels diff between edges). This can be difficult to notice during development because it doesn't always happen, and fixing this is not much fun. So we are just compensating for that. That's why we only intended to fix the image when the diff is really small. Noticeable changes to the images are better done with a visual cropper.

So instead of having an enum like ImageSquaringStrategy we can also have a parameter like edgeDifferenceFixingThreshold: CGFloat = 0.02 in the AvatarService.upload(...). And this would replace the minorDifferenceThreshold constant we already have. They can pass 0 if they don't want us to intervene. This is probably more suitable for the purpose.

[andrewdmontgomery]

Yeah, the more I worked on this yesterday, the more weird it got.

We only need the ImageSquaringStrategy in the AvatarService.upload(...) method. Not in the other UI parts. Our cropper already creates perfect squares. This is more for devs who use AvatarService directly.

That's clarifying. Going into this, I thought we were trying to give devs complete control over the squaring check (like we do with the custom editor). But the more I tried to make that work, the weirder it got. If you are going to provide your own editor, it makes sense to make you responsible for delivering a square image from your editor. And if your editor isn't producing square images, under what circumstances would you rather apply a second "squaring" function instead of just updating your editor?

Then I saw this comment in the AvatarPickerView.swift:

// If there's a custom image editor, it should take care of squaring.

If that's true of the custom image editor, it should also be true of ours. So, I refactored the image editing flow to guarantee that it produces perfectly square images by:

• applying the squaring strategy immediately before the onImageEdited() callback
• giving developers the ability to set their own squarer
• giving developers the option of using the default squarer, but with a custom background

But again, why would developers use a custom squarer like that instead of updating their editor?

What you are proposing here is simpler. Instead of refactoring the image editor flow to guarantee square images, we can just add an assert(image.isSquare) just before we call onImageEdited() callback, so that we give developers a warning.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Gravatar Prototype Build by scanning the QR code below to install the corresponding build.

	App Name	Gravatar Prototype Build
	Build Number	1773
	Version	1.0
	Bundle ID	com.automattic.gravatar-sdk-demo-uikit.prototype-build
	Commit	cb58cf68063ed0acaffc300d4847381251d7f38a
	App Center Build	Gravatar SDK Demo - UIKit #412

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[andrewdmontgomery]

So instead of having an enum like ImageSquaringStrategy we can also have a parameter like edgeDifferenceFixingThreshold: CGFloat = 0.02 in the AvatarService.upload(...). And this would replace the minorDifferenceThreshold constant we already have. They can pass 0 if they don't want us to intervene. This is probably more suitable for the purpose.

In our current implementation, that threshold defines whether the non-square image uses aspect fit or aspect fill. All differences above that threshold will fit with background color added (.black).

I think setting that threshold to 0 (which would mean all non-square images will use aspect fit) would be a legitimate choice.

So I went with the argument name cropToFitThreshold, and I've made it optional with our default value of 0.02:

• If set, the threshold defines which images are fit and fill
• If nil, no squaring is applied

[andrewdmontgomery]

Could you explain the Demo app changes a bit? I am a bit confused about what to expect from the new segmented control.

Yeah, I really don't love this UX. We just have no extra room. This is the last row we can add to the setting before we start pushing things off the bottom of an iPhone SE. So, just to make this testable, I squished and abbreviated.

I'm going to give this UX some more thought today, with a clearer head than last week. Then, I was going to write up in the description whatever we decide on.

For now...

The segmented control decides where squaring happens:

• Does the Image Picker continue to handle squaring the image with its own logic, as it currently does?
• Or, does the Image Picker just deliver an image, and then squaring will be handled during the upload (via AvatarService)?

The problem with the existing Image Upload is that it calls .squared() on the selected image before returning it. So the new squaring logic in the upload can't be used. This setting allows us to bypass that if we want.

Also, to be able to enter decimal numbers I think we need to make the keyboard type .decimalPad.

Oops, yes. Thanks.

And this page overall doesn't look nice when dark mode is enabled (In Simulator "Settings > Developer > Dark Appearance"). For example the text color is not visible. It can be nice to use system defined colors so things can adjust automatically. If this looks like a big effort feel free to open a new issue so we can handle separately.

Thanks for catching this.

[andrewdmontgomery]

I pushed those changes. Dark mode is readable now, and the decimal pad is used. I'll update the description.

This UX needs to be revisited, though. But #129 may be a better place to do that, perhaps with sub-issues.

[etoledom]

@andrewdmontgomery - Thanks for improving the looks of the demo app!

And this page overall doesn't look nice when dark mode is enabled

I've been having this in mind for a very long time.

This ticket (https://github.com/Automattic/Gravatar-SDK-iOS/issues/129) is meant to handle these kind of UI issues on the demo 👍

cc @pinarol

[andrewdmontgomery]

All changes are in this branch now

[andrewdmontgomery]

CropToFit references are removed, and I updated some of the docs.

The UI in the Demo app is definitely going to need some love:

That whole Square: line is too cramped. And the Min: label barely makes senes even if you know what it is. There just isn't any more room on an SE screen.

[andrewdmontgomery]

Let's keep changes minimal. I think we don't need 90% of changes in this PR anymore. It can be easier to start from trunk at this point.

Agreed.

Take one more look. I've pulled a ton out:

• The new squaring function is only called during AvatarService.upload(), and is scoped internal to Gravatar
• No public changes to the API
• All of the new maxSize logic has been pulled (See comment above)
• Clamping of CGFloat is just a function extended from CGFloat

Other change

• AvatarPickerViewModel no longer squares images as it used to. This will cause a caching bug as described above. (See comment above)

[andrewdmontgomery]

The PR description has been updated for the simplified scope, including testing steps.

[andrewdmontgomery]

Since the Demo app no longer needs changes for this PR, this PR no longer fixes the Dark Mode issues with the Upload demo. But that can be handled in the UX improvements issue (#129)

[andrewdmontgomery]

Looks good to me 🎉 And thanks for your patience 🙏 This was such a roller coaster.

Thanks for your diligence and guidance. 🙏