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

[Jaan Vajakas]

> 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?
> 
> Albert Astals Cid wrote:
>     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.
> 
> Jaan Vajakas wrote:
>     Now in revision r2 I use QCOMPARE. Actually from my tests with Qt 4.8.5 it seemed there is no difference in the behavior of QCOMPARE, whether it is called from a class member function or a global function: in either case it prints an error message and returns from the function, but does not abort the calling test function (i. e. does not behave like throwing an exception). But either way this is undocumented behavior, as the documentation ( http://qt-project.org/doc/qt-4.8/qtest.html#QCOMPARE ) says "Note: This macro can only be used in a test function that is invoked by the test framework.". So actually now I am thinking that perhaps it would be better if I refactor my function into a macro – then QCOMPARE would be used in accordance with the documentation.

Now in diff r3 I refactored searchtest.cpp so that QCOMPARE is used in accordance with the Qt documentation.


- Jaan


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


On Feb. 22, 2014, 2:51 p.m., Jaan Vajakas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115759/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2014, 2:51 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/20140222/7b2e2f5b/attachment-0001.html>