Bug 28865 – [Qt] Make cursor set cleaner in QtWebKit Api: eliminate SetCursorEvent hack.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Antonio Gomes]

[Background]

Instead of the default architecture flow commonly used when embedding through by QtWebKit API (QWebView -> QWebPage -> QWebFrame), one has the following: CustomWidget -> QWebPage -> QWebFrame (e.g. DUI's QFxWebView [1] or plasma's WebView [2])

[1] http://qt.gitorious.org/+qt-kinetic-developers/qt/kinetic/blobs/kinetic-declarativeui-gv/src/declarative/fx/qfxwebview.h
[2] http://api.kde.org/4.x-api/kdelibs-apidocs/plasma/html/classPlasma_1_1WebView.html

[Bug description]

currently it is very hacky the way cursors are set to the QApplication. The flow is +- :

1) an outer embedding widget (for example qwebview or any other custom web widget) receives mouse events (like the 'mousemove' event)

2) mousemove event is sent to qwebpage (see QWebPage::event and then to QWebPagePrivate::mouseMoveEvent) and then to WebCore.

3) When there is a cursor change, WidgetQt::setCursor method is called (see quotation below).

#include "qwebframe_p.h" // <-- that includes qwebpage_p.h
(...)
void Widget::setCursor(const Cursor& cursor)
{
#ifndef QT_NO_CURSOR
    QWidget* widget = root()->hostWindow()->platformWindow();
(...)
    QCoreApplication::postEvent(widget, new SetCursorEvent(cursor.impl()));
#endif
(...)
}

4) 3_ sends an custom event back to QWebView and from within QWebView::event() method, we have:

void QWebView::event() {
(...)
#ifndef QT_NO_CURSOR
        } else if (e->type() == static_cast<QEvent::Type>(WebCore::SetCursorEvent::EventType)) {
            d->setCursor(static_cast<WebCore::SetCursorEvent*>(e)->cursor());
#if QT_VERSION >= 0x040400
(...)

whereas a static_cast to a not exposed class happens. pontentially wrong things:

* in WidgetQt::setCursor method we have

    ...
    QWidget* widget = root()->hostWindow()->platformWindow();
    if (!widget)
        return;
    ...

here, it is mandatory that platformWindow() returns a QWidget, which can be problematic for non-QWidget subclasses (e.g. QGraphicsWidget subclass objects).

    ...
    QCoreApplication::postEvent(widget, new SetCursorEvent(cursor.impl()));
    ...

SetCursorEvent is defined in a private header, so this class wont be available together with other API headers. In QWebView it is not a problem, since it includes this private header, but it can be a problem on other situations, including DUI and plasma's qgraphicswidget approaches.

this API needs some better implementation, imho

[Antonio Gomes]

in addition, in my case for example, since hostWindow()->platformWindow() *has* to be a QWidget and we had a QGraphicsWidget (not a QWidget) subclass, I had:

1) to make the QGraphicsView->viewport() (note it is a QWidget) as the parent of my QWebPage 
2) Install an eventFilter to my viweport widget, to filter the SetCursorEvent event from WidgetQt.
3) Since SetCursorEvent is in a private header (qwebpage_p.h), I had to duplicate the class in my code and make static_cast's for things to work.

:(

[Kenneth Rohde Christiansen]

first of all:

QCoreApplication::postEvent(widget, new SetCursorEvent(cursor.impl()));

The above call doesn't require a QWidget but merely a QObject (QWidget, QGraphicsWidget, ... are all QObjects)!

secondly: You do not need to duplicate the SetCursorEvent class when implementing this inside WebKit, and this definately is the right place to implement a QGraphicsItem based widget. You will see that my current patch doesn't duplicate the class.

Maybe changing a few things would make it work for you?

[Antonio Gomes]

> QCoreApplication::postEvent(widget, new SetCursorEvent(cursor.impl()));

of course if does not, but if you do not assigneda QWidget as parent of it will fail in a static_cast and QWebPage hostWindow()->plaftormWidget() will be null ... so method will bail our and no cursor will be set at all.
 
> The above call doesn't require a QWidget but merely a QObject (QWidget,
> QGraphicsWidget, ... are all QObjects)!

