Implemented find function from console
ClosedPublic

Authored by joaonetto on Jan 9 2019, 11:56 PM.

Details

Summary

Implemented feature request to find from console.

BUG: 362038

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
joaonetto created this revision.Jan 9 2019, 11:56 PM
Restricted Application added a project: Okular. · View Herald TranscriptJan 9 2019, 11:56 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
joaonetto requested review of this revision.Jan 9 2019, 11:56 PM
joaonetto updated this revision to Diff 49125.Jan 9 2019, 11:59 PM

Deleted iostream used in main for debugging

joaonetto added inline comments.Jan 10 2019, 12:04 AM
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.

yurchor added inline comments.
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);

?

Tried that, not possible, it gets me compilation error.

Tried that, not possible, it gets me compilation error.

Ooops... Sorry.

Otherwise, I can confirm that the patch works as expected.

part.cpp
1730

Trailing spaces

shell/shellutils.cpp
62

Trailing spaces.

joaonetto updated this revision to Diff 49190.Jan 10 2019, 7:41 PM

Using command find from terminal now opens FindBar

joaonetto updated this revision to Diff 49193.Jan 10 2019, 7:48 PM

one last change to avoid repeating code. I guess it's ready to land, just need a reviewer.

aacid added a subscriber: aacid.Jan 10 2019, 9:14 PM

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

joaonetto updated this revision to Diff 49206.Jan 10 2019, 10:36 PM

Changed the code so I no longer use query.

aacid added a comment.Jan 11 2019, 9:41 PM

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

joaonetto marked an inline comment as done.Jan 11 2019, 10:43 PM

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?

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?

Yes you need to search after the document has been opened.

joaonetto updated this revision to Diff 49324.Jan 12 2019, 1:17 PM

Made changes that allows file to be searched only when loaded

joaonetto updated this revision to Diff 49325.Jan 12 2019, 1:20 PM

Deleted remains and formatting issues

aacid added inline comments.Jan 12 2019, 6:21 PM
part.cpp
378 ↗(On Diff #49190)

You don't need this, just do it after open.

1715 ↗(On Diff #49190)

That is, move the code from findTextOnStartUp here, also clean up the textToFind variable.

part.h
413 ↗(On Diff #49325)

Needs m_ prefix.

And also a better name, something m_textToFindOnOpen.

joaonetto added inline comments.Jan 12 2019, 6:40 PM
part.cpp
1715 ↗(On Diff #49190)

Tried this, didn't work for network files

joaonetto updated this revision to Diff 49349.Jan 12 2019, 7:34 PM

Changed variable names and deleted QString after use

joaonetto updated this revision to Diff 49420.Jan 14 2019, 2:05 AM

Not initializing the variable was causing crashes. Fixed it

aacid added inline comments.Jan 14 2019, 11:19 PM
part.cpp
1715 ↗(On Diff #49190)

Right, you actually want it in openFile() not in openUrl, sorry :)

joaonetto updated this revision to Diff 49496.Jan 14 2019, 11:55 PM

Deleted signal, now I call the function after the file has been open

joaonetto updated this revision to Diff 49497.Jan 15 2019, 12:05 AM

Code cleanup, only important parts should be changed

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: "
it will fail because when it goes to deserialize it it has "too many" :

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"

joaonetto updated this revision to Diff 49685.Jan 17 2019, 3:56 AM

Fixed bug when searching for strings with : and some cleanup

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?

joaonetto updated this revision to Diff 50806.Feb 3 2019, 8:34 PM

Changed find String to a encrypted base64 string.

aacid added a comment.Feb 6 2019, 9:15 PM

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.

joaonetto updated this revision to Diff 51070.Feb 7 2019, 12:10 AM

Changed decrypt to shellUtils. Implemented a test

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.

aacid added a comment.Feb 7 2019, 12:34 AM

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.

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)

joaonetto updated this revision to Diff 51229.Feb 8 2019, 11:04 PM

Added two tests.
First one tests if the findBar was opened and second tests the decryption

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

aacid accepted this revision.Feb 13 2019, 10:59 PM

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.

This revision is now accepted and ready to land.Feb 13 2019, 10:59 PM
aacid closed this revision.Feb 13 2019, 11:00 PM