Bug 44252 – QWebView::selectionChanged() is never emitted.

• Component: WebKit Qt
• Status: RESOLVED FIXED

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 :)

[WebKit Review Bot]

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.

[Andreas Kling]

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.

Created attachment 64928 [details]
Proposed fix v2 (with ChangeLog)

[WebKit Review Bot]

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.

Created attachment 64932 [details]
Proposed fix v3 (with ChangeLog w/o tabs)

[Antonio Gomes]

Comment on attachment 64932 [details]
Proposed fix v3 (with ChangeLog w/o tabs)

Looks ok. Please do the same for qgraphicswebview.cpp

QGraphicsWebView doesn't _have_ a selectionChanged() signal.
Maybe it should have one, but this report is about an _existing_ signal not being emitted.

Created attachment 64947 [details]
Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request

[Kenneth Rohde Christiansen]

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?

[Antonio Gomes]

> 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.

> @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 :)

[Antonio Gomes]

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.

[Antonio Gomes]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r65789 cherry-picked into qtwebkit-2.1 with commit be1fdcbd1af1fab0c579606fce82303b36188ca1