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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

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

@RealLast
Copy link
Collaborator Author

RealLast commented Aug 14, 2024

Hi @PSchmiedmayer thanks a lot! I just pushed an update containing some restructuring and a separate unit test for the PDF export :)

Basically, I separated the PDF export logic into a separate type called “ConsentDocumentModel” (happy to take suggestions for a better name). My idea behind that is that this Model should hold all static information which is necessary to generate a PDF, i.e., the markdown text and the ExportConfiguration. From these two elements, we could already generate a PDF without signature. Once the user has signed the document in the View, we can pass this to the Model as well and export the PDF. This, essentially, allows us to generate PDFs without a signature easily, which is very handy for implementing the unit test (see below). On a side node, I also think this could be good starting point for the “lazy PDF generation” we talked about in the other issue, since we now can generate PDFs independent of the ConsentDocument (view).

I added a “testPDFExport” test which creates a PDF from a text, an export configuration, and an empty signature. The exported PDF is compared against a “known good PDF. I investigated whether it is possible to do a binary comparison, however, this sadly does not work. Even if I create a PDF document twice from the same data, the bytes are slightly different. I guess PDFDocument adds some metadata to the PDF (maybe the current timestamp; I found a similar issue here [1]). Thus, I currently compare the contents (page by page).

There are a few differences in the PDFs between iOS, macOS and visionOS due to differences in the fonts and also the absence of a “real” signature on macOS. Thus, I added 3 separate known good PDFs for each OS as Resources to the SpeziOnboardingTests target. We could also add more PDFs, e.g., with multiple pages, to have a more robust test. We could also move this to an external LFS, if we do not want to have the binaries directly on Github.

Let me know what you think about this; happy to take any suggestions! Once we think the implementation is good, I am happy to pull in #51 before we do the merge. I also already pulled in #55 :)

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 all the improvements and working on this @RealLast!

Extract Core Functionality

I really like the idea to move more of the elements out of the view! I added some comments adding to this. I am wondering if we can even pull the signature and names in the type as well so it serves as a true "export" and "repository" of all entered information while the view state in the view and some smaller @State and @bindings drive the state of the view itself.

We could change the ConsentDocumentExport into a final class (not really source breaking, might reduce the overall async calls to interact with the type):

@Observable
public final class ConsentDocumentExport: Sendable

In some way I would really like to rename this type to ConsentDocument and the view to something different (similar to other types and views), but I suspect that would be a breaking change and we might want to wait for a 2.0 release for that. Maybe we can document this in an issue.

Tests

Thank you as well for adding all the tests; that's amazing and makes it way easier to validate the output compared to any UI tests. We can see how we can even remove some of the elements there to simplify the setup.

I agree that LFS would be the way to go here, also added some thoughts on this in the review comments. I like the idea to also add a multi-page PDF there to make this a bit clearer that this support is there and tested.