that is a bug by itself.

> secondly: You do not need to duplicate the SetCursorEvent class when
> implementing this inside WebKit, and this definately is the right place to
> implement a QGraphicsItem based widget. You will see that my current patch
> doesn't duplicate the class.

it does not duplicate if it is inside Webkit, of course. but i do not care only about making things to work inside webkit. image you are an embedder ... and your code would not be in webkit ...

[Kenneth Rohde Christiansen]

You do not need to set the viewport() as the parent, setting it as the view of
the QWebPage should do the trick, and I actually do not find that that strange.

Maybe we could add a setView helper to QWebGraphicsItem, as a QGraphicsItem
isnt a view in itself, but would need one for showing cursors anyway.

[Kenneth Rohde Christiansen]

> of course if does not, but if you do not assigneda QWidget as parent of it will
> fail in a static_cast and QWebPage hostWindow()->plaftormWidget() will be null
> ... so method will bail our and no cursor will be set at all.

That makes a lot of sense. A hostWindow is required for showing cursors, that is how X works.

> it does not duplicate if it is inside Webkit, of course. but i do not care only
> about making things to work inside webkit. image you are an embedder ... and
> your code would not be in webkit ...

That is true, but we discourage this and that is part of why we will contribute a new QWebGraphicsItem. Also, that is a very special use-case.

[Antonio Gomes]

Created attachment 39126 [details]
patch 0.1

uploading the patch on behalf of Kenneth Christiansen

[Antonio Gomes]

it patch makes qwebgraphicsitem (see bug  28862) cursor's to work.  setting dep here ...

[Antonio Gomes]

Comment on attachment 39126 [details]
patch 0.1

clearing review(?) flag to address simon's feedback on IRC:

<tronical> tonikitoo, kenneth: I have an idea for teh cursor patch
<tronical> tonikitoo, kenneth: I _love_ your idea of using a dynamic property instead of an event. that's indeed much more reusable for things like the graphicsitem than the custom event
<tronical> tonikitoo, kenneth: but currently you do setProperty("WebCoreCursor", cursor); and _then_ setCursor(cursor);
<tronical> tonikitoo, kenneth: instead it might be easier to distinguish between the two cases, webcore setting teh cursor and QWidget::unsetCursor() being called, by simply _not_ calling setCursor() from within WidgetQt.cpp
<tronical> tonikitoo, kenneth: and instead listening to DynamicPropertyChangeEvent in QWebView

[Antonio Gomes]

Created attachment 39163 [details]
patch 0.2 - cursor set only on QEvent::DynamicPropertyChange

first attempt to address simon's suggetion.

[Antonio Gomes]

Created attachment 39164 [details]
patch 0.2.1 - cursor set only on QEvent::DynamicPropertyChange

same patch as 0.2 , but updated to tot.

[Kenneth Rohde Christiansen]

Using DynamicPropertyChange will not work for the QWebGraphicsItem so far, as the platformWidget returned is the QGraphicsView viewport (the last one who sent a event to the page), thus setCursor will work, but the DynamicPropertyChange will not.

So, I don't like this change unless we fix the platformWidget not to return who sent events, but the actual QWebPage wrapper (QWebView or QGraphicsView) - thus a QObject and not a QWidget.

I do not know if this is really desirable.

[Antonio Gomes]

