Implemented find function from console
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Okular
Summary

Implemented feature request to find from console.

BUG: 362038

Diff Detail

Repository
R223 Okular
Branch
arcpatch-D18144_2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7200
Build 7218: arc lint + arc unit
joaonetto created this revision.Wed, Jan 9, 11:56 PM
Restricted Application added a project: Okular. · View Herald TranscriptWed, Jan 9, 11:56 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
joaonetto requested review of this revision.Wed, Jan 9, 11:56 PM
joaonetto updated this revision to Diff 49125.Wed, Jan 9, 11:59 PM

Deleted iostream used in main for debugging

joaonetto added inline comments.Thu, Jan 10, 12:04 AM
part.cpp
1731

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
80

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
1737

Trailing spaces

shell/shellutils.cpp
62

Trailing spaces.

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

Using command find from terminal now opens FindBar

joaonetto updated this revision to Diff 49193.Thu, Jan 10, 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.Thu, Jan 10, 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.Thu, Jan 10, 10:36 PM

Changed the code so I no longer use query.

aacid added a comment.Fri, Jan 11, 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.Fri, Jan 11, 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.Sat, Jan 12, 1:17 PM

Made changes that allows file to be searched only when loaded

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

Deleted remains and formatting issues

aacid added inline comments.Sat, Jan 12, 6:21 PM
part.cpp
378

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

1722

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

part.h
412

Needs m_ prefix.

And also a better name, something m_textToFindOnOpen.

joaonetto added inline comments.Sat, Jan 12, 6:40 PM
part.cpp
1722

Tried this, didn't work for network files

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

Changed variable names and deleted QString after use

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

Not initializing the variable was causing crashes. Fixed it

aacid added inline comments.Mon, Jan 14, 11:19 PM
part.cpp
1722

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

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

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

joaonetto updated this revision to Diff 49497.Tue, Jan 15, 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

just move the code of the function here, it's three lines and makes it clearer it's something we only do here.

3636

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
412

This should just be a QString, not a QString *, set to it QString() when clearing it for "already searched"

shell/shellutils.cpp
80

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

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.Thu, Jan 17, 3:56 AM

Fixed bug when searching for strings with : and some cleanup