Bug 28865 - [Qt] Make cursor set cleaner in QtWebKit Api: eliminate SetCursorEvent hack.
Summary: [Qt] Make cursor set cleaner in QtWebKit Api: eliminate SetCursorEvent hack.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 28862
  Show dependency treegraph
 
Reported: 2009-08-31 20:40 PDT by Antonio Gomes
Modified: 2009-09-08 07:53 PDT (History)
6 users (show)

See Also:


Attachments
patch 0.1 (6.97 KB, patch)
2009-09-06 14:12 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 0.2 - cursor set only on QEvent::DynamicPropertyChange (8.28 KB, patch)
2009-09-07 14:41 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 0.2.1 - cursor set only on QEvent::DynamicPropertyChange (8.16 KB, patch)
2009-09-07 15:28 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
(landed in r48154) patch 0.2.2 - cursor set only on QEvent::DynamicPropertyChange (8.11 KB, patch)
2009-09-08 06:59 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-08-31 20:40:46 PDT
[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
Comment 1 Antonio Gomes 2009-09-01 04:41:19 PDT
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.

:(
Comment 2 Kenneth Rohde Christiansen 2009-09-01 05:37:20 PDT
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?
Comment 3 Antonio Gomes 2009-09-01 05:42:02 PDT
> 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 ...
Comment 4 Kenneth Rohde Christiansen 2009-09-01 05:49:55 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2009-09-01 05:53:03 PDT
> 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.
Comment 6 Antonio Gomes 2009-09-06 14:12:28 PDT
Created attachment 39126 [details]
patch 0.1

uploading the patch on behalf of Kenneth Christiansen
Comment 7 Antonio Gomes 2009-09-06 14:14:03 PDT
it patch makes qwebgraphicsitem (see bug  28862) cursor's to work.  setting dep here ...
Comment 8 Antonio Gomes 2009-09-07 08:11:51 PDT
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
Comment 9 Antonio Gomes 2009-09-07 14:41:10 PDT
Created attachment 39163 [details]
patch 0.2 - cursor set only on QEvent::DynamicPropertyChange

first attempt to address simon's suggetion.
Comment 10 Antonio Gomes 2009-09-07 15:28:45 PDT
Created attachment 39164 [details]
patch 0.2.1 - cursor set only on QEvent::DynamicPropertyChange

same patch as 0.2 , but updated to tot.
Comment 11 Kenneth Rohde Christiansen 2009-09-07 15:34:27 PDT
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.
Comment 12 Antonio Gomes 2009-09-07 15:42:50 PDT
(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.
Comment 13 Kenneth Rohde Christiansen 2009-09-07 15:54:13 PDT
> 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.
Comment 14 Simon Hausmann 2009-09-08 02:40:17 PDT
(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.
Comment 15 Simon Hausmann 2009-09-08 02:43:37 PDT
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 :)
Comment 16 Simon Hausmann 2009-09-08 02:44:06 PDT
(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 :)
Comment 17 Antonio Gomes 2009-09-08 06:59:49 PDT
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 ?
Comment 18 Simon Hausmann 2009-09-08 07:43:28 PDT
Landed in r48154
Comment 19 Simon Hausmann 2009-09-08 07:44:52 PDT
(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+.
Comment 20 Antonio Gomes 2009-09-08 07:52:17 PDT
thanks simon.

marking as 'fixed'
Comment 21 Antonio Gomes 2009-09-08 07:53:53 PDT
Comment on attachment 39164 [details]
patch 0.2.1 - cursor set only on QEvent::DynamicPropertyChange

clearing the r+ flag