Differential D7949 Diff 37522 generators/poppler/generator_pdf.cpp

Changeset View

Standalone View

generators/poppler/generator_pdf.cpp

Context not available.
26	#include <qstack.h>	26		#include <qstack.h>
27	#include <qtemporaryfile.h>	27		#include <qtemporaryfile.h>
28	#include <qtextstream.h>	28		#include <qtextstream.h>
		29		#include <QComboBox>
29	#include <QPrinter>	30		#include <QPrinter>
30	#include <QPainter>	31		#include <QPainter>
31	#include <QTimer>	32		#include <QTimer>
Context not available.
75	Q_OBJECT	76		Q_OBJECT
76		77
77	public:	78		public:
		79
		80		enum PrintBackend {
			rkflxUnsubmitted Done Add `Q_ENUM(PrintBackend)`… rkflx: Add `Q_ENUM(PrintBackend)`…
		81		CUPSBackend,
		82		QPrinterBackend
		83		};
		84		Q_ENUM(PrintBackend)
		85
78	PDFOptionsPage()	86		PDFOptionsPage()
79	{	87		{
80	setWindowTitle( i18n( "PDF Options" ) );	88		setWindowTitle( i18n( "PDF Options" ) );
Context not available.
87	m_forceRaster->setToolTip(i18n("Rasterize into an image before printing"));	95		m_forceRaster->setToolTip(i18n("Rasterize into an image before printing"));
			rkflxUnsubmitted Done `m_printBackend->insertItem(PrintBackend::CUPSBackend, "CUPS");`. ← Do it like this everywhere you currently have `0`. This way you don't risk mixing up magic numbers all over the code in the future. rkflx: `m_printBackend->insertItem(PrintBackend::CUPSBackend, "CUPS");`. ← Do it like this everywhere…
88	m_forceRaster->setWhatsThis(i18n("Forces the rasterization of each page into an image before printing it. This usually gives somewhat worse results, but is useful when printing documents that appear to print incorrectly."));	96		m_forceRaster->setWhatsThis(i18n("Forces the rasterization of each page into an image before printing it. This usually gives somewhat worse results, but is useful when printing documents that appear to print incorrectly."));
			rkflxUnsubmitted Done Remove `UNIX only` and only show the option on Unix in the first place. However, keep the combobox on Windows even if it only has the QPrinter option (clear communication and useful for users switching between Windows and Linux to not get confused debugging printing issues). rkflx: Remove `UNIX only` and only show the option on Unix in the first place. However, keep the…
89	layout->addWidget(m_forceRaster);	97		layout->addWidget(m_forceRaster);
		98
		99		m_printBackend = new QComboBox;
			rkflxUnsubmitted Done `i18n` needed here? rkflx: `i18n` needed here?
			michaelweghornUnsubmitted Not Done Should "this" be passed in the constructor to have this auto-destroyed and avoid a memory leak? michaelweghorn: Should "this" be passed in the constructor to have this auto-destroyed and avoid a memory leak?
		100		// Windows can only print with the QPrinter backend, because the CUPS backend
		101		// is UNIX-specific.
			rkflxUnsubmitted Done `i18n` everywhere rkflx: `i18n` everywhere
			rkflxUnsubmitted Done pdf → PDF postscript → PostScript rkflx: pdf → PDF postscript → PostScript
		102		#if !defined(Q_OS_WIN)
		103		m_printBackend->insertItem(CUPSBackend, i18n("CUPS"), CUPSBackend);
		104		m_printBackend->setItemData(CUPSBackend, xi18n("Print using the CUPS printing system.<nl/>This will convert your PDF file to PostScript first,<nl/>and then send the PostScript file to the printer."), Qt::ToolTipRole);
		105		m_printBackend->setItemData(CUPSBackend, xi18n("Print using the CUPS printing system.<nl/>This will convert your PDF file to PostScript first,<nl/>and then send the PostScript file to the printer."), Qt::WhatsThisRole);
			rkflxUnsubmitted Done Remove extra space before "This", add linebreak with `<nl/>` (needs `xi18n` instead of `i18n`). PDf → PDF rkflx: Remove extra space before "This", add linebreak with `<nl/>` (needs `xi18n` instead of `i18n`).
			rkflxUnsubmitted Not Done As there is no way to access `Qt::WhatsThisRole` from the UI, perhaps the line for `Qt::ToolTipRole` is enough? rkflx: As there is no way to access `Qt::WhatsThisRole` from the UI, perhaps the line for `Qt…
		106		#endif
		107
		108		// Print using the platform-independent Qt infrastructure. By necessity,
			rkflxUnsubmitted Done definitely `i18n` missing rkflx: definitely `i18n` missing
		109		// this uses a backend of the Poppler PDF library which is called 'Arthur'.
			rkflxUnsubmitted Done Remove all three lines. Could always be brought back if there were actually multiple options on Windows. rkflx: Remove all three lines. Could always be brought back if there were actually multiple options on…
		110		// This Arthur backend is still somewhat immature, and you may see misrenderings
			rkflxUnsubmitted Done pdf → PDF rkflx: pdf → PDF
		111		// of your PDF document. In fact, the backend is so bad before poppler 0.60
		112		// that we'd rather not expose the QPrinter option at all for older poppler versions.
		113		// On Windows we have no choice, because there the CUPS backend will not work.
		114		// On the positive side, QPrinter printing allows more printing options.
		115		#if defined(Q_OS_WIN) \|\| HAVE_POPPLER_0_60
		116		m_printBackend->insertItem(QPrinterBackend, i18n("QPrinter (experimental)"), QPrinterBackend);
		117		m_printBackend->setItemData(QPrinterBackend, i18n("Print with Qt (experimental, you may see rendering artifacts)"), Qt::ToolTipRole);
		118		#endif
			rkflxUnsubmitted Done Add colon. rkflx: Add colon.
		119		QWidget* formWidget = new QWidget(this);
		120		QFormLayout *printBackendLayout = new QFormLayout(formWidget);
		121		printBackendLayout->addRow(i18n("Print backend:"), m_printBackend);
		122		layout->addWidget(formWidget);
		123
90	layout->addStretch(1);	124		layout->addStretch(1);
91		125
92	#if defined(Q_OS_WIN) && !defined HAVE_POPPLER_0_60	126		#if defined(Q_OS_WIN) && !defined HAVE_POPPLER_0_60
93	m_printAnnots->setVisible( false );	127		m_printAnnots->setVisible( false );
94	#endif	128		#endif
		129
95	setPrintAnnots( true ); // Default value	130		setPrintAnnots( true ); // Default value
96	}	131		}
97		132
			rkflxUnsubmitted Done …then you can just do `return m_printBackend->currentData().value<PrintBackend>();` rkflx: …then you can just do `return m_printBackend->currentData().value<PrintBackend>();`
Context not available.
115	m_forceRaster->setChecked( forceRaster );	150		m_forceRaster->setChecked( forceRaster );
116	}	151		}
117		152
		153		PrintBackend printBackend() const
		154		{
		155		return m_printBackend->currentData().value<PrintBackend>();
		156		}
		157
		158
118	private:	159		private:
119	QCheckBox *m_printAnnots;	160		QCheckBox *m_printAnnots;
120	QCheckBox *m_forceRaster;	161		QCheckBox *m_forceRaster;
		162		QComboBox *m_printBackend;
121	};	163		};
122		164
123		165
			rkflxUnsubmitted Done As you depend on `pdfOptionsPage` where you already have this check anyway, this could go. Too easy to miss that you always have to change both places. The comment is nice though, just move it to the right place. rkflx: As you depend on `pdfOptionsPage` where you already have this check anyway, this could go. Too…
			rkflxUnsubmitted Done Just rename your variable to `oldRenderBackend`, then the initial comment should suffice and this one could go. rkflx: Just rename your variable to `oldRenderBackend`, then the initial comment should suffice and…
Context not available.
1269	{	1311		{
1270	bool printAnnots = true;	1312		bool printAnnots = true;
1271	bool forceRasterize = false;	1313		bool forceRasterize = false;
		1314		PDFOptionsPage::PrintBackend printBackend = PDFOptionsPage::CUPSBackend;
1272		1315
1273	if ( pdfOptionsPage )	1316		if ( pdfOptionsPage )
1274	{	1317		{
1275	printAnnots = pdfOptionsPage->printAnnots();	1318		printAnnots = pdfOptionsPage->printAnnots();
1276	forceRasterize = pdfOptionsPage->printForceRaster();	1319		forceRasterize = pdfOptionsPage->printForceRaster();
		1320		printBackend = pdfOptionsPage->printBackend();
1277	}	1321		}
1278		1322
1279	#ifdef Q_OS_WIN	1323		if ( printBackend == PDFOptionsPage::QPrinterBackend )
			rkflxUnsubmitted Done Coding style: Add spaces around `==`. rkflx: Coding style: Add spaces around `==`.
1280	// Windows can only print by rasterization, because that is
1281	// currently the only way Okular implements printing without using UNIX-specific
1282	// tools like 'lpr'.
1283	forceRasterize = true;
1284	#ifndef HAVE_POPPLER_0_60
1285	// The Document::HideAnnotations flags was introduced in poppler 0.60
1286	printAnnots = true;
1287	#endif
1288	#endif
1289
1290	#ifdef HAVE_POPPLER_0_60
1291	if ( forceRasterize )
1292	{
1293	pdfdoc->setRenderHint(Poppler::Document::HideAnnotations, !printAnnots);
1294	#else
1295	if ( forceRasterize && printAnnots)
1296	{	1324		{
		1325		#ifdef HAVE_POPPLER_0_60
		1326		// The Document::HideAnnotations flags was introduced in poppler 0.60
		1327		pdfdoc->setRenderHint( Poppler::Document::HideAnnotations, !printAnnots );
1297	#endif	1328		#endif
		1329
		1330		// Scale to the entire page, not only the printable area
		1331		printer.setFullPage( true );
		1332
1298	QPainter painter;	1333		QPainter painter;
1299	painter.begin(&printer);	1334		painter.begin(&printer);
1300		1335
Context not available.
1311	Poppler::Page *pp = pdfdoc->page( page );	1346		Poppler::Page *pp = pdfdoc->page( page );
1312	if (pp)	1347		if (pp)
1313	{	1348		{
		1349		if ( forceRasterize )
			michaelweghornUnsubmitted Not Done The indentation here is a little odd in my eyes, since the "if" is indented further than the following lines inside of the "if" block. I think moving the "if" (and "else" below) one indentation level to the "left" and the block one indentation level to the "right" would be the usual way to do it. michaelweghorn: The indentation here is a little odd in my eyes, since the "if" is indented further than the…
			sanderAuthorUnsubmitted Not Done The idea was the following: New conditional are introduced that surround existing code. For nice identation all of that would have to be reformated, which makes for a patch full of whitespace changes. I didn't want that, but the new code has been indented directly at the correct indentation. At least that was the plan... sander: The idea was the following: New conditional are introduced that surround existing code. For…
			michaelweghornUnsubmitted Not Done I don't know what "the right way" to handle this is within the Okular project. I just personally found the code a bit hard to read like this. michaelweghorn: I don't know what "the right way" to handle this is within the Okular project. I just…
		1350		{
1314	#ifdef Q_OS_WIN	1351		#ifdef Q_OS_WIN
1315	QImage img = pp->renderToImage( printer.physicalDpiX(), printer.physicalDpiY() );	1352		QImage img = pp->renderToImage( printer.physicalDpiX(), printer.physicalDpiY() );
1316	#else	1353		#else
Context not available.
1318	QImage img = pp->renderToImage( 300, 300 );	1355		QImage img = pp->renderToImage( 300, 300 );
1319	#endif	1356		#endif
1320	painter.drawImage( painter.window(), img, QRectF(0, 0, img.width(), img.height()) );	1357		painter.drawImage( painter.window(), img, QRectF(0, 0, img.width(), img.height()) );
		1358		} else {
		1359		// temporarily switch to poppler's Arthur backend.
		1360		// It is the only one that implements renderToPainter.
		1361		auto actualRenderBackend = pdfdoc->renderBackend();
		1362		pdfdoc->setRenderBackend( Poppler::Document::ArthurBackend );
		1363
		1364		pp->renderToPainter( &painter, printer.logicalDpiX(), printer.logicalDpiY() );
		1365
		1366		pdfdoc->setRenderBackend( actualRenderBackend );
		1367		}
1321	delete pp;	1368		delete pp;
1322	}	1369		}
1323	userMutex()->unlock();	1370		userMutex()->unlock();
Context not available.