Skip to content

Conversation

@evil159
Copy link
Contributor

@evil159 evil159 commented Apr 13, 2022

This PR provides an API to add a resizable image to the style without calculating stretchX and stretchY.

The resizable image can be defined either in Xcode asset catalog or programmatically, e.g.:

let image = UIImage(named: "circle")! // a 25x25 pt image of a colour filled circle
    .resizableImage(withCapInsets: UIEdgeInsets(top: 12, left: 12, bottom: 12, right: 12))

Which would produce the following result:

Simulator.Screen.Recording.-.iPhone.13.-.2022-04-13.at.12.15.27.mp4

Public API additions to Style

The method to add a resizable image

public func addImageResizable(
    _ image: UIImage,
    id: String,
    sdf: Bool = false,
    content: ImageContent? = nil) throws

A convenience method for adding regular images

public func addImage(
    _ image: UIImage,
    id: String,
    sdf: Bool = false,
    content: ImageContent? = nil) throws

Pull request checklist:

  • Write tests for all new functionality. If tests were not written, please explain why.
    Tests are not written as Style is not particularly testable at this point
  • Add documentation comments for any added or updated public APIs.
  • Describe the changes in this PR, especially public API changes.
  • Add a changelog entry to to bottom of the relevant section (typically the ## main heading near the top).
  • Review and agree to the Contributor License Agreement (CLA).

id: id,
stretchX: [ImageStretches(first: stretchXFirst, second: stretchXSecond)],
stretchY: [ImageStretches(first: stretchYFirst, second: stretchYSecond)],
content: content)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is a bit different here in compared to both Android and the v9 SDK.

In Android 9-patch image can also contain a padding box defining where the overlayed text can be drawn. So the Android SDK provides a derivative of that as the image content - source. iOS doesn't provide the ability to define content padding for a resizable image.

The v9 iOS SDK defines the image content area to be the same as the stretchable area of the image - source. While this produces a good looking result by default, it makes impossible to draw content on a resizable part of the image.

I decided to leave the image content as a parameter accessible, if needed, to the user. To make the API more Swifty the parameter type could be UIEdgeInsets instead of ImageContent, as edge insets should be more familiar to iOS developers.

Another point about the image content is that, for example, SymbolLayer has iconTextFitPadding property. Which does pretty much the same job as image content here. Though there probably might be some cases where image content is the only option to provide a content drawing box.

Copy link
Contributor

@ZiZasaurus ZiZasaurus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I also like the conscious change to addImageResizeable as it will make it easier for customers to encounter in the autocomplete.

Copy link
Contributor

@macdrevx macdrevx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need separate methods for addImage and addImageResizable? What if we just offered addImage and made it respect cap insets by default (and potentially had it take a flag to disable that behavior)?

@evil159
Copy link
Contributor Author

evil159 commented Apr 14, 2022

Do we need separate methods for addImage and addImageResizable? What if we just offered addImage and made it respect cap insets by default (and potentially had it take a flag to disable that behavior)?

Good point. Indeed, there's no immediate need to have two separate methods for regular and resizable images. It was a conscious decision. Adding resizable image support to the existing method might be considered a breaking change, as it would lead to changes in behaviour of the method when a resizable image is supplied.

I think it's a sound idea to add an opt out flag, however I feel that will make the "addImage(..." method even more crowded.

func addImage(_ image: UIImage,
                id: String,
                sdf: Bool = false,
                enableResizing: Bool = true,
                stretchX: [ImageStretches],
                stretchY: [ImageStretches],
                content: ImageContent? = nil) throws

If I understand your suggestion correctly, one would need to explicitly state that they want image to be resized by setting the flag. In that case I think there's very little difference between that and using a distinct method to add a resizable image.

I wonder if we could change the behaviour of addImage(... method to support resizable images by default(without a flag) in the next major version. If so, then adding a flag is definitely the way to, so the flag can be removed later on.

Comment on lines 773 to 777
guard image.capInsets != .zero else {
Log.warning(forMessage: "The supplied image is missing cap insets and might be resized incorrectly.")
try addImage(image, id: id, stretchX: [], stretchY: [])
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard statement makes me think that we don't need a separate methods.
It looks like we can introduce united addImage method and highlight in the description that this method supports resizable images.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is what I had in mind: #1269 (review)

Every UIImage has a non-optional capInsets property. It just defaults to zero for non-resizable images.

@OdNairy
Copy link
Contributor

OdNairy commented Apr 14, 2022

Adding resizable image support to the existing method might be considered a breaking change, as it would lead to changes in behaviour of the method when a resizable image is supplied.

It's an interesting problem on definition a "breaking changes". As for me, we should not consider that kind of changes as a breaking one. I feel like we had a bug which we just fixed.

@macdrevx
Copy link
Contributor

macdrevx commented Apr 14, 2022

My suggestion:

  1. Leave the preexisting addImage API (the one that involves imageStretches) as-is.
  2. Add a single new method:
public func addImage(
    _ image: UIImage,
    id: String,
    sdf: Bool = false,
    contentInsets: UIEdgeInsets = .zero) throws

and document that it respects UIImage.capInsets but not UIImage.resizingMode (the result is always as if resizingMode == .stretch)


I'd mentioned possibly adding a flag about whether to respect capInsets — that was targeted towards the new method — but thinking about it more, it seems unnecessary. Developers who don't want resizing behavior should just pass images with capInsets == .zero.

@evil159 evil159 requested review from OdNairy and macdrevx April 19, 2022 12:48
/// Setting this to `true` allows template images to be recolored. The
/// default value is `false`.
/// - contentInsets: The distances the edges of content are inset from the image rectangle.
/// If present, and if the icon uses icon-text-fit, the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specific to symbol layer?

@evil159 evil159 force-pushed the rl/resizable-image-support branch from e4ced50 to 1410ea8 Compare April 19, 2022 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants