-
Notifications
You must be signed in to change notification settings - Fork 514
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
[Accessibility] Update bindings for Xcode 13.0 beta 1 #11998
[Accessibility] Update bindings for Xcode 13.0 beta 1 #11998
Conversation
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.
needs a few changes, mostly minor :)
src/accessibility.cs
Outdated
|
||
[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)] | ||
[Protocol] | ||
[BaseType (typeof(NSObject))] |
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.
no [BaseType]
unless there's a [Model]
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.
By "unless there's a [Model]", do you mean unless the class inherits from another class, or unless the class literally has the attribute [Model]
? And how do we know when the tag is needed?
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.
He means the attribute, since Model will create an instance.
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.
The [Model]
attribute, you only add [Model]
, [BaseType]
and [Protocol]
to types with a suffix Delegate
or DataSource
so FooDelegate
and FooDataSource
will need the 3 of them any other @protocol
just needs the [Protocol]
attribute.
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.
gotcha, thanks for clarifying!
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.
Should I also remove the attribute from existing types in the file, or just let them be?
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.
I may be wrong, but I think we try to leave things already there alone unless it causes an issue
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.
Do not remove existing attributes since it will be a breaking change
} | ||
|
||
[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)] | ||
[BaseType (typeof(NSObject))] |
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.
since it's only static
members you should add [DisableDefaultCtor]
since creating an instance of this type is not helpful (and can be confusing)
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.
Do we need BaseType?
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results8 tests failed, 78 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
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.
This looks good!
Just a few minor comments, and also the manual API you added (in AXHearingUtilities.cs) needs manual tests (in monotouch-test).
{ | ||
return NSArray.ArrayFromHandle<NSUuid> (AXMFiHearingDevicePairedUUIDs ()); | ||
} | ||
} |
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.
It looks like you've used spaces for indentation - we use tabs.
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.
This is still the case 😄
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.
:( so strange... I'll check through everything again!
src/accessibility.cs
Outdated
|
||
[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)] | ||
[Protocol] | ||
[BaseType (typeof(NSObject))] |
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.
He means the attribute, since Model will create an instance.
} | ||
|
||
[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)] | ||
[BaseType (typeof(NSObject))] |
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.
Do we need BaseType?
src/accessibility.cs
Outdated
double UpperBound { get; set; } | ||
|
||
[Export ("valueDescriptionProvider", ArgumentSemantic.Copy)] | ||
Func<double, NSString> ValueDescriptionProvider { get; set; } |
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.
Maybe a delegate will be more helpful for the developers to understand what is going on.
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.
If I'm understanding correctly, would that be something like this?
delegate double AXDataValue;
delegate NSString AXDataValueDescription;
[Export ("valueDescriptionProvider", ArgumentSemantic.Copy)]
Func<AXDataValue, AXDataValueDescription> ValueDescriptionProvider { get; set; }
if not, what were you thinking? what am I missing?
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.
No, more replacing Func
with a delegate
type. The later are self-documenting.
The more arguments the more important it becomes because Func<byte,short,int,long>
has to meaning (without reading the docs). However a delegate type like delegate long Hash (byte b, short s, int i);
describe what the function is expected to do.
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.
Something like:
delegate AXDataValueDescription ValueDescriptionProviderHandler (AXDataValue dataValue);
To be honest, the headers do not help much.
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results2 tests failed, 84 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
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.
pending requested tests and green / no related failures from bots
using Foundation; | ||
using Accessibility; | ||
using NUnit.Framework; | ||
using MonoTests.System.Net.Http; |
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.
Why do you need this using
?
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.
I don't :p poor copy/paste! removing
{ | ||
return NSArray.ArrayFromHandle<NSUuid> (AXMFiHearingDevicePairedUUIDs ()); | ||
} | ||
} |
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.
This is still the case 😄
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 85 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 85 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
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.
Ouch looks like you will need to fix your PR :(
@dalexsoto yes :/ there was a bad merge - @mandel-macaque is helping me fix it though 😅 |
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 86 tests passed 🎉Pipeline on Agent XAMBOT-1100.BigSur' |
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com> Co-authored-by: Rachel Kang <rachelkang@microsoft.com>
f058918
to
eca53e5
Compare
intro and xtro are all green and happy :) |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 85 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
The one failing test seems to be unrelated: https://github.com/xamarin/maccore/issues/2434 |
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 86 tests passed 🎉Pipeline on Agent XAMBOT-1101.BigSur' |
No description provided.