Hi John,

Thanks for the contribution.

AFAIK you'd have to call QPrinter::setCurrentPage(x) before you pass the printer to the dialog for the the ‘Current Page’ option to appear, which seems a bit strange. Wouldn’t it be better to have an option you can set on the dialog (e.g. a new PrintDialogOption), that says whether or not the ‘Current Page’ option is to be shown? And if it is shown and the user have selected to print the current page, this is reflected in the QPrintDialog’s printRange() and the QPrinter’s toPage(), fromPage()? I can’t see the usefulness of QPrinter having a separate API for the current page number, when all the application needs to know is that the user wishes to print the current page.

Some general nitpicking, which I'm obliged to convey: the patch doesn’t strictly follow the Qt coding conventions. More specifically: we don’t use scoping braces for ‘if’ statements where the body only contains a single line and has no ‘else’ clause, in the setCurrentPage() function there’s to many space in between the ‘(’ and the expression in the if statements. Also when refering to OSX in the docs, you'd have to print that fully out: Mac OS X.

I hope this hasn’t put you off from contributing things to Qt, that is certainly not the intention.

Thanks for the review :–)

I'd point out that the setCurrentPage() is needed as how else would the fromPage() and toPage() know what value to return for the current page to print? And if the call to setCurrentPage() is required, then an extra call to set a PrintDialogOption would seem unnecessary (although it could do this transparently in the background).

Or are you suggesting that the app coder would need to specifically check if the printRange() == CurrentPage and execute a different path in that case? In which case what value should fromPage() and toPage() return, I assume 0?

If that is your preferred option, then I will also have to think about how that affects my next set of patches implementing multiple page ranges and page sets that I’ll be posting later this week.

Nitpicking is welcome, it needs to fit in and I find it hard to break kdelibs style :–) But I will repeat the point I've made earlier with other Qt persons that merge requests seem a poor place to be trying to sort out fundamental design and api issues, but that’s the path I've been told to take.

What I propose is the following:

An app coder that wants to support the ‘current page’ notion in his app would have to do this:

1. Call QPrintDialog::setOptions(QAbstractPrintDialog::PrintCurrentPage) in order to have the dialog display the option for selecting to print the current page.

2. When the dialog is closed, the QPrinter::pageRange() would return QPrinter::CurrentPage, if the user selected the ‘Current Page’ option in the dialog and the app coder would have to handle that.

That is all the app needs to know, and only the app knows what ‘Current Page’ means in this context. What fromPage() and toPage() returns is not really relevant in this case (they could be defined to return 0 in this case), since it’s simply “the current page”.

By doing it this way we can make use of the APIs that QPrintDialog and QPrinter already has, and it fits well what is there already (two new enums values are needed, obviously).
What do you think about that?

OK, updated to use a PrintDialogOption instead, style changes, and rebased onto master to discard my realNumPages merge request in favour of your change.

Only issue I've had with this is the auto test, for some reason it appears the print dialog is not gaining focus in the test so the alt-p click is being discarded and the test fails. If I extend the delay timer and manually click the title bar to focus the dialog it works OK, as does clicking Print manually. I'm not sure if this is just a local issue?

Updated to include QWS / Qtopia print dialog, again I've been unable to compile or test but it is a very simple copy-and-paste of the Selection option.

This looks good. However, opening the QPrintDialog in your test case will most likely only work on a Linux system, but you can drop that part of the test for now. Tests for QPrintDialog should ideally go into tests/auto/qprintdialog, but since we don’t have tests to cover that yet, it can wait. So please re-submit with that part of the test removed.

Note that I will squash your commits into a single change when I integrate this, which means I’ll have to edit the commit message(s), unless you prefer to do that yourself.

One last thing, you can easily check the QWS build by just configuring Qt with ‘configure -embedded -qvfb’ and compile. That will work on most Linux desktop system.

OK, I've fixed the test case, compiled under QWS, and squashed the commits with a new commit message (about time I learned how to do that :–). We should be good to go.

Thanks for the tip about QWS, that will come in handy in the future.

Coming next, Page Sets.

The patch doesn’t compile under Windows: qprintdialog_win.cpp:132 – the ‘q’ there should be ‘pdlg’. Please fix and re-submit.

As for testing the QWS build: just start the qvfb tool (must be from a normal X11 build, not the embedded one) and run e.g. the textedit demo with ‘textedit -qws’. The app should appear in the qvfb app.

Updated with windows fix.

I tried the qvfb thing with textedit, and that runs nicely, but the print dialog I get is the Unix one and not the QWS one. Looking in the dialogs.pri I see that it needs “contains(QT_CONFIG,qtopia)” to build the QWS dialog instead of the Unix one, but -qtopia is not a valid configure option and I can’t figure out what the right option would be.

You can leave that check for now.

I just merged your change into 4.7. I managed to sneak it into 4.7 since the API change only involved a couple of new enums. It should appear in the public repo in a day or two. Thanks for your contribution John.