Bug 44252 - QWebView::selectionChanged() is never emitted.
Summary: QWebView::selectionChanged() is never emitted.
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: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-08-19 05:11 PDT by marc
Modified: 2011-04-19 05:15 PDT (History)
7 users (show)

See Also:


Attachments
Proposed fix - apply with patch -p0 (498 bytes, patch)
2010-08-19 05:11 PDT, marc
kling: review-
Details | Formatted Diff | Diff
Proposed fix v2 (with ChangeLog) (1.15 KB, patch)
2010-08-19 23:34 PDT, marc
no flags Details | Formatted Diff | Diff
Proposed fix v3 (with ChangeLog w/o tabs) (1.16 KB, patch)
2010-08-20 00:47 PDT, marc
no flags Details | Formatted Diff | Diff
Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request (2.12 KB, patch)
2010-08-20 05:56 PDT, marc
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description marc 2010-08-19 05:11:34 PDT
Created attachment 64830 [details]
Proposed fix - apply with patch -p0

QWebView has a selectionChanged() signal that is never emitted. I can't find a connect() to it in the source code, and setting a breakpoint on it, and QWebPage::selectionChanged(), the latter is hit while the former is not. It's rather obvious it's been forgotten to be connected :)
Comment 1 WebKit Review Bot 2010-08-19 05:15:26 PDT
Attachment 64830 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Andreas Kling 2010-08-19 08:39:42 PDT
Comment on attachment 64830 [details]
Proposed fix - apply with patch -p0

Looks sane, but you need a ChangeLog entry.
See http://trac.webkit.org/wiki/QtWebKitContrib for more information.
Comment 3 marc 2010-08-19 23:34:20 PDT
Created attachment 64928 [details]
Proposed fix v2 (with ChangeLog)
Comment 4 WebKit Review Bot 2010-08-19 23:38:50 PDT
Attachment 64928 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebview.cpp"
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 marc 2010-08-20 00:47:02 PDT
Created attachment 64932 [details]
Proposed fix v3 (with ChangeLog w/o tabs)
Comment 6 Antonio Gomes 2010-08-20 05:00:49 PDT
Comment on attachment 64932 [details]
Proposed fix v3 (with ChangeLog w/o tabs)

Looks ok. Please do the same for qgraphicswebview.cpp
Comment 7 marc 2010-08-20 05:44:03 PDT
QGraphicsWebView doesn't _have_ a selectionChanged() signal.
Maybe it should have one, but this report is about an _existing_ signal not being emitted.
Comment 8 marc 2010-08-20 05:56:19 PDT
Created attachment 64947 [details]
Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request
Comment 9 Kenneth Rohde Christiansen 2010-08-20 06:00:28 PDT
Comment on attachment 64947 [details]
Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request

Shouldn't you add the selectedText method as well? Please make sure that there are no other important selection methods missing?

Do we really want all these methods from QWebPage everywhere? Simon?
Comment 10 Antonio Gomes 2010-08-20 09:30:38 PDT
> Do we really want all these methods from QWebPage everywhere? Simon?

Good point. We possibly do not need them all connected to our view's. 

@marc, can not you make use of qwebpage's selectionChanged signal for your needs? If so, we could make it wont fix.

Btw, if we decide to go for it , you should combine both you previous patch with your newer patch in one.
Comment 11 marc 2010-08-21 13:43:09 PDT
> @marc, can not you make use of qwebpage's selectionChanged signal for your
> needs? If so, we could make it wont fix.

The point of the original report was that QWebView::selectionChanged() _which exists and is documented_ is never emitted. You cannot remove that signal due to binary compatibility. You can, of course, obsolete it, but that's not my call to make (and not my task to implement). Either way, the solution can't be wontfix.

The second patch was a request from you guys. Take it or leave it, I don't care either way :)

> Btw, if we decide to go for it , you should combine both you previous
> patch with your newer patch in one.

Sorry, I've been through enough iterations of this patch already :)
Comment 12 Antonio Gomes 2010-08-22 16:15:28 PDT
Comment on attachment 64932 [details]
Proposed fix v3 (with ChangeLog w/o tabs)

(In reply to comment #11)
> The point of the original report was that QWebView::selectionChanged() _which exists and is documented_ is never emitted. You cannot remove that signal due to binary compatibility. You can, of course, obsolete it, but that's not my call to make (and not my task to implement). Either way, the solution can't be wontfix.

Ok, I was wrong. r=me on your 3rd patch.

> The second patch was a request from you guys. Take it or leave it, I don't care either way :)

I do not think we need to have this signal to QGraphicsWebView. At least not without a public discussion on the mainling list about.

So lets go for your first approach.
Comment 13 Antonio Gomes 2010-08-22 16:17:08 PDT
Comment on attachment 64947 [details]
Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request

Clearing revie on this patch, since I blindly requested for it.

If you still want that, lets discuss in the mailing list first.
Comment 14 WebKit Commit Bot 2010-08-22 17:14:26 PDT
Comment on attachment 64932 [details]
Proposed fix v3 (with ChangeLog w/o tabs)

Clearing flags on attachment: 64932

Committed r65789: <http://trac.webkit.org/changeset/65789>
Comment 15 WebKit Commit Bot 2010-08-22 17:14:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Ademar Reis 2010-08-25 08:06:05 PDT
Revision r65789 cherry-picked into qtwebkit-2.1 with commit be1fdcbd1af1fab0c579606fce82303b36188ca1