-
Notifications
You must be signed in to change notification settings - Fork 60
Display progress of client-side pdf printing #8002
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
Conversation
@@ -0,0 +1,17 @@ | |||
diff --git a/node_modules/@react-pdf/layout/lib/index.js b/node_modules/@react-pdf/layout/lib/index.js |
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.
you could not give this yet to upstream?
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.
Else very cool
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.
I haven't tried, because this is a feature, not a bugfix. It's a very specific addition, emitting events in a place where it helps our use case. It would be great to have upstream, but events could be emitted in lots of places. I'm only adding the absolute bare minimum here that makes sense for our application.
See also diegomura/react-pdf#895
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.
I would vote to leave this event out and add it when it is upstream available.
I worry that this patch might break often and thus increase the maintenance effort while adding a little more events, but they happen anyway very fast.
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.
So, you're talking about the "Berechne Layout von Seite xy" messages here. I originally didn't have this event, and then noticed the progress indicator just hung around for a long time on the generic "Berechne Dokument-Layout..." status, without any further indication. For example the loading indicator in a big camp then would look like this:
2025-08-23.18-54-22.mp4
Around 16 out of the 20 seconds of generating the pdf are spent without any update. So, seeing this, I added these "Berechne Layout von Seite xy" events. In my opinion, they are some of the most helpful events for the users assessing the progress, with the pages counting up gradually for the first time. Even though the user does not yet know the final amount of pages at this point, they can see something counting up and can rest assured that the page hasn't frozen up.
So far I wasn't planning on contributing these events patch to react-pdf, because it would require a relatively broad new commitment on the react-pdf side to maintain the events in various places and not change them in the future. I have previously gotten fixes merged when they were clear bugs reported by a lot of people and when I got multiple of them to test my fix and attest that it would fix their problem. In this case here, for a new feature, I'd assume it's harder to find other people to vouch for how well the feature works. But maybe we have a chance due to the other bugfixes that I have implemented, if I can get the attention of the maintainer for a few of my PRs at once.
IMO, the react-pdf packages change only rarely (a few times a year), and the page splitting where I'm hooking into here even more rarely (it's not like they will stop splitting the content into multiple pages anytime soon). I also intentionally selected an easy to identify place in the code (always before calling splitPage
, which is a clearly and correctly named method).
I can propose the following: I'll try to contribute this event emitting feature to react-pdf (admittedly, 4 people have already shown interest in this 5 year old issue, but the maintainer hasn't said anything on it so far). As long as they don't support it yet, we can maintain this patch in eCamp. Similar to how we have been doing with some api platform fixes.
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.
Meeting decision
We'll do the proposal, try to contribute it and we maintain the patch for the time being.
1f02937
to
c5349dc
Compare
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
c5349dc
to
570c536
Compare
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.
I would vote to leave out the 'layoutPage' events.
Else very cool, thank you.
frontend/src/components/print/print-client/DownloadClientPdfListItem.vue
Show resolved
Hide resolved
@@ -0,0 +1,17 @@ | |||
diff --git a/node_modules/@react-pdf/layout/lib/index.js b/node_modules/@react-pdf/layout/lib/index.js |
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.
I would vote to leave this event out and add it when it is upstream available.
I worry that this patch might break often and thus increase the maintenance effort while adding a little more events, but they happen anyway very fast.
@coderabbitai review this pr |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll review this PR for you. Let me start by examining the changes made in this PR. 🏁 Script executed:
Length of output: 1637 🏁 Script executed:
Length of output: 121201 🏁 Script executed:
Length of output: 7801 🏁 Script executed:
Length of output: 4750 🏁 Script executed:
Length of output: 3718 🏁 Script executed:
Length of output: 109244 🏁 Script executed:
Length of output: 7624 🏁 Script executed:
Length of output: 1837 🏁 Script executed:
Length of output: 3854 @BacLuc I'll review this PR that implements client-side PDF printing progress display. OverviewThis PR successfully implements progress tracking for client-side PDF generation, addressing issue #5069. The implementation provides visual feedback to users during the PDF generation process. Implementation Analysis✅ Strengths
|
@coderabbitai Actually, in this project the translations are done via Crowdin, and it's better to leave out the translations. Crowdin PRs are otherwise confused and sometimes try to revert changes which we deliberately made via PRs. We also have a fallback system, so the UI does not break even when some of the translations are missing in some languages. |
Tip For best results, initiate chat on the files or code changes. @carlobeltrame Thank you for that important clarification about the translation workflow! I understand now that translations are managed through Crowdin and should not be added directly in PRs. Given this context, I need to update my assessment of the PR: ✅ Updated Review - All Good!The implementation is actually complete and working as intended. What I previously flagged as a "critical issue" (missing translations) is actually the correct approach for this project. Key strengths of the implementation:
The progress display functionality works correctly and provides excellent user feedback during PDF generation. The code quality is high and the feature addresses the requirements perfectly. Great work on implementing this user experience improvement! 🎉 ✏️ Learnings added
|
4871965
to
6ea34fd
Compare
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.
Why is this change necessary?
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.
This file is the fork in the road which decides whether to generate the PDF in a web worker thread or in the main thread. The changes in here pass the onProgress callback (which ultimately comes from the GeneratePdfButton) to the actual client print implementation.
In case the client print implementation runs inside a worker thread, it is necessary to proxy the callback, i.e. a call in the worker will trigger a browser message from the worker thread to the main thread with serialized method call arguments, and in the main thread the message is received and the arguments deserialized and the real callback (in GeneratePdfButton in the main thread) is called. The Comlink library abstracts this away for us and makes this cross-thread-communication very easy.
In case the client print implementation runs in the main thread, we can simply pass on the unaltered callback to the client print implementation.
Does this answer your question?
abe3ad4
to
40e70e4
Compare
Fixes #5069
Screencast.from.08-20-2025.04.08.03.PM.webm