Added option to search for whole words only
Needs RevisionPublic

Authored by joaonetto on Feb 18 2019, 6:25 PM.

Details

Reviewers
aacid
Group Reviewers
Okular
Summary

I included some screenshots, it only changes a box in the UI.

Before:


After:

This should work for most cases.

Still some problems with words like para-medic. Searching for para or medic would return a result.

BUG: 204709

Test Plan

Open file
Search string, should find anything.
Mark whole words.
Search string, should find only whole words.
Close Okular
Open Okular.
Search should be marked with whole words and only find whole words.

Diff Detail

Repository
R223 Okular
Branch
arcpatch-D19123_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11449
Build 11467: arc lint + arc unit
joaonetto created this revision.Feb 18 2019, 6:25 PM
Restricted Application added a project: Okular. · View Herald TranscriptFeb 18 2019, 6:25 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
joaonetto requested review of this revision.Feb 18 2019, 6:25 PM
joaonetto updated this revision to Diff 51988.Feb 18 2019, 6:30 PM

Updated comment on SearchText

This works properly for most cases, but still, it has some limitations.
If searching for r on a text, and the text contains R$, the R will be found.
Every word that has a space behind and a symbol after, or a symbol before and space after, or symbol before and after, will be found.
I guess that's a good trade-off, but we can work to improve it, if wanted.

ngraham added a subscriber: ngraham.EditedFeb 18 2019, 7:33 PM

Every patch with UI changes needs screenshots in the Summary section--preferably before-and-after. :)

https://community.kde.org/Infrastructure/Phabricator#Include_some_screenshots

aacid added a subscriber: aacid.Feb 18 2019, 8:25 PM

This needs a test for whole word search.

core/textpage.h
151

This breaks binary compatibility https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

What you want is adding a new function with the new param and make the old one call the new.

