Skip to content
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

[PhotosUI] Xcode13 PhotosUI beta1 #11945

Merged
merged 6 commits into from
Jun 22, 2021

Conversation

tj-devel709
Copy link
Contributor

I wasn't quite sure if I should create an enums file for this one enum introduced to the file?

@tj-devel709 tj-devel709 added the note-highlight Worth calling out specifically in release notes label Jun 15, 2021
@tj-devel709 tj-devel709 added this to the xcode13.0 milestone Jun 15, 2021
@spouliot
Copy link
Contributor

I wasn't quite sure if I should create an enums file for this one enum introduced to the file?

Add new enums into existing binding files. That way the generator will process them. That's useful in many cases, including automatic conversion of availability attributes for net6.

src/photosui.cs Outdated Show resolved Hide resolved
src/photosui.cs Outdated Show resolved Hide resolved
@@ -1,10 +0,0 @@
!missing-protocol! PHContentEditingController not bound
!missing-type! PHEditingExtensionContext not bound
## appended from unclassified file
Copy link
Contributor

Choose a reason for hiding this comment

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

^ hint ^ everything above this line is part of the backlog and needs a bit more validation

src/photosui.cs Outdated Show resolved Hide resolved
src/photosui.cs Outdated Show resolved Hide resolved
@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 85 tests passed.

Failed tests

  • introspection/watchOS 32-bits - simulator/Debug (watchOS 5.0): Failed

Pipeline on Agent XAMBOT-1100.BigSur'
Merge 4283139 into 170ea55

src/photosui.cs Outdated Show resolved Hide resolved
src/photosui.cs Outdated Show resolved Hide resolved
src/photosui.cs Outdated Show resolved Hide resolved
@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 85 tests passed.

Failed tests

  • introspection/watchOS 32-bits - simulator/Debug (watchOS 5.0): Failed

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 50f9163 into e19591f

src/photosui.cs Outdated
[Protocol]
[Unavailable (PlatformName.MacCatalyst)]
[Advice ("This API is not available when using UIKit on macOS.")]
Copy link
Member

Choose a reason for hiding this comment

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

You need to remove this advice too if you remove [Unavailable (PlatformName.MacCatalyst)]

Suggested change
[Advice ("This API is not available when using UIKit on macOS.")]

src/photosui.cs Outdated
@@ -474,7 +478,6 @@ interface PHProjectTypeDescriptionInvalidator
[iOS (8,0)]
[NoMac][NoTV]
[DisableDefaultCtor]
[Unavailable (PlatformName.MacCatalyst)]
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct, the PHEditingExtensionContext type doesn't show up for Mac Catalyst: https://developer.apple.com/documentation/photokit/pheditingextensioncontext?language=objc

If it turns out it actually is available (the documentation is wrong), then you need to remove the advice on the next line too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, you are right! I had added the MacCatalyst there and since removed it, but forgot to add that Unavailable back in!

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 85 tests passed.

Failed tests

  • introspection/watchOS 32-bits - simulator/Debug (watchOS 5.0): Failed

Pipeline on Agent XAMBOT-1096.BigSur'
Merge 4ea0a75 into 242b889


[iOS (15,0), MacCatalyst (15,0)]
[Export ("presentLimitedLibraryPickerFromViewController:completionHandler:")]
void PresentLimitedLibraryPicker (UIViewController controller, Action<string[]> completionHandler);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The new swift async/await makes it easy to know, from the docs:

You can call this method from synchronous code using a completion handler, as shown on this page, or you can call it as an asynchronous method that has the following declaration:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good call, are most completionHandlers a good indicator of an async method?

Copy link
Member

Choose a reason for hiding this comment

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

Most of the time, the new docs for swift make it easier to know. Intro will complain sometimes if it has the info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, but forgot to run the tests, I'll do that now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay they look like they are passing!

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

🎉 All 86 tests passed 🎉

Pipeline on Agent XAMBOT-1098.BigSur'
Merge fcf914a into c778e6b

@tj-devel709 tj-devel709 merged commit 621aaef into xamarin:main Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
note-highlight Worth calling out specifically in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants