Skip to content

Add Codable conformance to common CG types #10343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 20, 2017
Merged

Add Codable conformance to common CG types #10343

merged 1 commit into from
Jun 20, 2017

Conversation

itaiferber
Copy link
Contributor

What's in this pull request?
Addresses SR-5237.
Gives a custom Codable implementation for CGAffineTransform, CGPoint, CGSize, CGRect, and CGVector, along with unit tests.

@itaiferber
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 4f9e8073b0bf9df75475aafe2ebeb6545553c320
Test requested by - @itaiferber

@itaiferber itaiferber force-pushed the cg-types-codable-conformance branch from 4f9e807 to d4f4033 Compare June 19, 2017 16:26
@itaiferber
Copy link
Contributor Author

Some large test values need to be gated on OS version for JSON.

@itaiferber
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 4f9e8073b0bf9df75475aafe2ebeb6545553c320
Test requested by - @itaiferber

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 4f9e8073b0bf9df75475aafe2ebeb6545553c320
Test requested by - @itaiferber

@jrose-apple
Copy link
Contributor

For my own edification, can you share the rationale for making these all unkeyed? It seems like the affine transform, at least, would benefit from labels in its default encoded representation.

@itaiferber
Copy link
Contributor Author

@jrose-apple These types are the common currency types for doing most graphics work (macOS has NSPoint, NSRect, etc. from AppKit, but works with the CG variants when doing anything CG related, and iOS & co. use exclusively the CG versions) — they're often transacted upon and very often encoded. When they're encoded, they're usually encoded many many times over (think arrays of points to represent paths). Their representations are basically locked in forever; we can't change them without causing massive breakage across all OSes, so, it's relatively safe to do this, and it's more efficient than encoding keys.

@itaiferber itaiferber force-pushed the cg-types-codable-conformance branch from d4f4033 to e06b7e6 Compare June 19, 2017 21:10
@itaiferber
Copy link
Contributor Author

Missed a CGAffineTransform test case

@itaiferber
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - d4f4033d24c124edaa8ba261c7f723a87ac5436f
Test requested by - @itaiferber

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - d4f4033d24c124edaa8ba261c7f723a87ac5436f
Test requested by - @itaiferber

@itaiferber itaiferber force-pushed the cg-types-codable-conformance branch from e06b7e6 to feb3799 Compare June 19, 2017 23:01
@itaiferber
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - e06b7e6ea69e82639e7bdaece700e93a6354bae9
Test requested by - @itaiferber

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - e06b7e6ea69e82639e7bdaece700e93a6354bae9
Test requested by - @itaiferber

Add Codable conformance to common CG types
fedf8e6 
Give custom Codable implementations for CGAffineTransform, CGPoint,
CGSize, CGRect, and CGVector, along with unit tests.
@itaiferber itaiferber force-pushed the cg-types-codable-conformance branch from feb3799 to fedf8e6 Compare June 20, 2017 00:23
@itaiferber
Copy link
Contributor Author

@swift-ci Please test macOS

@itaiferber
Copy link
Contributor Author

@swift-ci Please smoke test Linux

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - feb3799016934b80e36ee56b9efef22cbb957c1d
Test requested by - @itaiferber

@itaiferber itaiferber requested review from parkera and phausler June 20, 2017 16:13
phausler
phausler approved these changes Jun 20, 2017
View reviewed changes
Copy link
Contributor

@phausler phausler left a comment

Choose a reason for hiding this comment

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

looks reasonable to me

@itaiferber itaiferber merged commit e0f75bf into swiftlang:master Jun 20, 2017
@itaiferber itaiferber mentioned this pull request Jun 20, 2017
Merged
@itaiferber itaiferber deleted the cg-types-codable-conformance branch June 20, 2017 17:11
@itaiferber itaiferber mentioned this pull request Jun 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phausler phausler phausler approved these changes

@parkera parkera Awaiting requested review from parkera

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@itaiferber @swift-ci @jrose-apple @phausler