Bug 24457 - [Qt] Extend Qtish API with functionality of finding all occurences of particular phrase and highlighting them.
Summary: [Qt] Extend Qtish API with functionality of finding all occurences of particu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-08 10:51 PDT by Jakub Wieczorek
Modified: 2009-06-25 17:24 PDT (History)
1 user (show)

See Also:


Attachments
patch (6.32 KB, patch)
2009-03-08 10:53 PDT, Jakub Wieczorek
hausmann: review-
Details | Formatted Diff | Diff
patch r2 (1.96 KB, patch)
2009-04-11 12:49 PDT, Jakub Wieczorek
no flags Details | Formatted Diff | Diff
patch r3 (4.75 KB, patch)
2009-04-29 10:14 PDT, Jakub Wieczorek
hausmann: review-
Details | Formatted Diff | Diff
patch r4 (4.82 KB, patch)
2009-06-25 11:07 PDT, Jakub Wieczorek
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Wieczorek 2009-03-08 10:51:18 PDT
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.
Comment 1 Jakub Wieczorek 2009-03-08 10:53:15 PDT
Created attachment 28409 [details]
patch
Comment 2 Simon Hausmann 2009-03-27 06:11:43 PDT
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?
Comment 3 Jakub Wieczorek 2009-03-28 05:50:59 PDT
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.
Comment 4 Jakub Wieczorek 2009-04-11 12:48:10 PDT
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.
Comment 5 Jakub Wieczorek 2009-04-11 12:49:09 PDT
Created attachment 29416 [details]
patch r2
Comment 6 Jakub Wieczorek 2009-04-29 10:14:27 PDT
Created attachment 29881 [details]
patch r3
Comment 7 Simon Hausmann 2009-06-25 08:22:46 PDT
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."
Comment 8 Simon Hausmann 2009-06-25 08:42:59 PDT
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 :)
Comment 9 Jakub Wieczorek 2009-06-25 11:07:45 PDT
Created attachment 31866 [details]
patch r4

Enhanced the documentation.
Comment 10 Adam Treat 2009-06-25 11:14:28 PDT
Comment on attachment 31866 [details]
patch r4

Looks good. 
r=me
Comment 11 Eric Seidel (no email) 2009-06-25 17:24:44 PDT
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