Skip to content

[Accessibility] Update bindings for Xcode 13.0 beta 1 - #11998

Merged
mandel-macaque merged 2 commits into
dotnet:mainfrom
rachelkang:xcode13.0-accessibility-b1
Jul 7, 2021
Merged

[Accessibility] Update bindings for Xcode 13.0 beta 1#11998
mandel-macaque merged 2 commits into
dotnet:mainfrom
rachelkang:xcode13.0-accessibility-b1

Conversation

@rachelkang

Copy link
Copy Markdown
Member

No description provided.

@rachelkang rachelkang added the note-highlight Worth calling out specifically in release notes label Jun 21, 2021
@rachelkang rachelkang added this to the xcode13.0 milestone Jun 21, 2021
@rachelkang
rachelkang requested review from dalexsoto and spouliot June 21, 2021 22:36

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

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 :)

Comment thread src/Accessibility/AXHearingUtilities.cs Outdated
Comment thread src/accessibility.cs Outdated

[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)]
[Protocol]
[BaseType (typeof(NSObject))]

Copy link
Copy Markdown
Contributor

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]

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

gotcha, thanks for clarifying!

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

Comment thread src/accessibility.cs Outdated
Comment thread src/accessibility.cs Outdated
Comment thread src/accessibility.cs
Comment thread src/accessibility.cs Outdated
Comment thread src/accessibility.cs
}

[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)]
[BaseType (typeof(NSObject))]

Copy link
Copy Markdown
Contributor

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need BaseType?

Comment thread src/accessibility.cs Outdated
Comment thread src/accessibility.cs Outdated
Comment thread src/accessibility.cs Outdated
@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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

8 tests failed, 78 tests passed.

Failed tests

  • monotouch-test/Mac Catalyst/Debug [dotnet]: Failed (Tests run: 2622 Passed: 2489 Inconclusive: 35 Failed: 1 Ignored: 132)
  • introspection/Mac Catalyst/Debug: Failed (Tests run: 44 Passed: 31 Inconclusive: 0 Failed: 8 Ignored: 5)
  • introspection/iOS Unified 64-bits - simulator/Debug: Failed
  • introspection/tvOS - simulator/Debug: Failed
  • introspection/watchOS 32-bits - simulator/Debug: Failed
  • introspection/iOS Unified 64-bits - simulator/Debug (iOS 11.4): Failed
  • introspection/watchOS 32-bits - simulator/Debug (watchOS 6.0): Failed
  • link sdk/Mac Catalyst/Debug: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)

Pipeline on Agent XAMBOT-1101.BigSur'
Merge 85e649e into 6ce5f80

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

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).

Comment thread src/Accessibility/AXHearingUtilities.cs Outdated
Comment thread src/Accessibility/AXHearingUtilities.cs Outdated
{
return NSArray.ArrayFromHandle<NSUuid> (AXMFiHearingDevicePairedUUIDs ());
}
}

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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 😄

Copy link
Copy Markdown
Member Author

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!

Comment thread src/accessibility.cs Outdated

[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)]
[Protocol]
[BaseType (typeof(NSObject))]

Copy link
Copy Markdown
Contributor

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.

Comment thread src/accessibility.cs
Comment thread src/accessibility.cs
Comment thread src/accessibility.cs
Comment thread src/accessibility.cs
Comment thread src/accessibility.cs
}

[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0)]
[BaseType (typeof(NSObject))]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need BaseType?

Comment thread src/accessibility.cs Outdated
double UpperBound { get; set; }

[Export ("valueDescriptionProvider", ArgumentSemantic.Copy)]
Func<double, NSString> ValueDescriptionProvider { get; set; }

Copy link
Copy Markdown
Contributor

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.

@rachelkang rachelkang Jun 22, 2021

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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.

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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

2 tests failed, 84 tests passed.

Failed tests

  • introspection/Mac Catalyst/Debug: Failed (Tests run: 44 Passed: 34 Inconclusive: 0 Failed: 5 Ignored: 5)
  • monotouch-test/iOS Unified 64-bits - simulator/Debug (LinkSdk): Failed

Pipeline on Agent XAMBOT-1104.BigSur'
Merge ebc3918 into 6898920

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

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

@mandel-macaque
mandel-macaque marked this pull request as ready for review June 23, 2021 02:36
using Foundation;
using Accessibility;
using NUnit.Framework;
using MonoTests.System.Net.Http;

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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

Comment thread src/Accessibility/AXHearingUtilities.cs Outdated
{
return NSArray.ArrayFromHandle<NSUuid> (AXMFiHearingDevicePairedUUIDs ());
}
}

Copy link
Copy Markdown
Member

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 😄

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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/Mac Catalyst/Debug: Failed (Tests run: 44 Passed: 34 Inconclusive: 0 Failed: 5 Ignored: 5)

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 0f5b82c into c4d5433

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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/Mac Catalyst/Debug: Failed (Tests run: 44 Passed: 34 Inconclusive: 0 Failed: 5 Ignored: 5)

Pipeline on Agent XAMBOT-1101.BigSur'
Merge 6ee7efb into 4914136

@rachelkang
rachelkang requested a review from emaf as a code owner June 29, 2021 20:56
@rachelkang
rachelkang removed the request for review from emaf June 29, 2021 20:59

@dalexsoto dalexsoto left a comment

Copy link
Copy Markdown
Member

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 :(

@rachelkang

Copy link
Copy Markdown
Member Author

@dalexsoto yes :/ there was a bad merge - @mandel-macaque is helping me fix it though 😅

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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-1100.BigSur'
Merge f058918 into a64b030

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
Co-authored-by: Rachel Kang <rachelkang@microsoft.com>
@mandel-macaque
mandel-macaque force-pushed the xcode13.0-accessibility-b1 branch from f058918 to eca53e5 Compare June 30, 2021 14:58
@rachelkang

rachelkang commented Jun 30, 2021

Copy link
Copy Markdown
Member Author

intro and xtro are all green and happy :)

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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

  • link sdk/Mac Catalyst/Debug: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)

Pipeline on Agent XAMBOT-1101.BigSur'
Merge eca53e5 into 801063d

@tj-devel709

Copy link
Copy Markdown
Member

The one failing test seems to be unrelated: https://github.com/xamarin/maccore/issues/2434

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
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-1101.BigSur'
Merge 20a1503 into 3b6f059

@rachelkang
rachelkang requested a review from dalexsoto July 7, 2021 17:16
@mandel-macaque
mandel-macaque merged commit 9ca0942 into dotnet:main Jul 7, 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.

7 participants