> 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? > > Jaan Vajakas wrote: > 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.
Lazyness is seldom a good excuse ;-) Please do the computation of NormalizedRects if you think that makes more sense. > 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? > > Jaan Vajakas wrote: > 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. 2000.0 is indeed easier to read. Please change it :-) > 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 > > Jaan Vajakas wrote: > 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? We just don't use iostreams much. If you need QCOMPARE just make the function non static and bring it to the SearchTest object as non slot (so it's not a test itself) and it should work. - Albert ----------------------------------------------------------- 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 > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
