Bug 26584 – [Qt] Provide a way for better control over the process of printing

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Jakob Truelsen]

There should be a way to add extra information on printed pages, and a way to print more then one page in one job.

[Jakob Truelsen]

Created attachment 31613 [details]
Add support for suplying a costum painter, and counting pages

[Jakob Truelsen]

Created attachment 31614 [details]
Add support for suplying a costum painter, and counting pages

[Holger Freyther]

Comment on attachment 31614 [details]
Add support for suplying a costum painter, and counting pages

Please see:
   http://webkit.org/coding/coding-style.html (specially for the way you handle if statements)





> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 44911)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Jakob Truelsen  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix: https://bugs.webkit.org/show_bug.cgi?id=26584
> +
> +        Allow one to count page numbers and to print extra information on pages.

maybe mention that you add API?


> +	
> +        * Api/qwebframe.cpp:
> +        (QWebFrame::countPages):
> +        (QWebFrame::print):
> +        * Api/qwebframe.h:
> +
>  2009-06-19  Daniel Teske <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> Index: WebKit/qt/Api/qwebframe.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebframe.cpp	(revision 44911)
> +++ WebKit/qt/Api/qwebframe.cpp	(working copy)
> @@ -1033,15 +1033,44 @@ bool QWebFrame::event(QEvent *e)
>  
>  #ifndef QT_NO_PRINTER
>  /*!
> -    Prints the frame to the given \a printer.
> +    \since 4.6
> +    Count the number of pages, that will be printed by print.
> +
> +    \sa print
> +*/
> +uint QWebFrame::countPages(QPrinter *printer) {
> +    const qreal zoomFactorX = printer->logicalDpiX() / qt_defaultDpi();
> +    const qreal zoomFactorY = printer->logicalDpiY() / qt_defaultDpi();
> +
> +    PrintContext printContext(d->frame);
> +    float pageHeight = 0;
> +
> +    QRect qprinterRect = printer->pageRect();
> +
> +    IntRect pageRect(0, 0,
> +                     int(qprinterRect.width() / zoomFactorX),
> +                     int(qprinterRect.height() / zoomFactorY));
> +
> +    printContext.begin(pageRect.width());
> +    printContext.computePageRects(pageRect, 0, 0, 1.0, pageHeight);
> +    uint count = printContext.pageCount();
> +    printContext.end();
> +    return count;
> +}

If print() is const, countPages should be const as well... then please check of uint is ever used in Qt API. I think it is not the case and you should either go for unsigned int or such... And it seems a bit wasteful to use a PrintContext for that...



> +/*!
> +    Prints the frame to the given \a printer. If \a painter is
> +    provided print with this painter, this allows to print multiple
> +    pages into on job.
>  
>      \sa render()
>  */
> -void QWebFrame::print(QPrinter *printer) const
> +void QWebFrame::print(QPrinter *printer, QPainter * painter) const

You can not simply remove a paramater (+ style guideline violation). You will need to add a new overload...


