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

[AppKit] Xcode 12.0 Beta 1 #9106

Merged
merged 7 commits into from
Jul 17, 2020
Merged

Conversation

chamons
Copy link
Contributor

@chamons chamons commented Jul 16, 2020

No description provided.

Small = 1,
Mini = 2,
[Mac (10,16)]
Large = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: add a , so any future addition won't make the last value part of the diff

Automatic,
FullWidth,
Inset,
SourceList
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: add a , so any future addition won't make the last value part of the diff

Automatic,
None,
Line,
Shadow
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: add a , so any future addition won't make the last value part of the diff

Expanded,
Preference,
Unified,
UnifiedCompact
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: add a , so any future addition won't make the last value part of the diff

SlideUp = 0x10,
SlideDown = 0x20,
SlideLeft = 0x30,
SlideRight = 0x40
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: add a , so any future addition won't make the last value part of the diff

src/appkit.cs Outdated
NSTextContentTypePassword,

[Field ("NSTextContentTypeOneTimeCode")]
NSTextContentTypeOneTimeCode,
Copy link
Contributor

Choose a reason for hiding this comment

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

same

src/appkit.cs Outdated

[Static]
[Export ("tintConfigurationWithPreferredColor:")]
NSTintConfiguration TintConfigurationWithPreferredColor (NSColor color);
Copy link
Contributor

Choose a reason for hiding this comment

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

no verb -> CreateWithPreferredColor

src/appkit.cs Outdated

[Static]
[Export ("tintConfigurationWithFixedColor:")]
NSTintConfiguration TintConfigurationWithFixedColor (NSColor color);
Copy link
Contributor

Choose a reason for hiding this comment

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

no verb -> CreateWithFixedColor

src/appkit.cs Show resolved Hide resolved
src/appkit.cs Show resolved Hide resolved
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.

Some changes required

src/AppKit/Enums.cs Outdated Show resolved Hide resolved
Automatic,
FullWidth,
Inset,
SourceList
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SourceList
SourceList,

Automatic,
None,
Line,
Shadow
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Shadow
Shadow,

Expanded,
Preference,
Unified,
UnifiedCompact
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
UnifiedCompact
UnifiedCompact,

SlideUp = 0x10,
SlideDown = 0x20,
SlideLeft = 0x30,
SlideRight = 0x40
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SlideRight = 0x40
SlideRight = 0x40,

src/appkit.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@whitneyschmidt whitneyschmidt left a comment

Choose a reason for hiding this comment

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

LGTM after Sebastien suggestions :)

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Just a few minor issues.

src/appkit.cs Outdated
NSColor Knob { get; }

[Static]
[Export ("selectedKnobColor")]
[Advice ("Use 'NSScroller' instead")]
[Deprecated (PlatformName.MacOSX, 10, 16, message: "Use 'NSScroller' instead")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Deprecated (PlatformName.MacOSX, 10, 16, message: "Use 'NSScroller' instead")]
[Deprecated (PlatformName.MacOSX, 10, 16, message: "Use 'NSScroller' instead.")]

src/appkit.cs Outdated
[Static]
[Export ("imageWithSystemSymbolName:accessibilityDescription:")]
[return: NullAllowed]
NSImage GetSystemSymbol (string symbolName, [NullAllowed] string description);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
NSImage GetSystemSymbol (string symbolName, [NullAllowed] string description);
NSImage GetSystemSymbol (string symbolName, [NullAllowed] string accessibilityDescription);

src/appkit.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated
void ApplySnapshot (NSDiffableDataSourceSnapshot<NSObject, NSObject> snapshot, bool animatingDifferences);

[Export ("applySnapshot:animatingDifferences:completion:")]
void ApplySnapshot (NSDiffableDataSourceSnapshot<NSObject, NSObject> snapshot, bool animatingDifferences, [NullAllowed] Action completion);
Copy link
Member

Choose a reason for hiding this comment

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

+[Async]?


[Flags, Mac (10,16)]
[Native]
public enum NSTableViewAnimationOptions : ulong
Copy link
Contributor

Choose a reason for hiding this comment

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

weird, it's not in the diff
it's used but not defined, maybe it's an older type ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
Co-authored-by: Alex Soto <[email protected]>
@monojenkins
Copy link
Collaborator

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

@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

🔥 Build failed 🔥

src/appkit.cs Show resolved Hide resolved
src/appkit.cs Outdated
[Mac (10,16)]
[BaseType (typeof(NSObject))]
[DisableDefaultCtor]
interface NSTableViewDiffableDataSource : NSTableViewDataSource
Copy link
Contributor

Choose a reason for hiding this comment

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

that does not match the native side

@interface NSTableViewDiffableDataSource<SectionIdentifierType,ItemIdentifierType> : NSObject<NSTableViewDataSource>

and that's a similar issue to what we had in Foundation (and UIKit) that requires generator changes (iirc from last year)

src/appkit.cs Outdated
IntPtr Constructor (NSTableView tableView, NSTableViewDiffableDataSourceCellProvider cellProvider);

[Export ("snapshot")]
NSDiffableDataSourceSnapshot<NSObject, NSObject> Snapshot ();
Copy link
Contributor

Choose a reason for hiding this comment

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

NSObject are wrong since it should be identical to the generic used on the type itself

@monojenkins
Copy link
Collaborator

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, 92 tests passed.

Failed tests

  • introspection/Mac Modern/Debug: Failed (Test run failed.)

src/AppKit/NSWorkspace.cs Outdated Show resolved Hide resolved
src/AppKit/NSWorkspace.cs Outdated Show resolved Hide resolved
src/appkit.cs Outdated Show resolved Hide resolved
[Static]
[Export ("imageWithSystemSymbolName:accessibilityDescription:")]
[return: NullAllowed]
NSImage GetSystemSymbol (string symbolName, [NullAllowed] string accessibilityDescription);
Copy link
Contributor

Choose a reason for hiding this comment

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

GetSystemSymbolImage since we're returning an NSImage?

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 are inside NSImage already so it's NSImage.GetSystemSymbol()

@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

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

@monojenkins
Copy link
Collaborator

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, 92 tests passed.

Failed tests

  • introspection/Mac Modern/Debug: Failed (Test run failed.)

@spouliot
Copy link
Contributor

8:49:40.1399010 [FAIL] 10.7 < 10.9 (Min) on 'AppKit.NSTableViewAnimationOptions'.

we don't add availability when older than our minimum supported version

@chamons
Copy link
Contributor Author

chamons commented Jul 17, 2020

I remember when 10.7 was supported... Fixing.

@monojenkins
Copy link
Collaborator

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

@chamons chamons merged commit fbcbdcd into xamarin:xcode12 Jul 17, 2020
@chamons chamons deleted the xcode12-appkit-b1 branch July 17, 2020 20:19
@chamons chamons added the notes-mention Deserves a mention in release notes label Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-mention Deserves a mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants