Skip to content

add new initializer for transform rotation to CGAffineTransform extension - #395

Merged
michaelknoch merged 4 commits into
masterfrom
add-api-UIKit-rotation
Aug 7, 2024
Merged

add new initializer for transform rotation to CGAffineTransform extension#395
michaelknoch merged 4 commits into
masterfrom
add-api-UIKit-rotation

Conversation

@michaelknoch

Copy link
Copy Markdown
Member

Type of change: feature

Motivation (current vs expected behavior)

Add public init(rotationAngle: CGFloat)

Please check if the PR fulfills these requirements

  • Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
  • There are no dependencies on other PRs or I have linked dependencies through Zenhub
  • The commit messages are clean and understandable
  • Tests for the changes have been added (for bug fixes / features)

@michaelknoch
michaelknoch requested a review from reckjn July 31, 2024 10:46
@michaelknoch
michaelknoch enabled auto-merge (squash) July 31, 2024 10:47
@michaelknoch
michaelknoch merged commit 6aa03fa into master Aug 7, 2024
@michaelknoch
michaelknoch deleted the add-api-UIKit-rotation branch August 7, 2024 17:11
Comment on lines +16 to +18
init(rotationAngle: CGFloat) {
self.init(rotationByRadians: rotationAngle)
}

@ephemer ephemer Aug 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was a bit confused by this and just looked into it. Apparently AffineTransform is a Foundation (only?) type that has this rotationByRadians initializer. CGAffineTransform, which AFAIK is what is actually used by UIView in UIKit, appears to have different initializers.

Our CGAffineTransform type is copied from Foundation's AffineTransform type, so it's a bit of a weird mix of both. I'm not sure what implications this has for code re-use. It's probably "close-enough" API-wise that we can use it as is, especially given this new extension. But we should be aware of the differences

@michaelknoch @reckjn

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Got it, thanks for the info. I was confused as well tbh. Do you think our CGAffineTransform needs to be refactored because of this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we actually intend to create a real CGAffineTransform type here. So I would suggest to actually rename the rotationByRadians initializer to be called rotationAngle, instead of the change in this PR.

It would be a bit weird to see both in code completion, and then not be actually source compatible if you choose the wrong one.

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.

3 participants