/// - paperSize: The paperSize of an ExportConfiguration.
/// - Returns: A TPPDF `PDFPageFormat` according to the `ExportConfiguration.PaperSize`.
func getPDFPageFormat() -> PDFPageFormat {
switch paperSize {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering of we should move paper size into the ConsentDocumentModel in there as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The paper size comes from the ExportConfiguration, and therefore is already stored inside the model.

Tests/SpeziOnboardingTests/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Very cool to see this abstracted in here!

Given the structure we have merged in #52 I would suggest to move the properties & functions that we have here all in the ConsentDocumentExport actor? We would generate the ConsentDocumentExport right when the ConsentDocument view is initialized and then feed it with additional information as things are moving along. In the end it serves as a stable element that the view passes to the outside and it encapsulates all the functionality to get information about decisions (see #54) and the asynchronous PDF rendering functionality.

Copy link
Collaborator Author

@RealLast RealLast Aug 24, 2024

Choose a reason for hiding this comment

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

Great, I had the same thought while I introduced the ConsentDocumentModel and already had thought about proposing this as well! I have now merged in the changes from #51 and moved all the functions and properties to the ConsentDocumentExport. In general, I think makes the most sense moving forward, and now all the information + export are nicely separated from the view and can be passed around. I also took the chance and implemented the lazy PDF export when accessing the async pdf variable.

In general, the approach works, however, there were some minor "complications" or issues I had to work around, which I think we should discuss:

  1. The ConsentDocument creates a ConsentDocumentExport during initialization and feeds in the information (i.e., name, signature, ...) as they come in. During export, we need to pass the ConsentDocumentExport to the OnboardingConsentView. Before, the exported PDF was passed via the ConsentViewState. Now, we would need to pass the ConsentDocumentExport, thus the signature of the state has to change.
  2. Further, the OnboardingConsentViews listens for the state to become "exported", and then immediately uses the exported PDF in some synchronous functions (e.g., when showing the share sheet). However, the access to the PDF is now async via the ConsentDocumentExport. We would then need to asynchronously extract the PDF again, e.g., when showing the share sheet. But we know that the PDF has been exported before by the ConsentDocument (otherwise we wouldn't even be in the "exported" state). Therefore, I currently implemented it in a way that the exported state takes both the exported PDF (so that it can be accessed synchronously), as well as the ConsentDocumentExport, which can then be passed to the OnboardingDataSource for storing. An Alternative for this would be to let the OnboardingConsentView hold the ConsentDocument as internal variable, and then simply read out the ConsentDocumentExport, once the state changes to exported.
  3. In Supporting to distinguish multiple consent forms during onboarding via an identifier (#50) #51 we wanted to keep the signature of the store function of the OnboardingDataSource to avoid a breaking change. Thus, we made it so that the ConsentDocumentExport is only created inside the store function (see here) from the passed PDF and identifier. However, this now doesn't really make sense anymore, as the ConsentDocumentExport is already created by the ConsentDocument. Thus, it seems we have to change the signature here, so that we can pass the ConsentDocumentExport to the OnboardingDataSource directly.
  4. I have some warnings regarding async access to some of the variables from ConsentDocumentExport via "non main-actor isolated contexts". I have resolved most of them, but some seem not to be easily fixable with the current structure. I can look into this further once we have discussed on whether to keep the current approach.

Tests/SpeziOnboardingTests/SpeziOnboardingTests.swift Outdated Show resolved Hide resolved
Moved functionality of ConsentDocumentModel to ConsentDocumentExport.
ConsentDocumentExport now is a container storing all information required for exporting a ConsentDocument to a PDF.
The ConsentDocument holds a ConsentDocumentExport and fills in the information such as exportConfiguration, content, signature and name of the signee as they come in.
PDF export is thus separated from the ConsentDocument. The ConsentDocumentExport now supports "lazy loading" of PDFs, i.e., if the PDF was not exported before (no cached PDF was set), the export function will be called automatically when accessing the async pdf property.
@RealLast
Copy link
Collaborator Author

RealLast commented Aug 24, 2024

Hi @PSchmiedmayer thanks again for checking the code and for the suggestions! I have adapted the implementation and moved the relevant code and properties to the ConsentDocumentExport. I added a few considerations as comment to our previous discussion above.

Also, I expanded the unit test to test against a known good PDF with 2 pages. I changed the structure of the test, so it would be easier to also add additional files with 3 or more pages in the future.

Finally, I have set up tracking for the PDF files with LFS. From now on, they should be managed by lfs automatically. I also added a few words about this to the readme.

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 working on the next steps here; I have added the LFS support for our reusable GitHub Actions and we can see that a lot of tests are passing successfully!

It seems like the visionOS tests are failing as the PDF looks slightly different on visionOS.

Thank you for generating all the PDFs. I think it would be great if we can generally reduce the margins for all the test to render it more similar to a normal document. Might mean that we have to add more text to our example MD file, but it would get closer to a real PDF export and consent document.

Apart from this: I only had a few smaller comments and once those are addressed, we can merge the PR; thank you for keeping track of all the changes and requests. Looking forward to get this merged 🚀

.reuse/dep5 Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Sources/SpeziOnboarding/ConsentView/ConsentDocument.swift Outdated Show resolved Hide resolved
Sources/SpeziOnboarding/ConsentView/ConsentDocument.swift Outdated Show resolved Hide resolved
Comment on lines 49 to 55
if let cached = cachedPDF {
return cached
} else {
cachedPDF = await export()
// If the export failed, return an empty document.
return cachedPDF ?? .init()
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can avoid creating an empty PDF document as we can ensure that it is never nil:

Suggested change
if let cached = cachedPDF {
return cached
} else {
cachedPDF = await export()
// If the export failed, return an empty document.
return cachedPDF ?? .init()
}
if let pdf = cachedPDF {
return pdf
}
let pdf = await export()
cachedPDF = pdf
return pdf

Copy link
Collaborator Author

@RealLast RealLast Sep 4, 2024

Choose a reason for hiding this comment

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

That's a good point, however the problem is that the PDF export could actually fail. When generating the PDFDocument from TPPDF, the generator could throw, and/or the data could be incompatible with PDFDocument, so that the document is nil (see here lines 175-183).

I am not sure under which circumstances TPPDF would throw, or in what cases the data might be incompatible with the PDFDocument, but we cannot assure that this will not happen. As an alternative, we would need to make the export function throwing. But then, also the async pdf variable of ConsentDocumentExport would still need to return an empty document in that case (or simply be throwing as well?)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think the best option is to bubble the issue up to the pdf variable. We can also make this property throwing and pass the error up to the caller to handle it there. Better than giving back an empty PDF.

Choose a reason for hiding this comment

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

Hi @RealLast, just got a message about your comment (seems like I am still subscribed to this PR), so I'll give my two cents here:

TPPDF is designed to throw errors in the case when the APIs are not used correctly (i.e. when accessing indicies of a table which are out of bounds).

You can actually lookup the throwing code in the repository to see it yourself.

Hope this helps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect, I have made the export throwing and propagate a potential exception up to the pdf variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that we probably do not really have full "control" over the content of the PDF file, as the content is generated based on a markdown file. Theoretically, users of Spezi could write down whatever they like in that markdown file. I am not sure if we can guarantee that this content is always "valid". For example, if we add support for tables to markdown in the future, can we guarantee that this table would not be empty (i.e., PDFError.tableIsEmpty)? Also, even if TPPDF does not throw an error, there is still the chance that the conversion from TPPDF.PDFDocument to PDFKit.PDFDocument fails, if PDFKit cannot parse the data (see here). Not sure if that would ever really happen though.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation 🚀

Is my understanding correct, that the pdf generation fails if (and only if) the original markdown document contains invalid elements (that cannot converted to a pdf document). Or phrased differently, if PDF generation fails it fails all the time or if it doesn't fail it will never fail. If that would be the case, we could verify that a markdown document can generate a valid PDF document at startup (or when we receive the markdown document).

In any case, would it be feasible to do verification of the pdf generation of the markdown file on startup (or once we receive that markdown file) anyways? Just to ensure that such errors can be caught as early as possible.

Just adding some of my general thoughts here. I really do not have any deep insights of the development or what you already discussed with @PSchmiedmayer. So well free to ignore whatever I said if it doesn't apply or was already though of 🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, thanks for the considerations! You are right, I have the same understanding. The PDF generation should only fail if we provide invalid content, and then it should fail consistently, either because TPPDF throws or the generated data is invalid. Given that the markdown content is static and does not change, we could definitely test this at startup. However, then the "lazy PDF export" probably would not thaat much sense anymore (as we already test the PDF generation during startup).

Copy link
Member

Choose a reason for hiding this comment

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

Good context @philprime @Supereg @RealLast!

Given the context above I would make the variable non-throwing and rather focus on an initial verification when loading the markdown or other content (we would need to do this anyways, especially when we load it from a server) and display an appropriate error message in the UI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @PSchmiedmayer! I have changed it accordingly and made it non-throwing. Yet, the async functionality is still available, and if no cachedPDF was provided, then the export is triggered when acessing the PDF. We agreed to verify the content beforehand, so this should never throw, but still I would need to return a default value if it does. Thus, just for completeness, I added this statement:

guard let pdf = try? await export() else {
              return .init()
          }

As long as always pass known good markdown text to the export function, this should never get called.

Readded old store function to OnboardingDataSource.
Changed ConsentDocument to use guard let instead of if let.
@RealLast
Copy link
Collaborator Author

RealLast commented Sep 4, 2024

Thanks @PSchmiedmayer! I have pushed a new version :)
I added some new PDF documents with better margins, to make it more realistic.
I rerun all tests locally and they worked for me. I have checked the xcresult of the previous runners to figure out what was the issue.

For the UI test on VisionOS, it seems that the runner was again unable to press the "save" button during the "testOnboardingConsentPDFExport". I am not sure whether I can do anything from my side to resolve this, but maybe we want to remove that test since we now have the unit test. Regarding the failed unit test on VisionOS, I am not sure why this happens, because I also tested this locally on my side and it works. The only thing I could imagine is that you might be using a more recent version. I will look into this.

@PSchmiedmayer
Copy link
Member

Thank you, I will take a look at the PR diff later this week.

For a short response: Yes, it might be that we are already using the visionOS 2.0 simulators, you can download the latest Xcode 16 release from the developer web page, it might be released as a final version in the next few days anyways. You can check the logs which simulator version is used.

Thank you for looking into the tests. If we validate that the unit tests work as expected we can go ahead and remove the UI test. Might be worth a try to identify why the save button is not pressed first (probably a change in the bundle identifier to the save dialogue or some changes there). You might also be able to replicate this better with the latest Xcode versions.

…n propagates up to the ConsentDocumentExport.
@Supereg
Copy link
Member

Supereg commented Sep 4, 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.

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

Sources/SpeziOnboarding/ConsentView/ConsentDocument.swift Outdated Show resolved Hide resolved
Comment on lines 49 to 55
if let cached = cachedPDF {
return cached
} else {
cachedPDF = await export()
// If the export failed, return an empty document.
return cachedPDF ?? .init()
}
Copy link
Member

Choose a reason for hiding this comment

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

Good context @philprime @Supereg @RealLast!

Given the context above I would make the variable non-throwing and rather focus on an initial verification when loading the markdown or other content (we would need to do this anyways, especially when we load it from a server) and display an appropriate error message in the UI.

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

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: In Progress
Development

Successfully merging this pull request may close these issues.

Consent PDF export truncates at 1 page
4 participants