Implemented feature request to find from console.
BUG: 362038
Details
Details
- Reviewers
aacid - Group Reviewers
Okular - Commits
- R223:716b234100ca: Implemented find function from console
Diff Detail
Diff Detail
- Repository
- R223 Okular
- Branch
- arcpatch-D18144
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 6932 Build 6950: arc lint + arc unit
| part.cpp | ||
|---|---|---|
| 1724–1729 | Is this some bad practices? I'm sending as ID for search 1, since it's the first search, fromStart is set as false, since the document could be in another page. Change viewport is set as 1, since it has to change. Everything else I set default values, one problem is that I can't continue the search, I just find the first ocurrence. | |
| shell/shellutils.cpp | ||
|---|---|---|
| 85 | Can this be written as return QStringLiteral("%1:%2:%3:%4:%5:%6:%7").arg(startInPresentation, showPrintDialog, showPrintDialogAndExit, unique, noRaise, page, find);? | |
Comment Actions
one last change to avoid repeating code. I guess it's ready to land, just need a reviewer.
Comment Actions
Using query is a bad idea, now if i actually open a file that has a query in it, it opens the find bar for no reason
okular https://bugs.freedesktop.org/attachment.cgi?id=140739
Comment Actions
Doesn't work when opening files from the network, i.e. do
okular https://bugs.freedesktop.org/attachment.cgi?id=140739 --find poppler
and you'll see that poppler is not highlighted, while if you save the file locally and do the same it'll work
Comment Actions
It seems that it tries to search before copying the file, so I'm searching an empty string. Let me see what I can do to improve that. Maybe wait until the document is completely copied?
| part.cpp | ||
|---|---|---|
| 1715 ↗ | (On Diff #49190) | Tried this, didn't work for network files |
| part.cpp | ||
|---|---|---|
| 1715 ↗ | (On Diff #49190) | Right, you actually want it in openFile() not in openUrl, sorry :) |
Comment Actions
Please make sure you configure your text editor to not change whitespaces, there's lots of unneeded whitespace changes that we don't want.
| part.cpp | ||
|---|---|---|
| 1602 ↗ | (On Diff #49190) | just move the code of the function here, it's three lines and makes it clearer it's something we only do here. |
| 3634 ↗ | (On Diff #49190) | This won't matter since i asked you to move m_textToFindOnOpen to be just a QString, but ehre you had a bug, you needed to reset m_textToFindOnOpen to nullptr after delete; |
| part.h | ||
| 413 ↗ | (On Diff #49497) | This should just be a QString, not a QString *, set to it QString() when clearing it for "already searched" |
| shell/shellutils.cpp | ||
| 85 ↗ | (On Diff #49190) | There is a problem here when find contains a : that is, if you do okular https://bugs.freedesktop.org/attachment.cgi?id=140739 -find "ate: " So you need to encode it here and deserialize it properly on unserializeOptions |
| ui/findbar.cpp | ||
| 200 ↗ | (On Diff #49190) | This doesn't really have much to do with findTextOnStartup, other than "it's the only place it's used for now" But the function should be something more like "startSearch" |
Comment Actions
Using ; as a separator works, but only so far we don't need to add another parameter there, then it'll be a problem.
So i would prefer if you used QByteArray toBase64/fromBase64 on the string, this way we can continue using : as delimiters and all works fine.
Does that make sense to you?
Comment Actions
Do you think you could create an autotest for this? If not it's fine, but would make sure things don't break in the future.
| shell/shell.cpp | ||
|---|---|---|
| 684 ↗ | (On Diff #50806) | the fromBase64 should be in ShellUtils itself, the serialization/unserialization is what that "class" does, should not leak outside. |
Comment Actions
I don't know a lot about autotests, therefore I'm not so sure that it is implemented correctly. I can implement it, but I would need some assistance.
Comment Actions
Your test isn't testing anything, you added new rows, but you're not doing anything with them.
The test should be doing something like, if there's a find string, check that the text was actually searched (and found if it's on the file)
Comment Actions
Added two tests.
First one tests if the findBar was opened and second tests the decryption
Comment Actions
It seems I can't get the text on the findBar widget, so I can't test if the text there is equivalent to the intended.
If it was found or not is not imperative to the test, since this is tested on the searchtext.cpp
Comment Actions
Ah, you got bitten by findbar not being visible to the test.
I'll fix the test in a commit just after landing this, have a look in you're interested in the creative solution.