-
Notifications
You must be signed in to change notification settings - Fork 567
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
Add qtpdf and qtpng exporters #1611
Conversation
Nice work David, could you comment on the size of Qt vs Chromium, like what a minimal environment would take up in disk space? |
@maartenbreddels the PyQt5 package takes 357M on my machine and the pyppeteer Chromium download takes 300M. |
Excellent stuff! I had played with this some on nbconvert-pdfqt (hasn't been touched in a while). This uses JupyterLab as the renderer, and indeed, I would see that as eventually becoming a desirable feature. There were some features added (e.g.
In some package managers (e.g. But the big win, for my use case, is being able to specify/lock a single set of dependencies, with a single package manager, and get something that works without a "surprise" runtime download. It also means it's possible to create full, platform-specific distributions (a la Even if QtWebEngine lags behind whatever frankenchromium comes out of some of these custom things, it's likely the Qt one is going to be packaged more robustly, and supported for longer. Finally,
|
nbconvert/exporters/qtpdf.py
Outdated
self.size = self.page().contentsSize().toSize() | ||
self.resize(self.size) | ||
# Wait for resize | ||
QtCore.QTimer.singleShot(1000, self.take_screenshot) |
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.
Here we wait for one second for the resize to take effect. I know it's not great but I couldn't find another way.
nbconvert/exporters/qt_screenshot.py
Outdated
page_layout = QPageLayout(page_size, QPageLayout.Portrait, QtCore.QMarginsF()) | ||
else: | ||
factor = 0.75 | ||
page_size = QPageSize(QtCore.QSizeF(self.size.width() * factor, self.size.height() * factor), QPageSize.Point) |
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.
self.page().contentsSize()
already returns a QSizeF
object while QSizeF.toSize
returns a QSize
object.
We should be able to get away with
page_size = QPageSize(self.page().contentsSize(), QPageSize.Point)
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.
then we should be able to drop the line self.size = self.page().contentsSize().toSize()
.
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.
Hum, actually, looking at the unit QPageSize.Point
, it looks a bit arbitrary. I think we should determine that based on the DPI ratio of the "device".
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.
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.
page_size = QPageSize(self.page().contentsSize(), QPageSize.Point)
I had already tried that, but it generates a PDF with a larger size than the required page size.
Hum, actually, looking at the unit
QPageSize.Point
, it looks a bit arbitrary. I think we should determine that based on the DPI ratio of the "device".
I agree, I've been through these threads, but I couldn't find any relevant information about the DPI ratio.
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 have been doing some more experiments with this and I must say that I am very confused. I am enclined to merge the pull request as it is.
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.
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.
As long as it's a general Qt issue, and not PyQt specific I can try to get your question to the people working in QtPdf 👍 but if it's PyQt specific, you will need to reach the folks at Riverbank.
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 think it is likely to be Qt. I will post something a bit more specific.
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.
@cmaureir the bug is that printToPdf adds whitespace at the end of the document. It does not do so when exporting to PDF. (This only occurs when we set pagination to False or course). Multiplicating the dimensions by 0.75 magically makes all whitespace disapear.
nbconvert/exporters/qt_screenshot.py
Outdated
else: | ||
raise RuntimeError(f"Export file extension not supported: {output_file}") | ||
self.show() | ||
self.app.exec() |
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.
When trying your PR locally, the process gets stuck on this line.
This happens when doing:
jupyter nbconvert --to qtpdf Note.ipynb
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.
Weird, it doesn't happen on my side. Could you post the installed versions of your environment?
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.
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 have the exact same versions of the qt-related packages as I have in my local environment, so I'm not sure what is going on here.
Can you export your notebook to e.g. HTML?
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.
Sorry for the late answer. I can easily export to HTML though WebPDF also doesn't work, the process gets also stuck.
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 suspect the WebPDF issue (and the issue I'm having with the QtScreenshot
) is related to the Chromium version installed on my machine. It seems to work on my other machine.
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 could fix the chromium version issue I mentioned for the webpdf export.
Though your PR still doesn't work for me. It looks like the loadFinished
event is never triggered, although the loadProgress
reached 100
. If I change your PR to use loadProgress
instead of loadFinished
(and waiting for it to reach a 100), I get an empty file for the pdf though.
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 think you can ignore my comments then. I reproduce this issue with basic qtwebengine examples, it's not due to your PR.
dec9089
to
4188792
Compare
setup.py
Outdated
@@ -244,6 +244,12 @@ def get_data_files(): | |||
'webpdf': [ | |||
pyppeteer_req | |||
], | |||
'qtpdf': [ |
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.
As with pyppeteer
, this likely needs to be added to test
.
Another approach, as we've done on ipython, is to move all of these to setup.cfg
and use the barely-documented string replacement technique: https://github.com/ipython/ipython/blob/master/setup.cfg#L78
I'd be curious to know if something similar to this: #1718 would need to or could be done. Does PyQt allow passing a |
Answering my own question. It looks like this PR works like a charm with the latest |
9e51658
to
3133478
Compare
I can see that installing |
You should still be able to use |
Thanks @blink1073, let's try that. |
9c02b22
to
e3295d1
Compare
for more information, see https://pre-commit.ci
Similar to the webpdf exporter, but using pyqtwebengine.