-
Notifications
You must be signed in to change notification settings - Fork 517
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
[QuickLook] Update bindings for Xcode 13.0 beta 1,2,3,4 #12337
[QuickLook] Update bindings for Xcode 13.0 beta 1,2,3,4 #12337
Conversation
nint InitialPreviewIndex { get; set; } | ||
} | ||
|
||
// TODO: BaseType UIWindowSceneActivationConfiguration must first be implemented in UIKit |
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.
[QuickLook] QLPreviewSceneActivationConfiguration not yet bound in Xcode13 because base type UIWindowSceneActivationConfiguration is not yet bound in UIKit
{ | ||
} | ||
|
||
[iOS (15,0), MacCatalyst (15,0)] |
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.
Mac ?
without a [Mac]
it means the oldest supported (10,9) has it available.
Same for watchOS and tvOS. You likely want [NoWatch, NoTV, NoMac]
if it's not available on macOS.
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 there's no indication on the diffs, xtro, web docs, or header files - I tend to leave the availability of unmentioned platforms as is. But is it best to assume that there is no availability on those platforms? Since it's a breaking change to remove things, and it's always easier to introduce things later 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.
But is it best to assume that there is no availability on those platforms?
No. Lack of availability attributes means it's been available since our earliest supported versions.
E.g. lack of [NoWatch]
implies it's [Watch (2,0)]
and that the API has been available in the past 6 years (or so)
This is almost always wrong when adding bindings for a new Xcode (unless it added a new platform).
Since it's a breaking change to remove things, and it's always easier to introduce things later on?
True :-) However it's true for code that we ship (post generation), not the binding sources!
Example: [NoWatch]
-> means we're not generating the API on watchOS.
If you don't add [NoWatch]
(in binding sources) then the API will be present inside Xamarin.WatchOS.dll
(and assumed to be available since 2.0 which is watchOS minimum supported version).
If you want to fix that later, by adding [NoWatch]
, then you are creating a breaking change (since the API won't be part of Xamarin.WatchOS.dll
in the next release).
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.
Ah, I think my question was a bit confusing! I understand what you explained about the lack of availability attributes in our code - I meant instead to ask about how we should interpret the lack of mention of platform availability in diffs, xtro, web docs, and header files.
So in this case, there is no Watch availability or unavailability mentioned whatsoever in the diffs, xtro, web docs, or header files for this API. So I was asking if that means it's best to assume there's no watch availability and tag as [NoWatch] - does that make sense?
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.
Short answer: Whenever in doubt it's better to assume [No*]
if availability is not mentioned because it means no code will be generated (and it won't be a breaking change to add it later).
When you add (or modify) sharpie's output then it's good to add comments (for reviewers and people who will update the bindings... years later), e.g.
[NoWatch][NoTV][NoMac] // availability not mentioned in the header files
Long answer
- Apple's macro are often incomplete or incorrect (e.g. wrong version)
- Some (not all) gets fixed in newer betas or the next Xcode...
- There are assumptions like
- old (pre 9.0) iOS API are assumed to be available on tvOS 9 (first version) unless decorated otherwise
- old (pre 13) iOS API are assumed to be available on MacCatalyst 13 (first version) ...
- enums are not always decorated since they are not API (you can see which API uses them to know which attributes are needed)
- Intro (runtime) and xtro (build time) will spot many, but not all, errors
- over time you get to know the API/framework and can identify things that looks unusual (and add tests to confirm your gut feelings)
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.
perfect, thank you!!!
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.
Nice!!
src/quicklook.cs
Outdated
interface QLFilePreviewRequest | ||
{ | ||
[Export ("fileURL")] | ||
NSUrl FileURL { get; } |
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.
NSUrl FileURL { get; } | |
NSUrl FileUrl { get; } |
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.
Bravo, approving taking into account sebs comments.
❌ [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, 91 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094.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, 91 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
@rachelkang it works locally for me and, if it was related to the generator change it should have failed in the non-botnet execution too. It would also have failed on the previous commits. Logs shows
which is not in your code (but the test code) so it looks like a very uncommon random failure. I file it inside You proceed with the merge! |
awesome, thank you!!! |
Updates to this PR do not include updates relevant to QuickLookThumbnailing, QuickLookUI
Generator fix courtesy of @spouliot
[QuickLook] QLPreviewSceneActivationConfiguration not yet bound in Xcode13