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

[photos][photosui] Xcode 14 Beta 1-4 #15608

Merged
merged 11 commits into from
Sep 6, 2022
Merged

Conversation

chamons
Copy link
Contributor

@chamons chamons commented Aug 2, 2022

No description provided.

@chamons chamons added the notes-mention Deserves a mention in release notes label Aug 2, 2022
@chamons chamons requested a review from dalexsoto as a code owner August 2, 2022 22:38
@@ -413,4 +418,13 @@ public enum PHAccessLevel : long
AddOnly = 1,
ReadWrite = 2,
}

[TV (16,0), Mac (13,0), iOS (16,0)]
Copy link
Member

Choose a reason for hiding this comment

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

We are missing the MacCatalyst.

@@ -158,6 +158,10 @@ interface PHAsset {
[TV (15,0), Mac (12,0), iOS (15,0), MacCatalyst (15,0)]
[NullAllowed, Export ("adjustmentFormatIdentifier")]
string AdjustmentFormatIdentifier { get; }

[TV (15, 0), Mac (12, 0), iOS (15, 0)]
Copy link
Member

Choose a reason for hiding this comment

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

We are missing MacCatalyst.

@@ -269,6 +273,14 @@ interface PHAssetResource
[Static]
[Export ("assetResourcesForLivePhoto:")]
PHAssetResource[] GetAssetResources (PHLivePhoto livePhoto);

[TV (16, 0), Mac (13, 0), iOS (16, 0)]
Copy link
Member

Choose a reason for hiding this comment

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

Same.

[Export ("pixelWidth")]
nint PixelWidth { get; }

[TV (16, 0), Mac (13, 0), iOS (16, 0)]
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@@ -584,6 +596,9 @@ interface PHAssetCollection {
[Export ("fetchAssetCollectionsContainingAsset:withType:options:")]
PHFetchResult FetchAssetCollections (PHAsset asset, PHAssetCollectionType type, [NullAllowed] PHFetchOptions options);

[Deprecated (PlatformName.iOS, 16, 0, message: "Will be removed in a future release.")]
[Deprecated (PlatformName.TvOS, 16, 0, message: "Will be removed in a future release.")]
[Deprecated (PlatformName.MacOSX, 13, 0, message: "Will be removed in a future release.")]
Copy link
Member

Choose a reason for hiding this comment

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

Same, Catalyst.

src/photosui.cs Outdated
Comment on lines 488 to 491
#if NET // Can't apply Deprecated and Obsoleted to same element
[Deprecated (PlatformName.iOS, 13, 0)]
#endif
[Obsoleted (PlatformName.iOS, 14, 0)] // Removed from headers completely
Copy link
Member

Choose a reason for hiding this comment

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

I though we converted the Obsoleted to Deprecated in the generator, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do, and then you get 2 copies of the attribute, one which over writes the second (whoever comes last). I have a test explicitly to detect this.

@@ -524,9 +530,17 @@ interface PHPickerViewController
[Export ("initWithConfiguration:")]
[DesignatedInitializer]
NativeHandle Constructor (PHPickerConfiguration configuration);

[NoWatch, NoTV, Mac (13,0), iOS (16,0)]
Copy link
Member

Choose a reason for hiding this comment

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

Catalyst?

[Export ("deselectAssetsWithIdentifiers:")]
void DeselectAssets (string[] identifiers);

[NoWatch, NoTV, Mac (13,0), iOS (16,0)]
Copy link
Member

Choose a reason for hiding this comment

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

Catalyst.

Comment on lines +592 to +635

[NoWatch, NoTV, Mac (13, 0), iOS (16, 0)]
[Static]
[Export ("depthEffectPhotosFilter")]
PHPickerFilter DepthEffectPhotosFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (16, 0)]
[Static]
[Export ("burstsFilter")]
PHPickerFilter BurstsFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (15, 0)]
[Static]
[Export ("panoramasFilter")]
PHPickerFilter PanoramasFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (15, 0)]
[Static]
[Export ("screenshotsFilter")]
PHPickerFilter ScreenshotsFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (15, 0)]
[Static]
[Export ("screenRecordingsFilter")]
PHPickerFilter ScreenRecordingsFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (16, 0)]
[Static]
[Export ("cinematicVideosFilter")]
PHPickerFilter CinematicVideosFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (15, 0)]
[Static]
[Export ("slomoVideosFilter")]
PHPickerFilter SlomoVideosFilter { get; }

