<html> <head> <style></style></head> <body class='hmmessage'><div dir='ltr'>The patches work for me with the example-all.pdf test file.<div><br></div><div>I said the same thing about the change from the if-else to the case. Also, in the new patch, if rotate doesn't have one of the four special values, m.init() isn't called. setRotate() in PSOutputDev.h sets rotate without checking, so I don't know how safe it is to depend on the possible values of rotate.</div><div><br></div><div>I also agree about using ceil(). I think that it is necessary to go to the next highest value.</div><div><br></div><div>Regards, William</div><div><br><br><div>To: Bugzilla@pahlx.de; poppler@lists.freedesktop.org<br>From: ajohnson@redneon.com<br>Date: Tue, 27 Oct 2015 22:47:27 +1030<br>Subject: Re: [poppler] Printing of certain PDF files does not work with "fit-to-page" because of wrong BoundingBox values in the PostScript<br><br><pre>On 25/10/15 02:58, Martin Pahl wrote:<br>> Attached you find an example:<br>> <br>> example.pdf: a box generated with xfig 321mm x 106mm<br>> <br>> example.ps generated with pdftops -paper A4 example.pdf<br>> example.ps:%%PageBoundingBox: 0 0 302 912<br>> <br>> example-patched.ps generated with pdftops -paper A4 example.pdf, but with the <br>> patched version.<br>> example-patched.ps:%%PageBoundingBox: 158 0 437 842<br>> <br>> You can view the postscript files with gv and select BBox. It's obvious that <br>> the unpatched PageBoundingBox is totally wrong, because source coordinates are <br>> used. The PageBoundingBox is bigger then the A4 paper format. It's just the <br>> size of the original box (302 /72 dpi * 25,4mm = 106,5mm; 912/72 dpi * 25,4mm <br>> = 321,73mm). But it must be the box scaled down to A4 and shifted to the <br>> center.<br>> <br>> Hope this example helps.<br> <br>I've reviewed the patch and have the following comments.<br> <br>It would have been a lot easier to review (and probably would have been<br>reviewed earlier) if you avoided the unnecessary changes to convert<br>if-else statements to case statements. Putting code style changes in a<br>separate patch to the bug fix makes reviewing changes much easier.<br> <br>+ int(pbbty),<br>+ int(width * xScale + pbbtx + 0.5),<br> <br>Using floor() and ceil() would be better and would make the code easier<br>to understand.<br> <br>You appear to be ignoring the value of tx and ty prior to centering<br>calculations. What happens if tx and ty are non zero at this point? I<br>would be more comfortable with the patch if the page bbox calculations<br>used the exact same transformation as is output to PS.<br> <br>I'm attaching a new patch that I think is a lot easier to understand. It<br>uses the same transformation as is output to PS to calculate the<br>bounding box.<br><br></pre></div></div> </div></body> </html>