GNOME Bugzilla – Bug 695524
/FitR Operator within Navigation Destination scales wrongly
Last modified: 2013-05-13 16:54:14 UTC
Created attachment 238479 [details] PDF file exposing the issue. The PDF specification states in Section 8.2.1 about the /FitR navigation destination: "[ page /FitR left bottom right top ] Display the page designated by page, with its contents magnified just enough to fit the rectangle specified by the coordinates left, bottom, right, and top entirely within the window both horizontally and vertically. If the required horizontal and vertical magnification factors are different, use the smaller of the two, centering the rectangle within the window in the other dimension. A null value for any of the parameters may result in unpredictable behavior." Evince however does not center the rectangle within the viewing area. Also there are some PDFs out there (most notably by altium designer, a PCB editor) which have dubiously constructed bounding boxes where bottom > top. Acrobat Reader 9 compensates for that by min/max'ing accordingly and I think evince should also silently ensure that left < right and bottom < top. Attached is a hand crafted PDF (I don't guarantee for its correctness, but it seems to work) which has various bounding box /FitR targets. This should help in evaluating this problem. While I only tested with evince 3.4.0 the problems seems to be in 3.6.x as well.
Created attachment 238486 [details] [review] Patch to fix weird scaling This is a patch that ensures, that left < right and top < bottom when importing pdf documents via Poppler. This adresses the "weird scaling" for FitR targets where top/bottom are swapped.
Created attachment 238487 [details] [review] Center FitR targets within viewport This patch actually centers the /FitR targets. Note that this patch is against evince 3.4.0.
Created attachment 238488 [details] [review] Patch to center the FitR-Targets This is the patch for actually centering the /FitR targets. Note that this is against Evince 3.4.0.
Review of attachment 238488 [details] [review]: Sorry, this accidentally was posted twice - the first time my browser stalled...
Created attachment 238500 [details] [review] Center FitR targets within viewport Updated patch to be applied in master. It seems git am got gests confused with the line numbers and similar functions.
Created attachment 238501 [details] Backtrace full The bug exists and the patches fix the bug. As a side effect, it makes another bug reproducible (I have hit the bug randomly, but I could not reproduce it). I am pasting the backtrace here just in case. I can reproduce by: - Opening the PDF test case attached in this bug. - F9 to show the sidebar - Show 'Index' in the sidebar - Navigate in the following order: a) Red, Green, Blue, Green, Red, Green, Blue (crash). After clicking the last Green, the output in the console is: or: b) Red, Green, Blue, Red buggy, Green buggy, Blue buggy, Green buggy, Red buggy, Blue, Green, Red, Green (crash) As you can see in the backtrace, before the segmentation fault, the output in the console is: EvinceDocument-CRITICAL **: ev_link_get_action: assertion `EV_IS_LINK (self)' failed FWIW, I did not open another bug, because with this patches I can reproduce the bug reliably.
FWIW, before filing this bug Simon asked on IRC why a document was rendered too small when using the index. Then, he cooked a test case. The document is: http://www.home.unix-ag.org/simon/files/STM32F4Discovery-schematics.pdf
Review of attachment 238500 [details] [review]: LGTM ::: libview/ev-view.c @@ +1605,2 @@ top = ev_link_dest_get_top (dest, &change_top); + bottom = ev_link_dest_get_bottom (dest); Instead of adding, right and bottom we could add directly doc_width and doc_height which is what we want for both calculating the zoom and centering. I think it makes de code easier to follow. doc_with = ev_link_dest_get_right (dest) - left; doc_height = ev_link_dest_get_bottom (dest) - top; @@ +1614,3 @@ ev_document_model_set_scale (view->model, zoom); + left -= (allocation.width / zoom - (right - left)) / 2; Please add a comment here saying this is to center the destination rect
Created attachment 238638 [details] [review] New version of the patches (basically adding comments) Here is a new version of the two patches (this time in a single file as mbox-formatted patches). Since I am not very familiar with the codebase I'll leave the doc_width/height suggestions to someone else - I have no idea of what the implications of such a change would be.
oh, the patches are against evince master HEAD btw.
Oh, missed that these patches have not been pushed to git master. Since they have the approval of Carlos, I just added the doc_width, doc_height variables that Carlos suggested in the review, and pushed both patches to git master. Simon, thank for the patches! This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Review of attachment 238638 [details] [review]: pushed!
Pushed to stable branch too, thanks!