D7962: Implement several new print scaling options

[Henrik Fehlauer]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20180722/a712c49a/attachment-0001.html>