rkflx added inline comments.
INLINE COMMENTS
> generator_pdf.cpp:80-89
> + bool isVisible = (printBackend() == QPrinterBackend ||
> m_forceRaster->isChecked());
> + m_scaleMode->setVisible ( isVisible );
> + m_scaleTo->setVisible ( isVisible );
> + if ( isVisible ) {
> + m_printBackendLayout->labelForField(m_scaleMode)->show();
> + m_printBackendLayout->labelForField(m_scaleTo)->show();
> + } else {
In general it's preferred to only "disable" options which are unavailable or do
not apply in a given context instead of hiding them, e.g. with `setEnabled(…)`.
This should result in less confusion for users.
Apart from that, nothing to complain about your approach ;)
> michaelweghorn wrote in generator_pdf.cpp:151
> Should this be `m_scaleMode->insertItem(FitToPage, i18n("Fit to page"),
> FitToPage);` (and likewise for all other calls to `insertItem` below?
> Otherwise a `QVariant()` is inserted.
Indeed, compared to a similar change in D7949
<https://phabricator.kde.org/D7949> the third parameter (i.e. where the magic
happens) is missing.
I'm not sure using `enum class` would gain us much, because then for the first
parameter we'd have to provide `int index` ourselves (e.g. `0, 1, 2`) and keep
track of duplicate numbers. However, this would not yet fix the issue of
decoupling the implementation of `scaleMode()` from the actual position in the
combobox. For that, setting `userData` in the third parameter is still required.
> generator_pdf.cpp:154
> + m_scaleMode->insertItem(None, i18n("None"));
> + m_printBackendLayout->addRow(i18n("Scale mode"), m_scaleMode);
> +
I'd suggest to append a colon after each label (if it belongs to another item
like a combobox), just like you did for Print backend:.
> generator_pdf.cpp:159
> + m_scaleTo->insertItem(FullPage, i18n("Full page"));
> + m_printBackendLayout->addRow(i18n("Scale to"), m_scaleTo);
> +
Another missing colon…
> generator_pdf.cpp:165-170
> + // Show scaleMode and scaleTo only if experimental QPrinter
> backend is selected
> + // or if the file is to be rasterized before printing
> + m_scaleMode->hide();
> + m_scaleTo->hide();
> + m_printBackendLayout->labelForField(m_scaleMode)->hide();
> + m_printBackendLayout->labelForField(m_scaleTo)->hide();
I'd use `setEnabled(false)`, see other comment.
> rkflx wrote in generator_pdf.cpp:1217
> Missing space: `g,v`.
Not really important, but this is marked as "done" even though the space after
the comma is still missing:
horizontalScaling,verticalScaling
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D7962
To: sander, #okular, aacid
Cc: okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid
