[coreimage] Update for Xcode 11[.1] #7120

spouliot · 2019-09-26T21:37:15Z

Incomplete (draft) - it's missing a bunch of `[Obsolete]` but those are
not breaking changes so the _core_ can be reviewed.

Pay attention to naming (many conflicts) and comment if you have better
names suggestions.

It's also a big PR with a lot of copy/paste so watch out for typos!

Apple decided to expose most (but not all) CIFilter using protocols
(instead of weakly named dictionaries). Most of this maps well with
our strong bindings but there are cases where we:

missing some properties (easy, there were added); or
used a different types [1] and new members were required

[1] Often ours are better (using float for a bool is not optimal)
but we do not have [BindAs] for protocols :( to fix them

``` Incomplete (draft) - it's missing a bunch of `[Obsolete]` but those are not breaking changes so the _core_ can be reviewed. Pay attention to naming (many conflicts) and comment if you have better names suggestions. It's also a big PR with a lot of copy/paste so watch out for typos! ``` Apple decided to expose most (but not all) `CIFilter` using protocols (instead of weakly named dictionaries). Most of this maps well with our strong bindings but there are cases where we: * missing some properties (easy, there were added); or * used a different types [1] and new members were required [1] Often ours are better (using `float` for a `bool` is not optimal) but we do not have `[BindAs]` for protocols :( to _fix_ them

monojenkins · 2019-09-27T01:13:56Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
⚠️ API Diff (from PR only) (🔥 breaking changes 🔥)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 90 tests passed.

Failed tests

MTouch tests/NUnit: Failed (Execution failed with exit code 8)

src/generator-filters.cs

src/coreimage.cs

rolfbjarne · 2019-09-27T10:15:49Z

src/coreimage.cs

@@ -4646,10 +5006,13 @@ interface CIFaceBalance {

 	[iOS (9,3)]
 	[TV (9,2)]
-	[Availability (Introduced = Platform.Mac_10_10, Obsoleted = Platform.Mac_10_11)]  // FIXME: Is htis actually deprecated?  Seems to be missing in El Capitan
+//	[Availability (Introduced = Platform.Mac_10_10, Obsoleted = Platform.Mac_10_11)]  // FIXME: Is htis actually deprecated?  Seems to be missing in El Capitan


A leftover?

Testing, it seems El Capitan was an exception (where it was not working) and not a real/undocumented obsolete filter. I'll look at (old macOS) bot results

rolfbjarne · 2019-09-27T10:27:03Z

src/coreimage.cs

+
+		[Abstract]
+		[Export ("numberOfFolds")]
+		// renamed for compatibility (originally bound as an integer)


Maybe rename it back in an #if XAMCORE_4_0.

rolfbjarne · 2019-09-27T11:28:49Z

src/coreimage.cs


 		[CoreImageFilterProperty ("inputCompactStyle")]
 		bool CompactStyle { get; set; }

+		[CoreImageFilterProperty ("inputCompactStyle")]
+		float FCompactStyle { get; set; }


What about InputCompactStyle?

If not, then I think I prefer FloatCompactStyle, it's just as ugly as FCompactStyle, but more informative (and the same goes for all the other new float properties).

yeah, ideally there's a common naming fix that can be used - at least for the F and N prefixes. I'll check if Input works well on all of them :)

…ode11.1

VincentDondain

❤️ Amazing 🎉
Reviewed everything and couldn't find a typo or major issue other than the breaking changes reported by Rolf. I also very much agree with him on the FXxx naming, it makes for pretty weird APIs IMHO (FLenght). Otherwise minor comments (:

VincentDondain · 2019-09-27T14:23:48Z

tests/introspection/ApiCoreImageFiltersTest.cs

 			// e.g. picking better types like `bool` instead of `NSNumber'
 			default:
- 				return false;
+				return false;


VincentDondain · 2019-09-27T14:23:53Z

tests/introspection/ApiCoreImageFiltersTest.cs

 				// we need to skip the filters that are not supported by the executing version of iOS
 				if (Skip (t))
-					continue; 
+					continue;


VincentDondain · 2019-09-27T14:31:19Z

src/coreimage.cs

@@ -1346,6 +1382,16 @@ interface CIImage : NSSecureCoding, NSCopying {
 		[Wrap ("FromCGImage (image, options == null ? null : options.Dictionary)")]
 		CIImage FromCGImage (CGImage image, [NullAllowed] CIImageInitializationOptionsWithMetadata options);

+		[iOS (13,0)][TV (13,0)][Watch (6,0)][Mac (10,15)]
+		[Static]
+		[Export ("imageWithCGImageSource:index:options:")]


Suggested change

[Export ("imageWithCGImageSource:index:options:")]

[EditorBrowsable (EditorBrowsableState.Advanced)]

[Export ("imageWithCGImageSource:index:options:")]

I think you want to encourage people to use the strongly typed version.

VincentDondain · 2019-09-27T14:34:08Z

src/coreimage.cs


+#if !XAMCORE_4_0
+		[Obsolete ("")]


Suggested change

[Obsolete ("")]

[Obsolete ("Use 'FoldCount' instead.")]

yup, like mentioned in the PR message, obsolete are not complete in the draft :)

VincentDondain · 2019-09-27T14:38:52Z

src/coreimage.cs


+#if !XAMCORE_4_0
 		// for some reason we prefixed all Striation* with Max - API compatibility
 		[CoreImageFilterProperty ("inputStriationContrast")]


Suggested change

[CoreImageFilterProperty ("inputStriationContrast")]

[Obsolete ("Use 'StriationContrast' instead.")]

[CoreImageFilterProperty ("inputStriationContrast")]

Yea, read your description and forgot about it after 10k lines reviewed :P

VincentDondain · 2019-09-27T14:42:55Z

src/coreimage.cs

+		[NullAllowed, Export ("inputImage", ArgumentSemantic.Retain)]
+		CIImage InputImage { get; set; }
+
+		// @required @property (nonatomic) float saturation;


Forgot to remove?

… properties

monojenkins · 2019-09-27T23:47:22Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

2 tests failed, 89 tests passed.

Failed tests

Xtro/Mac: Failed (Test run failed.)
MTouch tests/NUnit: Failed (Execution failed with exit code 8)

VincentDondain · 2019-09-30T14:37:43Z

src/coreimage.cs

 		float Size { get; set; }

+#if !XAMCORE_4_0
+		[Obsolete ("Use 'InputPoint0' instead.")]


Shouldn't this be pointing to InputPoint? Or CGPoint InputPoint { get; set; } below should be InputPoint0?

…et#6938, (dotnet#7134) Add '[Appearance]' to a few appearance properties as defined in Apple's headers. Fixes dotnet#6938.

* Drop the Xcode 9.4 dependency. (dotnet#7044) * Drop the Xcode 9.4 dependency. Also bump mono to get the removal of the mac32 binaries. New commits in mono/mono: * mono/mono@beb9a1b182a [sdks] Remove the mac32 build. * mono/mono@747a919a06e [ci] Make ios/mac sdks archive URL more predictable * mono/mono@114013096e1 [ci] Build iOS/Mac Mono sdks archive using Xcode 11 * mono/mono@10a24f3ea1d Implement WriteCore and ReadCore in DeflateStream * mono/mono@a925846b1f0 [offsets-tool] Install clang into the user-specific python directory. (dotnet#16933) * mono/mono@fe64a4765e6 [2019-06] Bump msbuild and sdk versions to 3.0.1xx latest (dotnet#16870) * mono/mono@7293597b905 [corlib] Fix building nunit-lite twice (dotnet#16910) * mono/mono@1648e886873 Rename bundle identifier for the various Mono.frameworks we create for Xamarin.iOS. Fixes dotnet#7005. (dotnet#16896) * mono/mono@a6b5187d76a [metadata] Fix leaks when handling a few attributes (dotnet#16675) (dotnet#16851) * mono/mono@7da9a041b3b [2019-06] Bump to mono/corefx@e79cf5b * mono/mono@2b7050bdf36 [2019-06] Add RenamedEvent* to FSW sources from CoreFX (dotnet#16758) * mono/mono@4f5ed502c6e [msbuild] pick up p4 versions * mono/mono@f04ee2219d5 [2019-06][msbuid][roslyn] Bump msbuild and roslyn-binaries to pick up dotnet 3.0.100-p9 toolset * mono/mono@6b4b99e571b Vtable [i] can be null so this should be check before use it. Fixes dotnet#16712 Diff: https://github.com/mono/mono/compare/7af64d1ebe9e9ee305cdae8ec5995c9521cbcf19..beb9a1b182a14986e836864e5d555c3b5ec52ba0 * [tests] Add a fat macOS dylib for testing purposes. Add a binary version of a fat macOS dylib (because we can't create one when we need it since we can't create 32-bit slice anymore). It was created like this (in tests/test-libraries): $ cat test.m int theUltimateAnswer () { return 42; } $ /Applications/Xcode94.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang test.m -olibtest.i386.dylib -shared -isysroot /Applications/Xcode94.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -framework Foundation -framework CoreLocation -lz -arch i386 $ lipo -create libtest.i386.dylib .libs/macos/libtest.dylib -output libtest-fat.dylib * [tests] Adjust XM tests to XM not having fat dylibs anymore. * [tests] Adjust product tests to some libraries not being fat anymore. * [tests] Don't treat an Xcode with the same major version number as old. Fixes an issue in the MT0091 test, where it would fail on tvOS because the test wanted to use an older Xcode, and we could end up returning Xcode 11.0 when the current Xcode is 11.1. Since the test depends on using the OS SDK as it was designed for (technically using an OS SDK earlier than the latest), it ended up failing because while the iOS SDK was bumped in Xcode 11.1, the tvOS SDK was not.

Not all filters support `inputImage` but we exposed it everywhere with the `Image` property in `CIFilter`. That cause problems with filters that did not support it, e.g. crashing in the debugger, so we had to check it's existence before [get/set]ing it. That's not good for performance (now a bit optimized) and the API actually makes it harder to discover which filters really support it. This also solve _part of_ the warnings coming from the static registrar

monojenkins · 2019-10-02T01:57:31Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

13 tests failed, 78 tests passed.

Failed tests

monotouch-test/iOS Unified 32-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 32-bits - simulator/Debug (LinkSdk): Failed
monotouch-test/iOS Unified 32-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 32-bits - simulator/Release (all optimizations): Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (LinkSdk): Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed
monotouch-test/tvOS - simulator/Debug: Failed
monotouch-test/tvOS - simulator/Debug (LinkSdk): Failed
monotouch-test/tvOS - simulator/Debug (static registrar): Failed
monotouch-test/tvOS - simulator/Release (all optimizations): Failed
MTouch tests/NUnit: Failed (Execution failed with exit code 1)

spouliot · 2019-10-02T12:56:59Z

Most (12/13) failures are (or will be) fixed by #7154
or a back port of it to the xcode11.1 branch

…changed its output. Fixes xamarin/maccore#2006. (dotnet#7156) microsoft.com is doing user agent sniffing, and broke our our test since their output is now different. Switch to example.com instead. Fixes xamarin/maccore#2006.

``` Incomplete (draft) - it's missing a bunch of `[Obsolete]` but those are not breaking changes so the _core_ can be reviewed. Pay attention to naming (many conflicts) and comment if you have better names suggestions. It's also a big PR with a lot of copy/paste so watch out for typos! ``` Apple decided to expose most (but not all) `CIFilter` using protocols (instead of weakly named dictionaries). Most of this maps well with our strong bindings but there are cases where we: * missing some properties (easy, there were added); or * used a different types [1] and new members were required [1] Often ours are better (using `float` for a `bool` is not optimal) but we do not have `[BindAs]` for protocols :( to _fix_ them

… properties

Not all filters support `inputImage` but we exposed it everywhere with the `Image` property in `CIFilter`. That cause problems with filters that did not support it, e.g. crashing in the debugger, so we had to check it's existence before [get/set]ing it. That's not good for performance (now a bit optimized) and the API actually makes it harder to discover which filters really support it. This also solve _part of_ the warnings coming from the static registrar

…code11.1

monojenkins · 2019-10-02T22:39:05Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 90 tests passed.

Failed tests

MTouch tests/NUnit: Failed (Execution failed with exit code 1)

Apple decided to expose most (but not all) `CIFilter` using protocols (instead of weakly named dictionaries). Most of this maps well with our strong bindings but there are cases where we: * missing some properties (easy, there were added); or * used a different types [1] and that requires new members / obsoletion [1] Often ours are better (using `float` for a `bool` value is not optimal) but we do not have `[BindAs]` for protocols :( to _fix_ them Note: this replace draft PR dotnet#7120 but it's has quite a bit of changes in filter generation (inlining protocols) and that affected bindings too.

spouliot · 2019-10-10T19:33:04Z

Closing in favour or updated/rebased #7216

Apple decided to expose most (but not all) `CIFilter` using protocols (instead of weakly named dictionaries). Most of this maps well with our strong bindings but there are cases where we: * missing some properties (easy, there were added); or * used a different types [1] and that requires new members / obsoletion A few other API were also added in Xcode 11 (nothing in 11.1 or 11.2) and included in this PR. New introspection tests were also added to minimize the risk that the API and generator changes produced incorrect code. This lead to the finding of some missing API (in particular `Output*` properties) that were added. [1] Often ours are better (using `float` for a `bool` value is not optimal) but we do not have `[BindAs]` for protocols :( to _fix_ them Note: this replace draft PR #7120 but it's has quite a bit of changes in filter generation (inlining protocols) and that affected bindings too.

spouliot added this to the xcode11.1 milestone Sep 26, 2019

rolfbjarne reviewed Sep 27, 2019

View reviewed changes

Merge branch 'xcode11.1' of github.com:xamarin/xamarin-macios into xc…

83ce534

…ode11.1

VincentDondain approved these changes Sep 27, 2019

View reviewed changes

spouliot added 3 commits September 27, 2019 11:18

Fix indentation

c057b2d

[generator] Generated [Obsolete] (and other) attribute(s) on CIFilter…

0887b1a

… properties

Add a bunch of [Obsolete] and a few renames

5f8988c

VincentDondain reviewed Sep 30, 2019

View reviewed changes

VincentDondain approved these changes Sep 30, 2019

View reviewed changes

rolfbjarne and others added 4 commits October 1, 2019 07:21

[UIKit] Add '[Appearance]' to a few appearance properties. Fixes dotn…

3ec3470

…et#6938, (dotnet#7134) Add '[Appearance]' to a few appearance properties as defined in Apple's headers. Fixes dotnet#6938.

[SceneKit] Update for Xcode 11 (dotnet#7140)

d81f05f

monojenkins and others added 8 commits October 2, 2019 18:08

Fix indentation

f022f3f

[generator] Generated [Obsolete] (and other) attribute(s) on CIFilter…

78f5db0

… properties

Add a bunch of [Obsolete] and a few renames

671099b

Merge branch 'xcode11.1' of github.com:spouliot/xamarin-macios into x…

2ebaa85

…code11.1

Fix typo

634fe69

spouliot mentioned this pull request Oct 10, 2019

[coreimage] Update for Xcode 11[.2] #7216

Merged

spouliot closed this Oct 10, 2019

	[Export ("imageWithCGImageSource:index:options:")]
	[EditorBrowsable (EditorBrowsableState.Advanced)]
	[Export ("imageWithCGImageSource:index:options:")]

	[CoreImageFilterProperty ("inputStriationContrast")]
	[Obsolete ("Use 'StriationContrast' instead.")]
	[CoreImageFilterProperty ("inputStriationContrast")]

[coreimage] Update for Xcode 11[.1] #7120

[coreimage] Update for Xcode 11[.1] #7120

Uh oh!

Conversation

spouliot commented Sep 26, 2019

Uh oh!

monojenkins commented Sep 27, 2019

Test results

Failed tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentDondain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Sep 27, 2019

Test results

Failed tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Oct 2, 2019

Test results

Failed tests

Uh oh!

spouliot commented Oct 2, 2019

Uh oh!

monojenkins commented Oct 2, 2019

Test results

Failed tests

Uh oh!

spouliot commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants