[Okular-devel] Review Request 115759: Fix for Bug 326207

[Jaan Vajakas]

> On Feb. 15, 2014, 3:13 p.m., Albert Astals Cid wrote:
> > core/textpage.cpp, line 790
> > <https://git.reviewboard.kde.org/r/115759/diff/1/?file=244161#file244161line790>
> >
> >     why this change?

As I wrote, the reason is that I think it is not good that layout analysis depends on the user's display DPI (m_page->height() is the height in pixels at 100% zoom, which depends on the DPI), since it makes no sense but would make testing more difficult (then testing layout detection would need to be done for several display DPI values separately and bug reports should include the DPI to be reproducible).

Actually here it would be most accurate to compute the y-overlap directly from the NormalizedRect's without scaling and rounding them to QRect's at all (and similarly rounding could be avoided in many other places in textpage.cpp), but I was too lazy to refactor the code and instead used a large constant for pageHeight to minimize rounding errors.


> On Feb. 15, 2014, 3:13 p.m., Albert Astals Cid wrote:
> > core/textpage.cpp, line 1873
> > <https://git.reviewboard.kde.org/r/115759/diff/1/?file=244161#file244161line1873>
> >
> >     Can you explain this rationale?
> >     
> >     Also any reason you decided to write 2E3 instead 2000?

Again I got rid of the DPI dependency. I chose the value 2000 because A4 paper at 72 dpi (the DPI used by Okular before the libkscreen patch) is 597.6 x 842.4 pixels, and I rounded 597.6 + 842.4 = 1440.0 up to 2000. Higher pageWidth/pageHeight values increase CPU usage (linearly in pageWidth+pageHeight) but I guess increasing the values generally doesn't hurt layout analysis quality; too low pageWidth/pageHeight values definitely cause bad layout analysis quality. And here the width/height ratio is important too, unlike the y-overlap case above that only used y coordinates.

I wrote 2E3 because it is shorter than 2000.0. I thought that 2E3 or 2000.0 would be better than 2000 — then it is obvious from first glance that this is not integer division and also the expression would work even if Page::width() and Page::height() were refactored to return int. But perhaps 2000.0 would be clearer indeed.


> On Feb. 15, 2014, 3:13 p.m., Albert Astals Cid wrote:
> > tests/searchtest.cpp, line 64
> > <https://git.reviewboard.kde.org/r/115759/diff/1/?file=244162#file244162line64>
> >
> >     please use kDebug and remove the iostream include

Actually I wanted something like QCOMPARE or an exception, so that the test were halted and error message shown. But QCOMPARE unfortunately doesn't work inside functions that are not tests themselves and when I tried exception throwing I got a compiler error that exceptions are disabled (and I don't know how the Qt test framework would handle them even if they were enabled). Then I chose stderr instead of kDebug since kDebug output can be disabled in kdebugdialog but I thought the test's reason of failure should always be shown (like the QCOMPARE error messages). But this is an unimportant error message anyway (it can only appear if the tests in searchtest.cpp themselves are incorrectly modified), so I don't oppose kDebug either. Is it a good style to never use std::cout and std::cerr, even in tests?


- Jaan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115759/#review49831
-----------------------------------------------------------


On Feb. 15, 2014, 1:08 p.m., Jaan Vajakas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115759/
> -----------------------------------------------------------
> 
> (Updated Feb. 15, 2014, 1:08 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 326207
>     http://bugs.kde.org/show_bug.cgi?id=326207
> 
> 
> Repository: okular
> 
> 
> Description
> -------
> 
> Here is a patch to fix Bug 326207. It was a simple bug in the XY Cut layout recognition code that made it too eager to see columns everywhere. I also removed the dependence of the layout analysis algorithms on the display DPI (introduced by the recently added feature of using KScreen) to make their behavior more predictable and reproducible. I added tests for this bug and also refactored SearchTest a little to use QVectors instead of bare arrays.
> 
> 
> Diffs
> -----
> 
>   core/textpage.cpp a95f7c6 
>   tests/searchtest.cpp caceb6f 
> 
> Diff: https://git.reviewboard.kde.org/r/115759/diff/
> 
> 
> Testing
> -------
> 
> added automatic tests and tested manually on some PDF files
> 
> 
> Thanks,
> 
> Jaan Vajakas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20140215/7e9b1e7c/attachment.html>