You have the same problem in the core/*.h files.

joaonetto edited the summary of this revision. (Show Details)Feb 18 2019, 10:40 PM

I'll fix the binary compatibility issues, I didn't know it would break it.

I'll try and make a test.

joaonetto updated this revision to Diff 52334.Feb 22 2019, 7:18 PM

Fixing binary compability issues

If I understood well, this should fix the binary compability issues.

As far as I can say about the text, I'm figuring out how to create a page with the macro, when I do, I'll diff the tests.

joaonetto updated this revision to Diff 52404.Feb 23 2019, 8:48 PM

Added some tests to validate the diff, and fixed a bug.

aacid added inline comments.Feb 26 2019, 11:36 PM
autotests/searchtest.cpp
369

Seems like using a _data function here so that "test data" and "test code" are separated may be a good idea.

Can you please investigate?

joaonetto updated this revision to Diff 52697.Feb 27 2019, 2:35 AM

Removed useless code, remodeled tests and fixed corner case

joaonetto edited the summary of this revision. (Show Details)Feb 27 2019, 2:43 AM

Many thanks in advance for fixing this minor typo.

core/textpage.cpp
857

Typo: beggining -> beginning

joaonetto updated this revision to Diff 52724.Feb 27 2019, 1:33 PM
joaonetto marked an inline comment as done.

Fixed typo

joaonetto retitled this revision from Added option to search for whole words only to [WIP]Added option to search for whole words only.Mar 2 2019, 10:49 AM
joaonetto edited the summary of this revision. (Show Details)

This works properly for most cases, but still, it has some limitations.
If searching for r on a text, and the text contains R$, the R will be found.
Every word that has a space behind and a symbol after, or a symbol before and space after, or symbol before and after, will be found.
I guess that's a good trade-off, but we can work to improve it, if wanted.

This sounds optimal to me, not like a trade-off. Or how could it be improved?

By requiring whitespace on both sides, BAUD would not find USART0.BAUD, and argv would not find char *argv[].

This works properly for most cases, but still, it has some limitations.
If searching for r on a text, and the text contains R$, the R will be found.
Every word that has a space behind and a symbol after, or a symbol before and space after, or symbol before and after, will be found.
I guess that's a good trade-off, but we can work to improve it, if wanted.

This sounds optimal to me, not like a trade-off. Or how could it be improved?

By requiring whitespace on both sides, BAUD would not find USART0.BAUD, and argv would not find char *argv[].

Well, in this version of the patch it would, since I'm checking if the char before/after is letter or number. Searching for argv or BAUD would return a result.

joaonetto updated this revision to Diff 53073.Mar 3 2019, 5:38 PM

Fixed word being in the same textEntity and added some new tests.

joaonetto retitled this revision from [WIP]Added option to search for whole words only to Added option to search for whole words only.Mar 3 2019, 5:39 PM
joaonetto edited the summary of this revision. (Show Details)
joaonetto marked 2 inline comments as done.Mar 3 2019, 5:45 PM

I think it has most cases covered, I added a lot of tests which reflect on the new changes.
I'll wait on feedback.

aacid requested changes to this revision.Mar 31 2019, 10:43 PM

Something needs fixing

==27223== Conditional jump or move depends on uninitialised value(s)
==27223==    at 0xFA1013F: Okular::TextPagePrivate::findTextInternalForward(int, QString const&, bool (*)(QStringRef const&, QStringRef const&), QList<TinyTextEntity*>::const_iterator const&, int, QList<TinyTextEntity*>::const_iterator const&, bool) (textpage.cpp:993)
==27223==    by 0xFA0F53A: Okular::TextPage::findText(int, QString const&, Okular::SearchDirection, Qt::CaseSensitivity, Okular::RegularAreaRect const*, bool) (textpage.cpp:768)
==27223==    by 0xF9F5FD3: Okular::Page::findText(int, QString const&, Okular::SearchDirection, Qt::CaseSensitivity, Okular::RegularAreaRect const*, bool) const (page.cpp:310)
==27223==    by 0xF9A87F3: Okular::DocumentPrivate::doContinueDirectionMatchSearch(void*) (document.cpp:1673)
==27223==    by 0xF9BF4E9: Okular::Document::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_document.cpp:370)
==27223==    by 0x643A101: QObject::event(QEvent*) (in /usr/lib/libQt5Core.so.5.12.2)
==27223==    by 0x5632E23: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.12.2)
==27223==    by 0x563A5F0: QApplication::notify(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.12.2)
==27223==    by 0x640EDF8: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib/libQt5Core.so.5.12.2)
==27223==    by 0x6411EE7: QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (in /usr/lib/libQt5Core.so.5.12.2)
==27223==    by 0x64656C3: ??? (in /usr/lib/libQt5Core.so.5.12.2)
==27223==    by 0x81937BE: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6000.0)

This is valgrind tell us there's something uninitalized being used, what i did to get this

valgrind okular
open file
Press Ctrl+F
type something on the search bar

the "find whole words" was not checked because that's the state it originally was on starting okular.

Can you please try to fix this?

This revision now requires changes to proceed.Mar 31 2019, 10:43 PM
joaonetto updated this revision to Diff 55324.Apr 3 2019, 3:26 AM

Fixed variable not initalized. Also, now you won't find words split with -, like Oku-\nlar

joaonetto added a comment.EditedApr 3 2019, 3:33 AM

I wasn't initializing m_wholeWords in searchLineEdit, that was causing it. Hope it works now.

Also ran ctest, worked fine.

Let me know if something is wrong.

Is "Find Whole Words Only" too big? Should it be only "Whole Words"?

joaonetto edited the test plan for this revision. (Show Details)Apr 3 2019, 3:35 AM
aacid added a comment.Apr 3 2019, 9:26 PM

Not sure if i'm reading the tests code wrong, but shouldn't

QTest::newRow("albert test") << (QVector<QString>() << QStringLiteral("it") << QStringLiteral(" should find this afind")) << QStringLiteral("find") << TEST_NEXT_AND_PREV;

pass?

joaonetto updated this revision to Diff 55432.Apr 4 2019, 5:52 PM
  • Bug fixing. Also, fixed some things I'd not seen before

Yes, it should. I had some oversights that I hope are fixed now.

Added two new tests to test them, one with endsWith('\n') too see if I'm in the last char and yours.

aacid added inline comments.Apr 21 2019, 11:01 PM
core/page.h
194

Both functions need to have proper documentation. And the new one needs a since marker (same for all the other "public" functions) in core/*.h you added/changed

And the "to be merged" should be marked with a TODO so maybe we see it when we decided to break BC

core/textpage.cpp
850

can this function be marked as const? seems like it could/should

858

add {} if you're going to have an else with {}

875

Make this const
Are we sure this is never going to be 0? I had a look at the strignLengthAdapatedWithHyphens and i'm not 100% sure :D

887

const

1025

spaces around =

1027

spaces around +

joaonetto updated this revision to Diff 57388.May 2 2019, 5:34 PM
joaonetto marked 5 inline comments as done.

Added documentation

joaonetto marked an inline comment as done.May 2 2019, 5:36 PM
joaonetto added inline comments.
core/page.h
194

Done the TODO part.
Is this documentation all right?
If not, can you explain what should I be looking for defining?

core/textpage.cpp
875

Only if the tiny text entity is "-\n", I made it to see to make sure.

aacid requested changes to this revision.Feb 21 2020, 6:53 PM

Please move as a Merge Request in https://invent.kde.org/kde/okular

We have pre-commit CI and lots of checks including clazy and clang-tidy there so it's a much better place for doing the review/approval/merge of the code.

This revision now requires changes to proceed.Feb 21 2020, 6:53 PM