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

Added algorithm for pagination to PDF export, allowing to export consent forms with more than 1 page (#49) #52

Merged
merged 48 commits into from
Jan 21, 2025

Conversation

RealLast
Copy link
Collaborator

Fixing exported PDF being truncated if text is larger than 1 page(#49)

♻️ Current situation & Problem

Closes #49
If the text in the consent document is longer than 1 page, then the text was truncated during the PDF export of a consent document. The ImageRenderer cannot automatically split the rendered text across multiple pages.

⚙️ Release Notes

  • Added pagination algorithm, automatically splitting the text across multiple PDF pages if the text is too long.
  • Added constraint to ensure footerHeight (signature) + headerHeight (title) + pageHeight <= pdfHeight during PDF generation

📚 Documentation

*Documented changes in the appropriate Markdown files.

✅ Testing

  • Manually tested with 1, 2, and 3 page PDFs for usLetter and dinA4 PDF formats
  • Possibly requires more testing to ensure all edge cases are covered

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

…le pages, if footer, header and text do not fit on a single page.

Addresses StanfordSpezi#49.
RealLast added 2 commits June 22, 2024 16:20
Changed rendering to use ImageRenderer instead of UIGraphicsImageRenderer for compatibility with macOS and visionOS.
@PSchmiedmayer PSchmiedmayer added bug Something isn't working enhancement New feature or request labels Jun 26, 2024
@PSchmiedmayer PSchmiedmayer self-requested a review June 26, 2024 17:47
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you, @RealLast, for investigating the multi-page PDF export functionality and contributing to Spezi.
Sorry for the late review and getting back to this PR after a few days.

I took a look at the general issue a few weeks ago and attempted a similar approach as the one you have been proposing in this PR. While I can see the appeal to using ImageRenderer and SwiftUI views for simplicity and parity between the UI components and PDF export, I would argue that the overall complexity of doing all these functionalities might not be best located in the core goal of Spezi Onboarding and a possible smaller and scoped Spezi Consent package.

I have been debating this over the last few weeks, and I can see two approaches that would generally be more promising and extensible than rendering our own PDF.
This will be especially important as we need to expand this functionality in two different ways in the near future:

  1. Multi-page PDF export as addressed in this PR and hardening this with different possible edge cases that we might have, learning to a significant complexity down the line.
  2. Adding support for more elements within the consent document, including the option to partially consent to elements (e.g., adding toggles that would indicate decisions within the consent document) and options to add initials at parts of the document as required by some Stanford consent document (e.g., HIPAA compliance).

Therefore, I would see two different options:

  1. Follow the path of the old ResearchKit implementation that uses a web view to render the document (e.g. HTML or an MD with JS that renders that in a web page) and then use the PDF export from that.
  2. Use a library that allows us to easily render a PDF on Apple platforms.

I am leaning to the second option, allowing us some better customization and arguably performance in the long run as we don't need to first render the content in a web view and at the same time providing us some flexibility in the implementation using native components.

I have looked at different options and one of the best ones that I found was TPPDF. I looked at it, created a fork to add visionOS support (https://github.com/PSchmiedmayer/TPPDF) and made a PR: techprimate/TPPDF#384.

It would be great to get your input on this @vishnuravi, @philippzagar, @RealLast and what you think about this approach.
We should also take a look at TPPDF in detail and judge its current state. They seem to have a good test coverage, and I tested the PDF exports in visionOS, which all seemed good. An option question is how responsive the maintainers are and how well it would fit our specific use case; I am generally optimistic about both, and it is always great to support some other open-source repos with some contributions that might stem from this integration.

@RealLast
Copy link
Collaborator Author

RealLast commented Jul 4, 2024

Thank you very much @PSchmiedmayer for having a look at this and providing detailed feedback!

I found TPPDF to be highly interesting and it seems to be in a good state. So I had a closer look at it and drafted a proof-of-concept for using TPPDF to do the PDF generation in Spezi. You can check it here. I made it backward compatible, the export still returns a PDFKit.PDFDocument (as opposed to TPPDF.PDFDocument). I simply use TPDDF to generate the PDF, and then convert it to PDFKit.PDFDocument.

That being said, TPDDF is a breeze to use for generating a (multi-page) PDF. It only takes 3 lines to add the title, text, and signature:

let document = TPPDF.PDFDocument(format: .usLetter)
    
let header = await headerToImage()
let signature = await signatureToImage()
      
document.add(image: PDFImage(image: header))
document.add(attributedText: NSAttributedString(markdownString))
document.add(image: PDFImage(image: signature))
                   
let generator = PDFGenerator(document: document)

You can find an example of a two-page consent document PDF generated with TPPDF from Spezi below. Currently, I still render the title and signature footer as Images, but we can also change it to completely use the text rendering of TPPDF. I think the solution is very promising. The only problem is that I could not get the markdown attribution to work. While TPPDF supports AttributedStrings, somehow when generating an AttributedString from markdown it just renders the plain text without any attribution. I can look into this further, if we think that this is our preferred solution.

Signed Consent Form98.pdf

@philprime
Copy link

Hi @RealLast,

I'm the maintainer of TPPDF and due to @PSchmiedmayer's PR on our repo, I felt like I could chime in here regarding the Markdown issue :)

TPPDF is built on-top of CoreGraphics for general rendering and CoreText for handling text.
You can find the full implementation in PDFAttributedTextObject.swift.

As CoreText is quite a complex framework to work with, we opted-in to use NSAttributedString as an abstraction-layer, in combination with CTFramesetter to render it.

We did not get a report of issues rendering Markdown yet, so I would appreciate if you could create a small reproducible example and create an issue for me to look into it. Even though I have hardly any time available to work on TPPDF, I am honestly curious what could cause the issues.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the work @RealLast, for further looking into this!

That looks great and is so much easier compared to our custom logic! As far as I can see, the visionOS PR in TPPDF should also be merged soon and, therefore, unblock us here to continue supporting visionOS. Thanks @philprime for the input and helping us to get the PR merged and incorporating TPPDF in this PR; greatly appreciated!

The attributed String challenges might relate to the way the attributed string is pared (maybe we need to use .full?) or how TPPDF interprets these strings.
As noted by @ philprime, it might be nice if you can see if you can isolate the issue in either TPPDF with a small example of an attributed string not working as expected and create an issue in the repo or isolate it here to our string parsing and see how and if we could improve this.

But apart from this, Feel free to update this PR with the TPPDF-based code; I think this is the way to go.

It would be great if we could also render the title as a native text. Same for the header (maybe TPPDF has built-in functionality for headers and footers?).

It would be amazing if we could also render the signature field as native TPPDF components and only inject the signature itself as an image (ideally high-resolution) but if that is not easily done then we might stick with the image render for the complete signature view. But based on what I could see this should be doable and we already have a canvas to image feature that we can use.

@philprime
Copy link

TPPDF has support for headers & footers and support most elements, as they are just considered as different layout containers. See documentation and example.

Regarding the signature field, I did introduce dynamic geometry shapes a couple of years ago, as I needed to create chat speech bubbles like these:

image

In that use-case I added PDFDynamicGeometryShape which can be used a background shape of PDFGroup which automatically adapts to the calculated frame of the group (each point in the shape has a relative anchor, so that the shape scales dynamically without stretching). There is a simple example available.

Directly adding a PDFDynamicGeometryShape as an element to a PDFDocument is currently not possible, but maybe that is a useful addition to the framework.

Maybe you can use these approaches to render the shape directly into the PDF render context, instead of using an image first.

@RealLast
Copy link
Collaborator Author

@philprime thank you very much for the help on this and the suggestions! I have created a new issue regarding the markdown rendering at the TPPDF repo. In the issue description, you can also find a small reproducible code example and a link to an XCode project (macOS) for further testing. Maybe it's a simple problem and I was simply not using it correctly, looking forward to your response!

@PSchmiedmayer Thanks for the further feedback! I will go ahead and turn this into a fully working solution and change the rendering of the title and signature to use TPPDF functions as well (where possible).

@PSchmiedmayer
Copy link
Member

Amazing, thank you @RealLast and @philprime for the work and input here!

@RealLast
Copy link
Collaborator Author

RealLast commented Jul 11, 2024

I finished the full implementation for generating the PDF using TPPDF. All elements except the signature itself (i.e., the strokes) are now added using TPPDF elements. A one- and two-page PDF example can be found below.

In my tests, it worked very well, and adding new elements to the PDF is very easy using TPPDF.

Some general considerations (mostly regarding the style)

  • Header and footer: I did not use TPPDF’s header and footer features, as I understand that the header and footer are repeated on each page. From my understanding, this is not what we want, we simply want to add the title once in the beginning and the signature at the end. Thus, I simply added the header as the title text and the signature separately.
  • Header timestamp spacing: If exportConfiguration.includeTimestamp is true, then there is a separate text representing the export timestamp above the header. Should there be an exact spacing (i.e., in inch) between the header and the export text? Currently, they are separated by 4 newlines. I did not find a way to add an exact spacing with TPPDF.
  • Signature generation: I first started using PDFDynamicGeometryShape to render the signature box as suggest by @philprime, but then I found the same can be done by simply using a PDFGroup and a line separator. The text and line are rendered using TPPDF, the signature itself is still an image. I think this is the easiest solution and it looks just as good.
  • PDFPageFormat: We need to convert Spezi’s ExportConfiguration.PaperSize to PDFPageFormat. I added a function for this, but we might need to expand this if there are more paper formats added to Spezi.

1page.pdf
2pages.pdf

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the improvements @RealLast; it is great to see the simplification of the PDF export!

I would suggest that we merge a first version of the PR once we have written some smaller unit tests for the export as noted in my comment.
This might require some smaller rework of the logic in the ConsentDocument view, in the long run we should avoid having so much logic in a view and should rather move that to a view model or dedicated Spezi module that is easier to test and also better to separate the logic into separate components.
Then we can go ahead incorporate the markdown syntax parsing in a future PR once we made a PR to the TPPDF framework; I think that would be a great addition.

I have taken a look at the UI testing issues and it is a mixture between the performance of our GitHub runners and some changes in iOS that broke the Files export tests.
The updates UI tests should generally pass (might need to be re-run once if the performance is bad and they all run at the same time). We should explore how we can add more timeouts in some of the UI tests that fail due to the simulator performance; you can inspect my adjusted UI test how I changed it and added some proper Task.wait statements.

The only test that reliably fails is the visionOS test, leading me to believe that we need to adjust something there in how we test the share sheet; would be great if you can try to replicate that locally. You might need to install the latest developer beta of Xcode from developer.apple.com and use the visionOS 2.0 simulator.

@PSchmiedmayer
Copy link
Member

@RealLast Wanted to follow up on the state of this PR and if there is anything where we can support you to merge it. As noted in the comment above, we can TPPDF updates in a future PR but it would be great to include them at some point.

@RealLast
Copy link
Collaborator Author

RealLast commented Aug 14, 2024

Hi @PSchmiedmayer, sorry for the delayed response; was a bit busy the previous week, so I didn't manage to finish it yet. But I'm on it and will push an update this week, maybe even later today :)

@Supereg
Copy link
Member

Supereg commented Sep 6, 2024

@RealLast if I might add some context to the discussion, on iOS 18, vision OS 2, … simulators the Files app is completely broken. You cannot save any files nor create folders. The Save button is just disabled (you can check that with XCTest as well). On visionOS disabled buttons are just really hard to spot. You can clearly see that when running it on iOS. This was first discovered here #55 (comment). In my PR I introduced a logic that automatically skips the test if it detects that the Save button is disabled. Not sure if that will be addressed till the final release of Xcode.

Something I just found out. The release notes of iOS & iPadOS 18 Beta 8 show the following known issue for the files app:
Creating local files in the Files app fails in the visionOS 2 and iOS 18 simulators if the host is not upgraded to macOS Sequoia Beta. (132561244)

So it seems this should work again once the CI agents update to macOS Sequoia later this fall. So, if the tests are any useful, we could still include them but skip them for now, if we detect that the "Save" button is disabled. If we do not need the tests anyways this is no longer important. Removing them will also dramatically decrease the test time which would be a nice side effect.

@RealLast
Copy link
Collaborator Author

RealLast commented Sep 6, 2024

Thanks for the great suggestions and insights @Supereg! I am currently looking into it with the new Xcode version. I agree that if the issue with the "Save" button is resolved with the new macOS version, then we can keep the tests and simply skip them, if the Save button is not there. I will test that out accordingly. Thanks again!

@RealLast
Copy link
Collaborator Author

RealLast commented Sep 6, 2024

Alright, after having checked with the newest Xcode beta version, here are my new findings:

  1. The PDF export Unit test on Vision OS 2.0 fails, because the generated PDF looks slightly different compared to the one generated on Vision OS 1.0. I do not know why this happens, but I suspect that might be related to TPPDF. Sadly I cannot store the known good PDF for Vision OS 2.0, as the file manager does not work.
  2. The UI Tests for visionOS and iOS are failing, because in both cases, the "Save" button is not even available according to the xcresult file (not just disabled, it is simply not there; check the pictures below). I already have the newest code that you added @Supereg, which skips the test if the "Save" button is disabled. However, it still fails if the Save button is not even there.
  3. Further, in one of the UI test cases for iOS, the runner experienced the following error:
Error: error: /Users/githubaction/runner/_work/SpeziOnboarding/SpeziOnboarding/Tests/UITests/.derivedData/Build/Products/Release-iphonesimulator/SpeziViews_SpeziViews.bundle: No such file or directory (in target 'TestApp' from project 'UITests')

For issue 1, it is hard to investigate because we cannot store the PDF file, making it a bit difficult to debug why it looks different. Regarding issue 2, I am happy to introduce some checks that skip the test if the save button is simply not available.

missing save button vision OS
missing save button iOS

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

@RealLast Sorry for the late repose; we have been traveling a lot in the last weeks.

Thank you for the additional context regarding the tests.
I would suggest the following:

  1. I would ensure that we add some unit tests for visionOS that we can put in place by, e.g., just storing the PDF locally in the apps storage directory on the simulator which you can print out in the console. The storage directories on the simulators are actually stored on the macOS file system so you should be able to extract the PDF from there to add it to the unit tests.
  2. I think we might need to remove these tests and rather rely on unit tests that test the UI rendering and a test that just reaches the file sharing UI but doesn't interact with any of these components. We are basically testing Apple's UI which is maybe not the best time investment for us and should probably be something they should do to avoid elements like this 😄
  3. Strange error regarding SpeziViews, might be some optimization in the build process. I would suggest to try it with the latest Xcode release and now that we have updated all our runners to the latest version. If the error persists, @Supereg might have some insights.

…ntrol over appearance of the exported ConsentDocument.

Added Defaults to ExportConfiguration to specify default FontSettings.
Improved PDF export unit test.
Generated new known good PDFs for PDF export unit test on macOS, iOS and visionOS, using fixed font sizes, to ensure similiar appearance across platforms.
Removed "testOnboardingConsentPDFExport" from UI test.
@RealLast
Copy link
Collaborator Author

Thanks @PSchmiedmayer for the new input! I just pushed the new changes and also made the unit test more robust.

  1. I looked into this again and was able to solve it. I now have the unit test working again for VisionOS 2.0+. A major issue was regarding the font rendering and different font styles across VisionOS1.0 and 2.0. To address this, I introduced FontSettings to enable precise control over font size and style during export. Additionally, I introduced ExportConfiguration.Defaults to define default font styles and enforce consistent font sizing.
    TL;DR We should make PDF exports independent of system font styles and sizes to ensure consistency across platforms. The PDF output now looks identical on iOS, VisionOS 1.0, and 2.0, and only minor variations on macOS.
  2. I 100% agree :D I removed the test now and I think we can rely on the unit test in the future.
  3. I verified again that everything works from my side with the newest XCode version. Curious to see the output of the runners later.

@PSchmiedmayer
Copy link
Member

@RealLast Wanted to follow-up on this PR & push a small update to this branch to re-trigger the CI & update dependencies to ensure that we can get this PR merged.

Do you mind giving me write access to your fork?

@PSchmiedmayer
Copy link
Member

PSchmiedmayer commented Jan 20, 2025

@RealLast Thank you for the write access; I pushed a new build and we can see how the builds are looking like with our new Xcode 16.2 CI setup & Swift 6 setup.

We might merge #58 first but it would be great to get both of the PRs merged soon.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Sorry for the many pushes; I used this PR to fix some of our link checking in our CI infrastructure.

Should be ready to be merged now @RealLast & @philippzagar. Code coverage uploads will fail due to the missing token from a fork, but I can merge the PR with my admin permissions.

@philippzagar Let me know if there are any reservations to merge the PR in its current state as noted in #59 (comment)

Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

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

Thanks @RealLast for investing lots of time into getting the right approach here! 🚀
Just did a final review of the PR and from my side we're a read-to-merge state here. I had some minor thoughts about code structure but I'll perform that together with the other PRs that we'll merge soon (before we tag a new major).

@PSchmiedmayer Please merge the PR, as you noted the codecov upload is failing because of the fork.

@PSchmiedmayer
Copy link
Member

Amazing, thank you @philippzagar!

@PSchmiedmayer PSchmiedmayer merged commit 411494e into StanfordSpezi:main Jan 21, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Consent PDF export truncates at 1 page
5 participants