[NoWatch, NoTV, Mac (13, 0), iOS (15, 0)]
[Static]
[Export ("timelapseVideosFilter")]
PHPickerFilter TimelapseVideosFilter { get; }

[NoWatch, NoTV, Mac (13,0), iOS (15,0)]
[Static]
[Export ("playbackStyleFilter:")]
Copy link
Member

Choose a reason for hiding this comment

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

Catalyst is missing.

Comment on lines +638 to +646
[NoWatch, NoTV, Mac (13,0), iOS (15,0)]
[Static]
[Export ("allFilterMatchingSubfilters:")]
PHPickerFilter GetAllFilterMatchingSubfilters (PHPickerFilter[] subfilters);

[NoWatch, NoTV, Mac (13,0), iOS (15,0)]
[Static]
[Export ("notFilterOfSubfilter:")]
PHPickerFilter GetNotFilterOfSubfilter (PHPickerFilter subfilter);
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@mandel-macaque mandel-macaque added this to the xcode14 milestone Aug 2, 2022
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

After @mandel-macaque 👍 and Api Diff is green

src/Photos/Enums.cs Outdated Show resolved Hide resolved
@vs-mobiletools-engineering-service2

This comment has been minimized.

Co-authored-by: Alex Soto <alex@alexsoto.me>
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

src/photos.cs Outdated Show resolved Hide resolved
src/photos.cs Outdated Show resolved Hide resolved
src/photosui.cs Outdated

[TV (10,0), iOS (9,1), NoMac]
[Export ("livePhotoView:extraMinimumTouchDurationForTouch:withStyle:")]
double ExtraMinimumTouchDuration (PHLivePhotoView livePhotoView, UITouch touch, PHLivePhotoViewPlaybackStyle playbackStyle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe verb-ify this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions welcome on a verb here. You are providing the extra min touch duration.

ProvideExtraMinimumTouchDuration was the best I could come up with, but that's a large delta and I didn't love that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why GetExtraMinimumTouchDuration doesn't work?

@@ -0,0 +1,2 @@
# Removed in Xcode 14 from header
!unknown-type! PHEditingExtensionContext bound
Copy link
Contributor

Choose a reason for hiding this comment

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

nit missing newline

@@ -0,0 +1,2 @@
# Removed in Xcode 14 from header
!unknown-type! PHEditingExtensionContext bound
Copy link
Contributor

Choose a reason for hiding this comment

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

nit missing newline

chamons and others added 2 commits August 3, 2022 17:12
Co-authored-by: TJ Lambert <50846373+tj-devel709@users.noreply.github.com>
Co-authored-by: TJ Lambert <50846373+tj-devel709@users.noreply.github.com>
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@chamons
Copy link
Contributor Author

chamons commented Aug 8, 2022

Blocked now on #15659

@chamons
Copy link
Contributor Author

chamons commented Aug 8, 2022

🤞 that'll fix the test failures.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

janwiebe-jump pushed a commit to janwiebe-jump/xamarin-macios that referenced this pull request Aug 9, 2022
…5648)

