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

[PDFKit] Updating Xcode13 Beta 1 #11987

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

tj-devel709
Copy link
Contributor

Wasn't sure if the PdfAccessPermissions type should be ulong.
Sharpie gave it the ulong type and used the (1uL << #) but the original header used NSUInteger which according to this should be nuint

@tj-devel709 tj-devel709 added the note-highlight Worth calling out specifically in release notes label Jun 19, 2021
@tj-devel709 tj-devel709 added this to the xcode13.0 milestone Jun 19, 2021
@vs-mobiletools-engineering-service2
Copy link
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/watchOS 32-bits - simulator/Debug (watchOS 5.0): Failed

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 39f006e into 4bc32a9

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.

Wasn't sure if the PdfAccessPermissions type should be ulong.
Sharpie gave it the ulong type and used the (1uL << #) but the original header used NSUInteger which according to this should be nuint

ulong is correct. We'd like to use nuint, but we can't do that for enums, C# doesn't allow it (only [s]byte, [u]short, [u]int and [u]long are allowed as base types for enums), so we go with the second best option: the 64-bit type. It's the correct type for 64-bit platforms (which will one day be all platforms), and then we add the [Native] attribute to tell the generator and our runtime that it should be treated as a 32-bit value on 32-bit platforms.

src/pdfkit.cs Outdated

[iOS (15,0), Mac (12,0), MacCatalyst (15,0)]
[Field ("PDFDocumentAccessPermissionsOption")]
NSString AccessPermissionsOption { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

that last field should with inside PdfDocumentWriteOptionKeys

the hint is that the previous fields (in the headers) are similarly named and (in binding files) already grouped into their own type

Copy link
Contributor

@spouliot spouliot Jun 21, 2021

Choose a reason for hiding this comment

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

and also update the PdfDocumentWriteOptions strong dictionary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spouliot I added PDFDocumentAccessPermissionsOption to PdfDocumentWriteOptionKeys and PdfDocumentWriteOptions. I followed along with the pattern of the other elements in there, but not sure why the "+PDFKit" is there or if I needed to add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops forgot to run tests, I'll do that now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay the tests look good!

@@ -434,6 +438,7 @@ interface PdfDocumentWriteOptions {

string OwnerPassword { get; set; }
string UserPassword { get; set; }
Copy link
Member

@mandel-macaque mandel-macaque Jun 21, 2021

Choose a reason for hiding this comment

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

Is missing the availability attrs which should be the same as those of the key.

Copy link
Contributor

Choose a reason for hiding this comment

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

without them the developer won't know they are not available (since the keys are not public)

and they (internal) keys also needs them otherwise introspection would not be able to do its job :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated :)

@vs-mobiletools-engineering-service2
Copy link
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-1096.BigSur'
Merge c73297b into 6ce5f80

@tj-devel709 tj-devel709 merged commit 6898920 into xamarin:main Jun 22, 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.

5 participants