-
Notifications
You must be signed in to change notification settings - Fork 486
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.
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
Added GPSLatitudeRef and GPSLongitudeRef to CGImagePropertiesGPS. #17166
Added GPSLatitudeRef and GPSLongitudeRef to CGImagePropertiesGPS. #17166
Conversation
I think the types for LatitudeRef & LongitudeRef should be string and not float, right? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured I would take a stab at this. Since these are properties and the underlying functions are tested in DictionaryPickerTest, I didn't write any tests. I can if that is desired though.
I think a test would be a good idea, because:
Note: There is a TODO section that implies that more properties should be brought into the class... is that something I should add in as well?
Yes, that would be nice. Note that the problem is figuring out which type to expose (neither Apple's documentation nor headers seem to be particularly useful) - which is why a test would be useful.
The same can also be said for the other custom dictionaries in this file (CGImagePropertiesExif, CGImagePropertiesTiff, CGImagePropertiesJfif, CGImagePropertiesPng, CGImagePropertiesIptc)
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 as long as @rolfbjarne comments are taken into account and at test is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, +1 on the test
|
I added a test for the properties. Should these additional properties be assigned their own separate issue? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Yes, that's probably the simplest. |
This comment has been minimized.
This comment has been minimized.
c861282
to
94423d7
Compare
|
Additional properties have been assigned their own issue: #17315 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a8dfe74
to
e0a999e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e0a999e
to
115c588
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…as still linking to non-loc version of image file. Found some more instances where new file needs to be added.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…thin dotnet tests.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
🔥 Failed to compute test summaries on VSTS: simulator tests 🔥Failed to compute test summaries:
. |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 225 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Fixes #17162
Figured I would take a stab at this. Since these are properties and the underlying functions are tested in DictionaryPickerTest, I didn't write any tests. I can if that is desired though.
Note: There is a TODO section that implies that more properties should be brought into the class... is that something I should add in as well?