(In reply to comment #11)
> Using DynamicPropertyChange will not work for the QWebGraphicsItem so far, as
> the platformWidget returned is the QGraphicsView viewport (the last one who
> sent a event to the page), thus setCursor will work, but the
> DynamicPropertyChange will not.
> 
> So, I don't like this change unless we fix the platformWidget not to return who
> sent events, but the actual QWebPage wrapper (QWebView or QGraphicsView) - thus
> a QObject and not a QWidget
> I do not know if this is really desirable.

hum , i see your concern. can not we install an event filter to qgraphicsview->viewport() and make the thing to happen there ?

kenneth, if simon does not agree w/ this patch as well, please feel free to un-obsolete the previous one and ignore my name in patch-by fields.

[Kenneth Rohde Christiansen]

> hum , i see your concern. can not we install an event filter to
> qgraphicsview->viewport() and make the thing to happen there ?
> 
> kenneth, if simon does not agree w/ this patch as well, please feel free to
> un-obsolete the previous one and ignore my name in patch-by fields.

As far as I understood it, it was a good suggestions from Simon's side, making it a bit easier for us, knowing when WebCore set the cursor and when it was set by the user.

Adding an eventfilter is really a bad workaround and I would avoid that at all costs. As you know from your old eventfilter patch, it is currently not possible to get notified by Qt when a scene gets / looses a view, and thus adding the eventfilter doesn't always work as expected as it depends on the call order of the app using QWebGraphicsItem.

I believe that it is right for the platformWidget to return the last widget who sent events to the QWebPage, as pointed out by Simon, as it is always used to act on events.

Maybe we need a way to get the QWebPage  or its view (QWebView or QWebGraphicsItem - QWidget or QGraphicsWidget) from within Widget in a similar way to get the platformWidget.

I think we can commit the first patch as it is, and then make a bug report for the remaining improvements/issues.

[Simon Hausmann]

(In reply to comment #11)
> Using DynamicPropertyChange will not work for the QWebGraphicsItem so far, as
> the platformWidget returned is the QGraphicsView viewport (the last one who
> sent a event to the page), thus setCursor will work, but the
> DynamicPropertyChange will not.
> 
> So, I don't like this change unless we fix the platformWidget not to return who
> sent events, but the actual QWebPage wrapper (QWebView or QGraphicsView) - thus
> a QObject and not a QWidget.
> 
> I do not know if this is really desirable.

I agree it won't work if the property is always set on the QWidget that platformWidget() returns. We _want_ to set the property on the visual wrapper around QWebPage, which can be either a QWebView (what platformWidget() returns) or the QWebGraphicsItem. The latter is also a QObject and therefore supports dynamic properties. We just need to somehow make WidgetQt::setCursor operate on a QObject instead of a QWidget for it to work with the graphics item. And of course we need to map it to QGraphicsItem's setCursor/unsetCursor :)

I believe in WebCore/manual-tests there's a few tests (.html files) that allow testing the custom cursors.

[Simon Hausmann]

Comment on attachment 39164 [details]
patch 0.2.1 - cursor set only on QEvent::DynamicPropertyChange

r=me

> +            // might be a QWidget::unserCursor()

Please fix the typo when landing :)

[Simon Hausmann]

(In reply to comment #13)
> I think we can commit the first patch as it is, and then make a bug report for
> the remaining improvements/issues.

I fully agree :)

[Antonio Gomes]

Created attachment 39182 [details]
(landed in r48154) patch 0.2.2 - cursor set only on QEvent::DynamicPropertyChange 

patch ready to land (typo fixed, updated changelogs).

same as patch 0.2.1 but updated to tot.

ps: do we need to r+ this again ?

[Simon Hausmann]

Landed in r48154

[Simon Hausmann]

(In reply to comment #17)
> Created an attachment (id=39182) [details]
> patch 0.2.2 - cursor set only on QEvent::DynamicPropertyChange 
> 
> patch ready to land (typo fixed, updated changelogs).
> 
> same as patch 0.2.1 but updated to tot.
> 
> ps: do we need to r+ this again ?

Ah no, I thought you had received your commit access already :)

I've landed the latest patch.

Setting the commit-queue+ flag won't automatically commit the patch unless it has received an r+.

[Antonio Gomes]

thanks simon.

marking as 'fixed'

[Antonio Gomes]

Comment on attachment 39164 [details]
patch 0.2.1 - cursor set only on QEvent::DynamicPropertyChange

clearing the r+ flag