544216 - Switch from QWidgets to QGraphicsWidgets

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Steffen Imhof]

Attachment: Patch to switch from QWidgets to QGraphicsWidgets

The attached patch switches the MozQWidget which is used for the native painting and event handling from being QWidget-derived to QGraphicsWidget-derived.

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 425189 [details] [diff] [review]
Patch to switch from QWidgets to QGraphicsWidgets

Is there a need for the typedef here?

+typedef QGraphicsWidget MozQBaseWidget;


need to check for failures after your new's:

+        mScene = new QGraphicsScene();
+        newView = new MozQGraphicsView(mScene);



style nit: White space is off here:

 MozQWidget::~MozQWidget()
 {
     if (mReceiver)
-        mReceiver->QWidgetDestroyed();
+          mReceiver->QWidgetDestroyed();


style nit:

QGraphicsSceneContextMenuEvent * aEvent

should be -

QGraphicsSceneContextMenuEvent* aEvent

there are a few of these.


Lets not add the commented out code to nsWindow::SetCursor.  It is probably best to just return NS_ERROR_NOT_AVAILABLE instead of the OOM error


how is the MozQGraphicsView ever destroyed and how do we know that the scene passed to it during construction is never deleted.  Do we need a nsRefPtr or something?


style nit:

+            mTopLevelWidget->setGeometry( 0.0, 0.0, 
+                static_cast<qreal>( aEvent->size().width() ),
+                static_cast<qreal>( aEvent->size().height() ) );

Too many spaces.  Probably something like this is better:

+            mTopLevelWidget->setGeometry(0.0, 0.0, 
+                static_cast<qreal>(aEvent->size().width()),
+                static_cast<qreal>(aEvent->size().height()));


same here (note the closing ')'):

QApplication::postEvent(mTopLevelWidget, new QCloseEvent(*aEvent) );



Can parent now not be null?  Maybe add an assertion?

-    QWidget *parentWidget = (parent)? (QWidget*)parent->GetNativeData(NS_NATIVE_WIDGET):0;
 
+    QWidget *parentWidget = static_cast<QWidget*>(parent->GetNativeData(NS_NATIVE_SHELLWIDGET));


+
+    // FIXME: using a QFileDialog here at the moment, later this might be replaced
+    // by more platform specific code


Lets not talk about vague future plans here.  lets drop the comment.


I don't understand:
+        if (mScene->views().size() == 1)
+            delete mScene->views()[0];


What if the size is greater than 1?  does QGraphicsScene deal with that?


Why do we not need to test for mWidget here, but in other places we do?

-    MozQWidget *mozWidget = static_cast<MozQWidget*>(mWidget);
-    if (mozWidget)
-        mozWidget->setModal(aModal);
+    mWidget->setModal(aModal);


What is the value in adding (and similar commented out setZValue lines):
+             LOG(("FIXME:>>>>>>Func:%s::%d\n", __PRETTY_FUNCTION__, __LINE__));
+             //mWidget->setZValue( aZIndex );


Nit (don't add extra white spaces)

     // grab it.  Note that we don't set our focus flag in this case.
-
     LOGFOCUS(("  SetFocus [%p]\n", (void *)this));
-
+  




It might be easiest to just add a recursive helper function that replaces this code (or I don't think you need to use bInvisibleItem)

+    while (parent) {
+        if (bInvisibleItem) {
+            if (parent->isVisible()) {
+                realFocusItem = parent;
+            }
+            bInvisibleItem = false;
+        }
+
+        if (!parent->isVisible()) {
+            bInvisibleItem = true;
+        }
+
+        parent = parent->parentItem();
+    }
+


Why two calls?  and a nit about the whitespace:

-    mWidget->setFocus();
+        realFocusItem->setFocus(Qt::ActiveWindowFocusReason);
+    realFocusItem->setFocus();



In  nsWindow::Invalidate, can you explain why we don't need to call repaint in the sync case?


Could you file a bug about nsWindow::GetAttention need for a real implementation.



Lastly, i just applied this to the tip of mc and tried to build FF and saw this error:


home/dougt/builds/firefox/mozilla-central/widget/src/qt/nsWindow.cpp: In member function ‘QWidget* nsWindow::GetViewWidget()’:
/home/dougt/builds/firefox/mozilla-central/widget/src/qt/nsWindow.cpp:668: error: ‘((nsWindow*)this)->nsWindow::mScene->QGraphicsScene::views’ does not have class type
make[6]: *** [nsWindow.o] 


ac_add_options --enable-default-toolkit=cairo-qt
ac_add_options --with-qtdir=/usr/lib/qt4

Comment 2

[Steffen Imhof]

Attachment: Patch to switch to QGraphicsWidgets addressing Doug's concerns

(In reply to comment #1)

Thanks for your review, Doug. You raised a few good points, I hope I could address them.

> (From update of attachment 425189 [details] [diff] [review])
> Is there a need for the typedef here?
> 
> +typedef QGraphicsWidget MozQBaseWidget;

Not really at the moment, I removed it.

> need to check for failures after your new's:
> 
> +        mScene = new QGraphicsScene();
> +        newView = new MozQGraphicsView(mScene);
Added some early exits so that nsWindow::Create() can return NS_E_OOM.

> style nits
Fixed.

> Lets not add the commented out code to nsWindow::SetCursor.  It is probably
> best to just return NS_ERROR_NOT_AVAILABLE instead of the OOM error
Removed code. 

> how is the MozQGraphicsView ever destroyed and how do we know that the scene
> passed to it during construction is never deleted.  Do we need a nsRefPtr or
> something?

We created a scene for every toplevel window, so that we can have multiple "real" windows in Firefox. That one is tied to the nsWindow as member mScene and gets deleted during destruction. Each scene (in theory) can have multiple views on it, which is not the case here, since we create only one view (ours). So that is indirectly tied to the nsWindow and gets deleted during destruction, too. The idea was not to have an extra member variable for the view as it would be redundant, but on the other hand it might be a bit clearer. So I think we could change that.

> Can parent now not be null?  Maybe add an assertion?
> 
> -    QWidget *parentWidget = (parent)?
> (QWidget*)parent->GetNativeData(NS_NATIVE_WIDGET):0;
> 
> +    QWidget *parentWidget =
> static_cast<QWidget*>(parent->GetNativeData(NS_NATIVE_SHELLWIDGET));

I re-added the original null pointer guard.

> I don't understand:
> +        if (mScene->views().size() == 1)
> +            delete mScene->views()[0];

I now use GetViewWidget() directly and delete that.

> What if the size is greater than 1?  does QGraphicsScene deal with that?

No, QGraphicsScene does not delete any of its views.

> Why do we not need to test for mWidget here, but in other places we do?
> 
> -    MozQWidget *mozWidget = static_cast<MozQWidget*>(mWidget);
> -    if (mozWidget)
> -        mozWidget->setModal(aModal);
> +    mWidget->setModal(aModal);

Another good point, added the check.

> What is the value in adding (and similar commented out setZValue lines):
> +             LOG(("FIXME:>>>>>>Func:%s::%d\n", __PRETTY_FUNCTION__,
> __LINE__));
> +             //mWidget->setZValue( aZIndex );

From what we could see that function is never called anyway. So for now I removed it completely, relying on nsBaseWidgets non-implementation.

> It might be easiest to just add a recursive helper function that replaces this
> code (or I don't think you need to use bInvisibleItem)

Yep, I added a function and even some comments trying to explain what it does.

> Why two calls?  and a nit about the whitespace:
> 
> -    mWidget->setFocus();
> +        realFocusItem->setFocus(Qt::ActiveWindowFocusReason);
> +    realFocusItem->setFocus();

I added an "else" there.

> In  nsWindow::Invalidate, can you explain why we don't need to call repaint in
> the sync case?

Good question. There is no direct equivalent for repaint() for QGraphicsItems, but I added some code to trigger the immediate repaint via the view.

> Could you file a bug about nsWindow::GetAttention need for a real
> implementation.

Yes, I can do that.

> Lastly, i just applied this to the tip of mc and tried to build FF and saw this
> error:
> 
> 
> home/dougt/builds/firefox/mozilla-central/widget/src/qt/nsWindow.cpp: In member
> function ‘QWidget* nsWindow::GetViewWidget()’:
> /home/dougt/builds/firefox/mozilla-central/widget/src/qt/nsWindow.cpp:668:
> error: ‘((nsWindow*)this)->nsWindow::mScene->QGraphicsScene::views’ does not
> have class type
> make[6]: *** [nsWindow.o] 
> 
> 
> ac_add_options --enable-default-toolkit=cairo-qt
> ac_add_options --with-qtdir=/usr/lib/qt4

I cannot reproduce that here with Qt 4.5 and 4.6... What version of Qt are you using? Maybe we need some additional #ifdefs.

Comment 3

[Oleg Romashin (:romaxa)]

+void MozQWidget::mouseReleaseEvent(QGraphicsSceneMouseEvent * aEvent)

xx * xx - spaces comment ignored in second patch

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 425221 [details] [diff] [review]
Patch to switch to QGraphicsWidgets addressing Doug's concerns


>+    NS_ASSERTION(mScene && mScene->views.size() == 1, "Not exactly one view for our scene!");


I am using Qt 4.3 (default on KDE right now, I think).  Commenting out this assertion gets us around the build error.  Is size() not avaiable in older Qt versions?

Other than that:

1)  Applying this patch fails on m-c now.  Mostly my fault due to recent pushes.  Pretty trival to merge (by hand)

2)  After applying and building (including the commenting out of that assertion I mentioned above), the Qt build on KDE behaves pretty bad.  Are you seeing the same thing.  For example, typing in the URL bar is busted (busted focus?), menus aren't drawing correctly (maybe just my theme), the contents of dialogs do not draw (for example, the "Clear Recent History" window appears, but its content is just white).  Can you look into these regressions?

Comment 5

[Steffen Imhof]

(In reply to comment #4)
> (From update of attachment 425221 [details] [diff] [review])
> 
> >+    NS_ASSERTION(mScene && mScene->views.size() == 1, "Not exactly one view for our scene!");
> 
> 
> I am using Qt 4.3 (default on KDE right now, I think).  Commenting out this
> assertion gets us around the build error.  Is size() not avaiable in older Qt
> versions?

Sorry, my bad. The code in the assertion is missing braces, I was obviously compiling a non-debug build. I'll fix that.

> 
> Other than that:
> 
> 1)  Applying this patch fails on m-c now.  Mostly my fault due to recent
> pushes.  Pretty trival to merge (by hand)
> 
> 2)  After applying and building (including the commenting out of that assertion
> I mentioned above), the Qt build on KDE behaves pretty bad.  Are you seeing the
> same thing.  For example, typing in the URL bar is busted (busted focus?),
> menus aren't drawing correctly (maybe just my theme), the contents of dialogs
> do not draw (for example, the "Clear Recent History" window appears, but its
> content is just white).  Can you look into these regressions?

Ok, I'll have a look. I've seen these empty dialogs before, they get drawn after a resize, so there probably is some order problem during construction.

Comment 6

[Steffen Imhof]

Attachment: Updated patch to switch to QGraphicsWidget

Thanks for reviewing again, here's an updated version of the patch:

 - removed unused parameters from MozQWidget constructor
 - made GetViewWidget() work recursively
 - fixed the dialogs initially blank issue (the QGraphicsWidges were not positioned correctly)
 - updated everything to apply on latest mozilla-central (32a13ebe9ba0)
 - removed more unwanted spaces
 - added an "a" to some parameter names
 - removed some more lines of out-commented code

I'm seeing some focus and menu background issues, but frankly not more than without the patch. The font rendering is worse, because the QPainter transformations are not considered in the Cairo QPainter surface, see also bug 470780.

Comment 7

[Doug Turner (:dougt)]

Attachment: screenshot of before patch.

Comment 8

[Doug Turner (:dougt)]

Attachment: screenshot of after patch.

Comment 9

[Doug Turner (:dougt)]

steffen,
do you also see this problem.  running kde-fedora release 12 (constantine).  Other things like context menus are broken too.

Comment 10

[Steffen Imhof]

The difference in the screenshots looks to me like the font problems because of the missing transformation. Did you try with the patch from bug 470780?

Comment 11

[Steffen Imhof]

Attachment: Updated patch to switch to QGraphicsWidget (no red squares)

Oops, I did not even see it in the screenshot at first, but the last uploaded version of the patch had a small red square in every MozQWidget. While that helps a bit in understanding the window structure, it diminishes the browsing experience somewhat :-), so I uploaded a fixed version and I also verified that the patch still applies to mozilla-central as of 48fde4aa4334.

Comment 12

[Doug Turner (:dougt)]

1) 

placement of the popup menus is wrong.  For example, html select popups are positioned below the html select element.  (you can see this by going to http://www.google.com/advanced_search?hl=en and change the result per page)

2)

With the patch from 470780, layout is no longer an issue.  I marked this bug as depending on that bug.  I do not think we can land this bug prior to having a story for fonts.

Comment 13

[Steffen Imhof]

Attachment: Updated patch to switch to QGraphicsWidget (popup placement fixed)

Indeed there was a bit of a coordinate system mismatch in the patch. This updated version should show the popups in the correct places.

(I'm currently seeing a strange effect that the popups get reeaally wide sometimes, but that also happens with the pure QWidgets. This can also mess up the placement a bit.)

Comment 14

[Steffen Imhof]

Created bug 545160 to track the wide comboboxes.

Comment 15

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/9cfb0b71fabd