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
I included some screenshots, it only changes a box in the UI.
Before:
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
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.
No Linters Available |
No Unit Test Coverage |
Buildable 10388 | |
Build 10406: arc lint + arc unit |
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.
Every patch with UI changes needs screenshots in the Summary section--preferably before-and-after. :)
https://community.kde.org/Infrastructure/Phabricator#Include_some_screenshots
This needs a test for whole word search.
core/textpage.h | ||
---|---|---|
148 | 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. |
I'll fix the binary compatibility issues, I didn't know it would break it.
I'll try and make a test.
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.
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? |
Many thanks in advance for fixing this minor typo.
core/textpage.cpp | ||
---|---|---|
857 | Typo: beggining -> beginning |
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.
I think it has most cases covered, I added a lot of tests which reflect on the new changes.
I'll wait on feedback.
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?
Fixed variable not initalized. Also, now you won't find words split with -, like Oku-\nlar
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"?
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?
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.
core/page.h | ||
---|---|---|
192 | 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 | |
887 | const | |
998 | spaces around = | |
1000 | spaces around + |
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.