Reviewing merge request #2335: QPrinter: Add support for Multiple Page Ranges

Allow the user to enter multiple page ranges to print, e.g. "1-3,5,9-15". Pages can be entered in any order and may be duplicated, e.g. "1-3,9-4,5,5".

The option is enabled by setting the Print Dialog option PrintMultiplePageRanges and optionally setting a default Page Range to be displayed. If a default is not set the From/To range is used.

The option is supported under UNIX, Windows, and QWS. The option is not supported under Mac OS X, but this is transparent to the app.

Sample initialisation code:

QPrinter printer(QPrinter::HighResolution);
printer.setFromTo(1, document()->numberOfPages());
printer.setPageRange("%1-%2").arg(1).arg(document()->numberOfPages());
QPrintDialog *dialog = new QPrintDialog(&printer, this);
dialog->setMinMax(1, document()->numberOfPages());
dialog->addEnabledOption(QAbstractPrintDialog::PrintMultiplePageRanges);
if (dialog->exec() == QDialog::Accepted) {
document()->print(&printer);
}

UNIX compiled and fully tested.

Windows compiled and fully tested.

QWS compiled and tested with UNIX dialog, QWS dialog untested.

Mac OSX not compiled or tested.

Possible issues:

API Naming:
pageRange() and setPageRange() may be considered too close to printRange() and setPrintRange(). Alternatives could be pageRanges() or multiplePageRanges() or something else?

Print Engine PPK_SupportsMultiplePageRanges Key:
Not actually used anywhere (I originally had code checking it, but realised it wasn't needed), this could be removed but I think it does no harm to keep it around in case anyone has need to check this.

[Note target branch is only set to 4.7 as master wasn't compiling, real target is 4.8]

Commits that would be merged:

Version 1
  • Version 1
  • 78b2e94
  • 6fdb350

    • QPrinter: Add support for multiple page ranges

Showing 78b2e94-6fdb350

Comments

Pierre started to review this patch. Please be patient, it is not a small patch :)

avatar Benjamin Poulain | 2010-03-23 18:36:43 UTC 

+     \value PrintMultiplPageRanges The multiple page range selection option is enabled

PrintMultiplPageRanges –> a “e” is missing at Multiple

avatar Benjamin Poulain | 2010-03-29 12:13:18 UTC on src/gui/dialogs/qabstractprintdialog.cpp 

+     void setPageRange(QString fromPageRange);
+     void setPageRange(QList<int> fromPageList);
+     QString pageRange() const;
+     QList<int> pageRangeAsList() const;
+

We will need to do an API reviews for those function.

I am personally not fan of: 

-void setPageRange(QString fromPageRange);

What if the string cannot be parsed? I would add a “bool *ok” to check if it can be parsed for example.

-void setPageRange(QList<int> fromPageList);

How do you pass 1-1000, 5, 2-1000?

I would rather use QList<QPair<int, int> >, in order to do:

 QList<QPair<int, int> > range;
 range << QPair<int, int>(1, 1000);  
 range << QPair<int, int>(5, 5);  
 range << QPair<int, int>(2, 1000);  
 setPageRange(range);

What are the return value of fromPage() and toPage() when there are multiple page range? It might be useful to have a function like:

bool hasMultiplePageRange() const;

?

What do you think?

avatar Benjamin Poulain | 2010-03-29 12:30:17 UTC on src/gui/painting/qprinter.h

→ State changed from New to Revise and resubmit

Nice contribution, thanks !

I mostly agree with Benjamin for the previous points, and I believe the additions to the public API will need a review before it gets integrated in any case.

Additionally, using QList<QPair<int, int> > internally as well rather than QString and providing fromString/toString convenience functions would seem cleaner to me.

Ideally the fromPage and toPage members could be dropped if a bool hasMultiplePageRange() is to be introduced, but then the behavior of fromPage() and toPage() is yet to be defined in the case of multiple page ranges (i.e. ranges.size() > 1).

Lastly, the auto test could probably be re-factored using a testMultiplePageRanges_data() slot.

avatar Pierre Rossi | 2010-03-29 19:45:40 UTC

I agree with Benjamin and Pierre – using a string to input the page range is not a common pattern in Qt. In fact, it’s a pattern we try to avoid at all costs due to the fact that you'd have to parse the string, and that it creates a non-intuitive and non-typesafe API.
Using a QList<QPair<int, int> > is a much better choice, but please typedef the QList<QPair<int, int> > into something (e.g. QPageRange), since it won’t look very nice in an API.

avatar Trond Kjernåsen | 2010-04-06 11:36:09 UTC

→ State changed from Revise and resubmit to Closed

Thank you for your contribution. With Qt 4 having moved [1] under Open
Governance [2] as well, you can now contribute your changes via Gerrit
[3]. At the same time, contributions via Gitorious can no longer be
merged.

We would still like to see your patch on the new platform, and wish
for you to become a member of the Qt Project. Please read [4] and in
particular [5] to familiarize yourself with the new tools. You will
still find a read-only mirror of all Qt repositories on Gitorious.

With a much wider set of possible reviewers, we are also looking
forward to better response times to contributions. Looking forward to
your patch!

Please note that at this point in time Qt 4.8 is a bugfix only branch,
and all fixes should be submitted to Qt 5 first, as far as
applicable. Qt 4 master is closed.

The Nokia Qt team

[1] http://labs.qt.nokia.com/2012/01/10/qt-4-moved-to-open-governance/
[2] http://labs.qt.nokia.com/2011/10/21/the-qt-project-is-live/
[3] http://codereview.qt-project.org
[4] http://www.qt-project.org
[5] http://developer.qt.nokia.com/wiki/Gerrit_Introduction

avatar The Qt Project | 2012-01-17 11:38:05 UTC

Add a new comment:

Login or create an account to post a comment

How to apply this merge request to your repository