Bug 24457 – [Qt] Extend Qtish API with functionality of finding all occurences of particular phrase and highlighting them.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jakub Wieczorek]

I've added a new method: findAllTextOccurences() with identical arguments as findText() has. It highlights all occurences of provided text, which are on the page.

I've also extended QWebPage::FindFlags with DontHighlightMatches flag, which used along with above mentioned function will make it only return the number of matches, without highlighting them.

One can clear the highlight through the resetTextHighlight() method.

Patch attached.

[Jakub Wieczorek]

Created attachment 28409 [details]
patch

[Simon Hausmann]

Comment on attachment 28409 [details]
patch

I think it would be better to not add a new method but to add FindAllOccurrences to the FindFlags instead.

Instead of adding resetTextHighlight(), what do you think about caling unmarkAllTextMatches() if findText() is called with an empty string?

[Jakub Wieczorek]

Originally I thought about just extending the findText() method with support for multiple occurences highlight but what made me actually create new method is the matches count that I wanted also to be available. Do you have any idea?

Resetting the highlight by passing an empty string seemed initially a bit unintuitive but still looks like a better idea than seperate method.

I'll rework my patch.

[Jakub Wieczorek]

Here is the revised version of my patch.

As suggested, I've reused the findText() function for multiple occurences functionality. To clear highlight an empty string needs to be passed as suggested as well.

If we are reusing the findText() method then I thought one should still be able to combine all of them. But to make it happen we'd need one more flag which is a FindForwards to distinguish whether we want to highlight only or highlight and select the next occurence.

Another thing I considered is providing the number of highlighted matches. But for now the findText() method returns a bool. It would be good to change it to integer in the next major version of Qt. Then in highlight mode it would return number of occurences and in select mode just 1 or 0.

[Jakub Wieczorek]

Created attachment 29416 [details]
patch r2

[Jakub Wieczorek]

Created attachment 29881 [details]
patch r3

[Simon Hausmann]

Two comments from manyoso from IRC:

<manyoso> If the HighlightAllOccurences flag is not passed, the function will select an occurrence and all subsequent calls will replace the current occurrence with the next one."
[17:19] <-- fabc has left this channel.
[17:20] <manyoso> and I'd suggest, "To clear the selection, just pass an empty string."

[Simon Hausmann]

Comment on attachment 29881 [details]
patch r3

Jakub, the patch looks great. Please include Manyoso's suggestions for the documentation. R- because of that, but then I think the patch is ready to go in!

Good stuff :)

[Jakub Wieczorek]

Created attachment 31866 [details]
patch r4

Enhanced the documentation.

[Adam Treat]

Comment on attachment 31866 [details]
patch r4

Looks good. 
r=me

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/qt/Api/qwebpage.cpp
	M	WebKit/qt/Api/qwebpage.h
	M	WebKit/qt/Api/qwebview.cpp
	M	WebKit/qt/ChangeLog
Committed r45220
http://trac.webkit.org/changeset/45220