>  {
> -    QPainter painter;
> -    if (!painter.begin(printer))
> -        return;
> +    QPainter * innerPainter = painter;
> +    if(!innerPainter) innerPainter = new QPainter();
> +    if(!innerPainter->isActive()) innerPainter->begin(printer);

style guide violation...


>      for (int i = 0; i < docCopies; ++i) {
>          int page = fromPage;
> @@ -1101,10 +1130,13 @@ void QWebFrame::print(QPrinter *printer)
>                      return;
>                  }
>                  printContext.spoolPage(ctx, page - 1, pageRect.width());
> -                if (j < pageCopies - 1)
> +                if (j < pageCopies - 1) {
> +                    emit printingNewPage(printer,fromPage,toPage,page);
>                      printer->newPage();
> +                }
>              }
> -
> +            emit printingNewPage(printer,fromPage,toPage,page);
> +        
>              if (page == toPage)
>                  break;
>  
> @@ -1121,6 +1153,8 @@ void QWebFrame::print(QPrinter *printer)
>      }
>  
>      printContext.end();
> +    innerPainter->restore();
> +    if(innerPainter != painter) delete innerPainter;
>  }
>  #endif // QT_NO_PRINTER
>  
> @@ -1229,6 +1263,18 @@ QWebFrame* QWebFramePrivate::kit(WebCore
>  */
>  
>  /*!
> +  \fn void QWebFrame::printingNewPage(QPrinter *printer, int fromPage, int toPage, int Page) const
> +  \since 4.6
> +
> +  This signal is emitted when when a page is printed. This allows one
> +  to add extra contest 

contest? did you meant conent?


>  public Q_SLOTS:
>      QVariant evaluateJavaScript(const QString& scriptSource);
>  #ifndef QT_NO_PRINTER
> -    void print(QPrinter *printer) const;
> +    void print(QPrinter *printer, QPainter * painter=NULL) const;

Don't use NULL in C++, and due ABI compability requirements you can not do that...


and in general I'm not sure if this is the best way of achieving this....

[Jakob Truelsen]

Do you have any pointers on what would be a good way to achieve this.. I have thought long and hard, but I have not found a better way. Inpaticular I cannot think of a nother way to print two documents into one job. 

[Jakob Truelsen]

Created attachment 31643 [details]
A better patch

As for using an printcontext in the countpages method: It is my impression that you need a print rendertree, to predict the number of pages.  It would of cause make good sense to use the same render three both for counting and for printing. But this would as I seed it require changes to existing API.  In my experience building the printcontext does not take long. 

If anyone know of a better way to achieve then please let my know. I'm willing to implement it.

[Adam Treat]

Comment on attachment 31643 [details]
A better patch

I'll just say at the outlet that I don't think this new API is appropriate or necessary.  I think the answer you are seeking is to just forgo the convenience API that QWebView/QWebFrame allows for printing and do the heavy lifting yourself.  It sounds like you are writing a specific application that has demands above and beyond what the convenience API allows.  I believe you can still accomplish your goals by using QWebFrame::render directly to draw the entire page to a QImage for instance.  And then, in your client code, you can do the heavy lifting of breaking up this QImage and sending it to the QPrinter.

That said, there were a number of problems with the patch still:

> +        Added an overloared QWebFrame::print allowing one to use an
> +        existing painter object.  

Spelling mistake.

> +unsigned int QWebFrame::countPages(QPrinter *printer) const {

I guess this should return a quint32?

> +    This allows to print multiple pages into on job.

Spelling mistake.

> +void QWebFrame::print(QPrinter* printer, QPainter* painter) const
> +{
> +    if(!painter->isActive() && !painter.begin(printer))  
>          return;

This looks wrong.  Why the check for painter->isActive() and why the && logic?

> +    This signal is emitted when when a page is printed. This allows one

Typo.

> +    void printingNewPage(QPrinter* printer, int fromPage, int toPage, int page) const;

Coding style issues with * decoration.

[Jakob Truelsen]

> Coding style issues with * decoration.
Why is this the case, if you read the Other Punctuation section of 
http://webkit.org/coding/coding-style.html

It tells me to do it like this (even though I also do not like it)

[Jakob Truelsen]

You tould me to use render instead of print. However I fail to see how
this would work. Render does not render under print-media-type (show
stopper). And also I would have to do the cutting into pages my self,
there is no way I could ever get this to support
'page-break-inside:avoid'. Even the render tree is not exposed, and
thus the is no way for me to know if I am cutting a text string in
half..

It seems to me that this is not something obscure than only I need..
Anyone implementing a browser on top of qtwebkit (arora?), would
probably want to add some kind of header and footer to the printed
document, with page: number, date and so on.  I do not see anyway that
this is currently possible..

As you (and everyone else I have talked with) seems to think that the
patch (given I fixed all the little faults) still is the wrong way to
add this to the API. My question is then what is the right way?

[Jakob Truelsen]

Reply from Trent:
First, this conversation would be much better easier to follow if it occurred
on the bugzillla.  Please reply there.

I am saying what you should do is render the WebPage to a QImage yourself.
The entire page without any breaks or clipping.

And then you can manipulate that QImage yourself into several smaller QImage's
corresponding to each page... add footers, and headers... and then send the
set of those QImage's to the printer to be printed.

I fail to see how this is impossible or how you could have any show stoppers.
A webpage can be rendered to a QImage and you can print QImage's yourself.

What is the problem?

[Jakob Truelsen]

There are a lot of problems with the solution that you propose, some of which I stated before, but let me state them again.

1) When printing with "render", it will use the display render tree. And not a new render tree produces under print media-type.. Thus what is rendered is not what should be printed.. This could be fixed by allowing one to switch the media-type but that would again require changes to the API.

2) When printing to a QImage you get a pixmap, and not a vector format. Rendering the QImage to a pdf document will not magicly convert it back into a vector format.

3) Printing the whole thing as one long image and splitting it up is a very hard thing to do.  It is not mearly a case of splitting up an image into some rectangels.  You need to splite above or below a line of text.  You cannot split in nodes that have style="page-break-inside:avoid".  If something can almost fit on one page you scale it down and so on and so fourth.  WebKit has a nice piece of code that does this for you. 


In all the solution proposed by Mr. Trent is not acceptable.

I seek to be able to do the following:

1) Count the number of pages that is going to be printed, I feel that this is something others might also need (and not some crasy feature that only I need)

2) To be able to insert stuff into each page printed. Again I feel that this this is a general feature that others might need.   This is actually possible today, by an ugly hack. That goes as follows.  Create a subclass of QPrinter exposing the setEngines method.  Create a normal QPrinter instance, then create a instance of the subclass then setEngines, with the engines from the first instance, only the QPrintEngine instance must be wrapped in a class doing something more on the new page virtual method.

3) Allow a supplied painter to be used (so that one can print more then one document into a single print job). Arguable this is not a feature that alot of others would need, but it does not change the fact that I do.


I would like to know how an api that *could* be used to do one ore more of the above could look like, so that it might be merged?

[Jakob Truelsen]

Created attachment 34388 [details]
New patch

Fixes the petty issues from comment 6. Safe the part about the style violation, which is clearly wrong.

[Jakob Truelsen]

Created attachment 34389 [details]
Newer patch

[Jakob Truelsen]

Created attachment 34390 [details]
New patch

[Eric Seidel (no email)]

Comment on attachment 34390 [details]
New patch

Looks like you deleted a line from the ChangeLog, probably by accident here:
[phone]  Jakob Truelsen  <[e-mail]>
+        Reviewed by NOBODY (OOPS!).

Seems you'll need a Qt person to review this.

[Eric Seidel (no email)]

Comment on attachment 34390 [details]
New patch

You ChangeLog entry is malformed.

style:
 1152 quint32 QWebFrame::countPages(QPrinter* printer) const {

c++ code in WebCore uses static_cast<int>:
+                     int(qprinterRect.width() / zoomFactorX),
+                     int(qprinterRect.height() / zoomFactorY));

WebCore style is to omit argument names when they don't add value:
 #ifndef QT_NO_PRINTER
-    void print(QPrinter *printer) const;
+    void print(QPrinter* printer) const;
+    void print(QPrinter* printer, QPainter* painter) const;
 #endif
maye that's not true of qt?

Since no Qt person has made a comment on this in the 20+ days that it has been up or review, I can only assume they don't want this patch.  I would encourage you to try and track down a Qt reviewer (ideally over email) and get their attention before posting the revised version.

r- for the nits above.

[Ariya Hidayat]

I am also not so confident that the new API functions that you propose here is
the right way to implement it. I am not even sure that it will work (you do not
include any unit test at all).
Note that I fully agree that there should be a mechanism for the developer to
control and fine-tune the printing process.

Anyway, here is my feedback.

> +quint32 QWebFrame::countPages(QPrinter* printer) const {

Looking at other Qt API functions, the closes I get is pageCount(), e.g.
QTextDocument. Granted, it does not take a QPrinter as an argument, hence more
like a property rather than an action. I am not sure about the name
"countPages", it seems to me that the web frame is counting the pages, but then
what? Maybe something like "computePageCount" sounds better? Feel free to
suggest other names as well.

> +void QWebFrame::print(QPrinter* printer, QPainter* painter) const

Here is my biggest concern, I am not sure that passing another QPainter is the
right thing to do. Read further more for more comments.

> +    \fn void QWebFrame::printingNewPage(QPrinter *printer, int fromPage, int toPage, int Page) const

This does not make a good API. A signal is usually in the form of past tense,
because it indicates that something has happened. Otherwise use the form
aboutToDoSomething.
Also, have you checked whether this works at all? I mean, from the moment the
signal is emitted until you do something in your object slot, another new page
might have been printed.


Finally, here is an idea if I were to implement this (I don't know if this is
technically manageable or not). Instead of using "pull" strategy, what about a
"push" one. Look at the code:

int pageCount = frame->computePageCount(myPrinter)
// setup the print dialog if necessary, which might need the page count
frame->beginPrint(myPrinter);
for (int i = 1; i < pageCount; ++i)
  frame->spoolPage(i);
frame->endPrint();

The drawbacks: we put some kind of state information into the QWebFrame, hence
you can't really interleave the printing e.g. to two printers (but seriously,
who does that).
Also if you need to grab the QPainter that operates on the printer, it's
possible via myPrinter->paintEngine()->painter() but then it's const (might
need some casting). If this is ugly, we might need some other hook like
"printingPainter()".

r- until we solve this issues.

[Jakob Truelsen]

I totally agree with the push strategy. However the solution you suggest have some draw backs as you say: One cannot do multiple prints on the same frame, one cannot print multiple frames into the same document (Or any other stuff for that matter).  I think that it would be cleener to make a seperate class for printing webpages,  QWebPrinter or whatever.  

Then one could do something like the following:

QWebPrinter p(myPrinter)
p.begin(myFrame)
for(int i=0; i < p.currentFramePageCount(); ++i)
  p->spoolPage(i);
p.end();

The QWebPrinter class could then have the following methods

::QWebPrinter 
   construct the QPainter and set up stuff
::begin()  
   Setup a frame for printing, construct the print context, compute pages rects and so on
::end()  
  Release the print context
::spoolPage
  Print a given page for the current frame
::currentFramePageCount 
  Return the number of pages for the current fram
::painter
  Return the current painter
::printer
  Return the current printer

Then QWebFrame print method could then be reduced to creating a QWebPrinter and calling spool page on it

[Jakob Truelsen]

If the above solution (with some prober naming) has a chance of being merged.  Then I am willing to put in the work to make it.  If there is some completely other way of doing it that is better please let know.

[Ariya Hidayat]

I am not sure I understand your concerns "One cannot do multiple prints on the same frame" and "one cannot print multiple frames into the same document" correctly. Could you elaborate and possibly give an example?

Note that even if the frame contains the printing state information, it is still possible to print the frame to different printers, provided that you don't interleave the printing process. And I am pretty confident there is no useful use case of such a printing interleave (I am happy to be proven wrong).

Before we delve into making a new class, let's find out whether we can get away without it.

[Jakob Truelsen]

Sure thing, so for the project I am developing what I need is to print the content of several different web pages (frames) into one print job (Create one pdf document).  The problem is that if the frame creates a printer then that is not possible. (One can extend QPrinter and make some ugly hacks but that is besides the point). Of cause the project I am working on (qtwkhtmltopdf) requires a lot of different changes to make all its features working, most of which you (webkit/qt) has made it quite clear are to obscure to make it into qt/webkit.  If printing multiple frames into one document is also considered an obscure thing (I do not find it obscure). Then I of cause am willing to implement the functionality into the QWebFrame object as you described.

[Ariya Hidayat]

I would argue that merging several contents into one printer job to result in one single PDF rather belongs to the Qt core library. Imagine if we need to duplicate this functionality everywhere (e.g. printing from other sources, QTextDocument), literally we would sprinkle the API with functions that can be centralized in e.g. QPrinter.

Coming back to the patch, if we only exclude the above merging feature, can we get away without introducing a new class (QWebPrinter)? IOW is that possible to realize the code snippet I posted before?

[Jakob Truelsen]

I have made a patch allowing one to do

frame->beginPrint(myPrinter);
int pageCount = frame->printingPageCount(myPrinter)
for (int i = 1; i < pageCount; ++i)
  frame->spoolPage(i);
frame->endPrint();

note that moving the pageCount inside the begin and end.. it can be made an O(1) operation and then it makes sence calling it printingPageCount instead of computePageCount.

I will submit the patch once I get it testet probberly, and within the style guidelines..

[Jakob Truelsen]

Created attachment 39513 [details]
Ariya Hidayat's solution

[Jakob Truelsen]

Created attachment 39514 [details]
My solution

This patch implements my solution. Ariya's proposal has the following disadvantages:
* For Ariya's solution to work the print slot can no longer be const. The only way it could be const, is to either duplicate the code or use const_cast.
* Printing state is stored in the QWebFrame, where I feel it does not belong.
* A lot of pointer magic is needed to store the state.
* All the added methods needs to have some document string stating that they are only to be installed between begin and end
It has the following advantages
* No new classes are added

My solution has the following diadvantages
* A new class is added
* The QWebPrinterBeginCaller constructor is "clever code" and not very pretty.
It has the following advantages
* the print slot can stay const
* no printing state is stored in QWebFrame and as such one can make two print jobs concurently. In the future I hope to have even more state added to printing and as such this will not get into the QWebFrame.

My current solution no longer allows printing more document then one into a print job, Ariya rightfully pointed out that that belongs elsewhere, and is therefore quite simple.

[Eric Seidel (no email)]

Ping?  No comments in 2 months.  I'm not capable of reviewing this, however I would expect Qt folks are. :)

[Simon Hausmann]

Comment on attachment 39513 [details]
Ariya Hidayat's solution

This patch has various style issues and it changes the signature of an existing public method, which breaks binary compatibility with Visual Studio builds.

I think the concept of it is okay, but we indeed have to discuss properly whether this belongs straight into QWebFrame or into a separate class.

[Simon Hausmann]

Comment on attachment 39514 [details]
My solution

This patch, too, has coding style issues.

However the bigger problem right now is that the Qt team is crunching towards the next release together with Qt 4.6. Apparently there is not enough bandwidth to review this API currently.

We prioritized review of existing API and bugfixing over this feature, unfortunately.

However we need to make time for this after the release.

As it stands right now this bug only shows up in the list of webkit reviewers.

For the API design I would like to involve everyone interested, in particular the Qt team of course.

That means we either add everyone to the CC list of this bug (not feasible) or we take the API discussion to the webkit-qt mailing list first. One we reach a concencus there I think it makes sense to go back to the coding side and bring it back onto the radar of the webkit code reviewers.

[Simon Hausmann]

*** Bug 29859 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

*** Bug 29868 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

*** Bug 29863 has been marked as a duplicate of this bug. ***

[Jakob Truelsen]

I see that there is has been some activity in this bug report lately. I just wanted to note that the patches I have posted in here are not my latest patches. The can be found here

http://qt.gitorious.org/+wkhtml2pdf/qt/wkhtmltopdf-qt/commits/wk-printing

In paticular the interface I propose is found here
http://qt.gitorious.org/+wkhtml2pdf/qt/wkhtmltopdf-qt/blobs/wk-printing/src/3rdparty/webkit/WebKit/qt/Api/qwebframe.h#line109

If anyone should have interest in this. I am willing to push the new patches up here, including doing proper documentation, and coding style.

[Diego Gonzalez]

Created attachment 79593 [details]
Patch to try do turn this bug alive again

[Benjamin Poulain]

Comment on attachment 79593 [details]
Patch to try do turn this bug alive again

(In reply to comment #32)
> Created an attachment (id=79593) [details]
> Patch to try do turn this bug alive again

It is nice someone is working on this.

But I do not get the purpose of your patch(?). The QWebPrinter is not accessible by the public API. The test is also not really testing anything new there.

The changelog do not describe anything so not much help there.


Personnally, I would like to see the printing support improve. I thing a public class to control the print settings could make sense. If this is done, I think that should not be for just one use case but for the most common settings offered by QtWebKit.

Since it is new API, it will need API review. You can start a new thread on the mailing list to suggest a new API. This way the chance of changes because of the API review is lower.

[Diego Gonzalez]

(In reply to comment #33)
> (From update of attachment 79593 [details])
> (In reply to comment #32)
> > Created an attachment (id=79593) [details] [details]
> > Patch to try do turn this bug alive again
> 
> It is nice someone is working on this.
> 

Thanks Benjamin,

> But I do not get the purpose of your patch(?). The QWebPrinter is not accessible by the public API. The test is also not really testing anything new there.
> 
> The changelog do not describe anything so not much help there.
> 

I will start to improve it

> Personnally, I would like to see the printing support improve. I thing a public class to control the print settings could make sense. If this is done, I think that should not be for just one use case but for the most common settings offered by QtWebKit.

agree

> Since it is new API, it will need API review. You can start a new thread on the mailing list to suggest a new API. This way the chance of changes because of the API review is lower.

I will start this thread after improve the patch. Thanks :)

[Indrajit]

As there are no updates on this report for a long time and I am not in the mailing list either, do not know the status of this.

Is this going to be in near future? or something else is suggested to get better printing controls ?