- In the [Xcode 14 Photo PR](xamarin#15608) a test is failing with this:

```
ILLINK : error MT2362: The linker step 'Registrar' failed during processing: One or more errors occurred. (The type 'Photos.PHPersistentObjectChangeDetails' (used as a return type in Photos.PHPersistentChange.ChangeDetails) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode). [/Users/donblas/Programming/xamarin-macios/tests/dotnet/MySimpleApp/MacCatalyst/MySimpleApp.csproj]
  		) (The type 'Photos.PHObjectType' (used as a parameter in Photos.PHPersistentChange.ChangeDetails) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode).
  		) (The type 'Photos.PHPersistentChangeFetchResult' (used as a return type in Photos.PHPhotoLibrary.FetchPersistentChanges) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode).
  		) (The type 'Photos.PHPersistentChangeToken' (used as a parameter in Photos.PHPhotoLibrary.FetchPersistentChanges) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode).
```

The details of how we fail are written up in [this issue](xamarin#15643) but since sharpie never outputs versions in the form of x.y.z where .z is zero we only hit this with generated attributes.

Because of this fact, we can work around it with a generator change.

This commit changes how we "imply" attributes from iOS to Catalyst. As a brief reminder, because of historical bindings we assume anything that has iOS and not a Catalyst really means "treat iOS as if it was also Catalyst".

This work is done in `AddImpliedCatalyst` and uses `CloneFromOtherPlatform` to make a copy of an attribute, because there is no easy way to say "I want a copy of this, but with this other platform". `CloneFromOtherPlatform` used to always call the 3 version (Major, Minor, Revision) constructor, even when the attribute being cloned only used Major.Minor.

However, this caused us to "zero extend" the version with another zero, which triggers this bug, so stop doing that. Uglier code in the generator, but it works better.

Co-authored-by: Manuel de la Pena <mandel@microsoft.com>
src/photos.cs Outdated Show resolved Hide resolved
src/photos.cs Outdated Show resolved Hide resolved
src/photosui.cs Outdated

[TV (10,0), iOS (9,1), NoMac]
[Export ("livePhotoView:extraMinimumTouchDurationForTouch:withStyle:")]
double ExtraMinimumTouchDuration (PHLivePhotoView livePhotoView, UITouch touch, PHLivePhotoViewPlaybackStyle playbackStyle);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why GetExtraMinimumTouchDuration doesn't work?

spouliot pushed a commit to spouliot/xamarin-macios that referenced this pull request Aug 19, 2022
…5659)

- In the [Xcode 14 Photo PR](xamarin#15608) a test is failing with this:

```
ILLINK : error MT2362: The linker step 'Registrar' failed during processing: One or more errors occurred. (The type 'Photos.PHPersistentObjectChangeDetails' (used as a return type in Photos.PHPersistentChange.ChangeDetails) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode). [/Users/donblas/Programming/xamarin-macios/tests/dotnet/MySimpleApp/MacCatalyst/MySimpleApp.csproj]
  		) (The type 'Photos.PHObjectType' (used as a parameter in Photos.PHPersistentChange.ChangeDetails) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode).
  		) (The type 'Photos.PHPersistentChangeFetchResult' (used as a return type in Photos.PHPhotoLibrary.FetchPersistentChanges) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode).
  		) (The type 'Photos.PHPersistentChangeToken' (used as a parameter in Photos.PHPhotoLibrary.FetchPersistentChanges) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode).
```

The details of how we fail are written up in [this issue](xamarin#15643) but since sharpie never outputs versions in the form of x.y.z where .z is zero we only hit this with generated attributes.

Because of this fact, we can work around it with a generator change.

This commit changes how we "imply" attributes from iOS to Catalyst. As a brief reminder, because of historical bindings we assume anything that has iOS and not a Catalyst really means "treat iOS as if it was also Catalyst".

This work is done in `AddImpliedCatalyst` and uses `CloneFromOtherPlatform` to make a copy of an attribute, because there is no easy way to say "I want a copy of this, but with this other platform". `CloneFromOtherPlatform` used to always call the 3 version (Major, Minor, Revision) constructor, even when the attribute being cloned only used Major.Minor.

However, this caused us to "zero extend" the version with another zero, which triggers this bug, so stop doing that. Uglier code in the generator, but it works better.

Co-authored-by: Chris Hamons <chris.hamons@xamarin.com>
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 40f0de9bf5c8c51fa55b229ee65f5ae91dac7c3e [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1173.Monterey'
Hash: 40f0de9bf5c8c51fa55b229ee65f5ae91dac7c3e [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

  • introspection
  • xammac_tests
  • monotouch-test

Pipeline on Agent
Hash: 40f0de9bf5c8c51fa55b229ee65f5ae91dac7c3e [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

0 tests crashed, 1 tests failed, 227 tests passed.

Failures

❌ introspection tests

1 tests failed, 12 tests passed.
  • introspection/watchOS 32-bits - simulator/Debug (watchOS 6.0): Crashed Known issue: HE0038)

Html Report (VSDrops) Download

Successes

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

@chamons
Copy link
Contributor Author

chamons commented Sep 2, 2022

Last failure is just watch.

@chamons chamons merged commit 2f6b916 into xamarin:xcode14 Sep 6, 2022
@chamons chamons deleted the xcode14_photo branch September 6, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Do not merge this pull request notes-mention Deserves a mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants