Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/#review41948 --- This review has been submitted with commit dff8bf1b365f28c6a7b7d97e8ca2a0e579738619 by Albert Astals Cid on behalf of Jaan Vajakas to branch master. - Commit Hook On Oct. 5, 2013, 8:20 p.m., Jaan Vajakas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Oct. 5, 2013, 8:20 p.m.) Review request for Okular. Bugs: 323262 and 323263 http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Repository: okular Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) Diffs - core/textpage_p.h 8ecf0c9 core/textpage.cpp 855942d tests/searchtest.cpp 495107d Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Oct. 18, 2013, 2:44 p.m.) Status -- This change has been marked as submitted. Review request for Okular. Bugs: 323262 and 323263 http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Repository: okular Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) Diffs - core/textpage_p.h 8ecf0c9 core/textpage.cpp 855942d tests/searchtest.cpp 495107d Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/#review41949 --- Thanks a lot for the patch Jaan, it'll be available in 4.12.0 If you have interest in other search related bugs you can have a look at https://bugs.kde.org/show_bug.cgi?id=326207 ;-) - Albert Astals Cid On Oct. 18, 2013, 2:44 p.m., Jaan Vajakas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Oct. 18, 2013, 2:44 p.m.) Review request for Okular. Bugs: 323262 and 323263 http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Repository: okular Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) Diffs - core/textpage_p.h 8ecf0c9 core/textpage.cpp 855942d tests/searchtest.cpp 495107d Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Oct. 5, 2013, 8:20 p.m.) Review request for Okular. Changes --- I updated the tests so that no new external files are needed (I gave up testing Document::searchText and contended myself with testing TextPage::findText only, since I have not changed the Document class; so I could easily use synthetic test pages). By the way, my tests cover Bug 309030 already (testHyphenAtEndOfPage). Bugs: 323262 and 323263 http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Repository: okular Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) Diffs (updated) - core/textpage_p.h 8ecf0c9 core/textpage.cpp 855942d tests/searchtest.cpp 495107d Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
On Sept. 25, 2013, 9 p.m., Albert Astals Cid wrote: core/textpage.cpp, line 120 http://git.reviewboard.kde.org/r/112135/diff/2/?file=185753#file185753line120 This is an unrelated fix, right? Yes, I noticed it by reviewing the code. I have also added a test for it (testHyphenWithYOverlap). - Jaan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/#review40819 --- On Oct. 5, 2013, 8:20 p.m., Jaan Vajakas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Oct. 5, 2013, 8:20 p.m.) Review request for Okular. Bugs: 323262 and 323263 http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Repository: okular Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) Diffs - core/textpage_p.h 8ecf0c9 core/textpage.cpp 855942d tests/searchtest.cpp 495107d Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/#review40819 --- Thanks and sorry for the delay in review, was away in holiday About if using real files or syntetic textpages, if not very hard, i think it'd be cooler to use syntetic textpages, since that way we can add tests for stuff like bugs 311232, 309030, etc. Do you think you can have a look (no need to add the bugs i mention, but it's probably easier to add more tests in the future if we have the syntetic textpages thing than adding more and more documents to git, which is big enough already) core/textpage.cpp http://git.reviewboard.kde.org/r/112135/#comment3 This is an unrelated fix, right? - Albert Astals Cid On Aug. 31, 2013, 6:13 a.m., Jaan Vajakas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Aug. 31, 2013, 6:13 a.m.) Review request for Okular. Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) This addresses bugs 323262 and 323263. http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Diffs - core/textpage.cpp 855942d core/textpage_p.h 8ecf0c9 tests/data/a_ba_b.djvu PRE-CREATION tests/data/abab.pdf PRE-CREATION tests/data/abababa.pdf PRE-CREATION tests/searchtest.cpp 495107d Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- File Attachments tests/data/a_ba_b.djvu http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/a_ba_b.djvu tests/data/abab.pdf http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/abab.pdf tests/data/abababa.pdf http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/abababa.pdf Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/#review38076 --- Nice, still haven't had a look at the code, will try find some time for it next week. What would be awesome is if you could add some autotests (we are trying to get more and more of those) to make sure that we don't break stuff in the future when changing some other things. In this case should not be too hard creating some TextPagePrivates and calling setWordList on them and then checking that findTextInternalForward/findTextInternalBackward return the correct values. Can you try to do that? - Albert Astals Cid On Aug. 17, 2013, 7:48 p.m., Jaan Vajakas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Aug. 17, 2013, 7:48 p.m.) Review request for Okular. Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) This addresses bugs 323262 and 323263. http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Diffs - core/textpage.cpp 855942d core/textpage_p.h 8ecf0c9 Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
On Aug. 18, 2013, 3:28 p.m., Albert Astals Cid wrote: Nice, still haven't had a look at the code, will try find some time for it next week. What would be awesome is if you could add some autotests (we are trying to get more and more of those) to make sure that we don't break stuff in the future when changing some other things. In this case should not be too hard creating some TextPagePrivates and calling setWordList on them and then checking that findTextInternalForward/findTextInternalBackward return the correct values. Can you try to do that? Sounds a very good idea. I will try to do it. - Jaan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/#review38076 --- On Aug. 17, 2013, 7:48 p.m., Jaan Vajakas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- (Updated Aug. 17, 2013, 7:48 p.m.) Review request for Okular. Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) This addresses bugs 323262 and 323263. http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Diffs - core/textpage.cpp 855942d core/textpage_p.h 8ecf0c9 Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Review Request 112135: Fix for Bug 323262 and Bug 323263
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112135/ --- Review request for Okular. Description --- This patch solves Bug 323262 and Bug 323263. I also refactored and simplified the code a little. By removing unnecessary calls to toLower in TextPagePrivate::findTextInternalForward and TextPagePrivate::findTextInternalBackward I also fixed a small bug: the letter capital I with dot above (U+0130) did not match itself in case-insensitive mode on Qt 4.8.4 (U+0130 still does not match lowercase i (U+0069), which can be considered another bug, that I didn't fix (although this behavior conforms to the Unicode case folding rules)). (I did not implement the Knuth-Morris-Pratt algorithm that I promised in a comment of Bug 323263 because on second thought I find that the win, if any, would probably be negligible except for some very special documents and special query strings.) This addresses bugs 323262 and 323263. http://bugs.kde.org/show_bug.cgi?id=323262 http://bugs.kde.org/show_bug.cgi?id=323263 Diffs - core/textpage.cpp 855942d core/textpage_p.h 8ecf0c9 Diff: http://git.reviewboard.kde.org/r/112135/diff/ Testing --- Thanks, Jaan Vajakas ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
