264027 - [patch] experimentation with mozilla/qt (--enable-default-toolkit=qt)

Status: RESOLVED INVALID

(Core Graveyard :: Ports: Qt, defect)

Description

[lkcl]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040928 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040928 Firefox/0.9.3

i'm messing around here: patch is against widgets/src/qt.

1) i removed SetModal because as-is, mozilla goes mental on the ModalFilter
function, and without SetModal it works pretty well.

2) i cut/paste over NativeResize and NativeShow from the gtk toolkit code.  it's
not perfect, but is better than what's there at the moment (where at present,
menu dropdowns are placed relative to the top left hand corner of the screen!)

3) added a function GetScreenBounds.

caveats: it appears that in the 24 hours since the qt patches were submitted
(which i downloaded from the bugzilla reports) there have been changes made in
the cvs tree since the bugzilla patches were submitted: including some removals
of qDebug but also including things around line 88 of nsWindow.cpp for example.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[lkcl]

Attachment: messing about with widgets/src/qt

experimentation with mozilla/qt

Comment 2

[Zack Rusin]

k, great, thanks. I see you're reverting essentially everything that was
committed in the last two days so it will take me a bit to review it. Also next
time if you could assign the bug to the Browser/Qt component that would be great.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

lkcl: I would suggest "cvs diff -u6p" for diffing, since that diffs against the
revision of the files that you checked out...

Comment 4

[lkcl]

Attachment: add and use NativeShow and NativeResize

only adds NativeShow and NativeResize none of the messing about of the previous
patch.

Comment 5

[Zack Rusin]

Some comments:
1) mIsTopLevel seems useless, mContainer fullfills that role,
2) +    if (mContainer)
+	mContainer->setGeometry(mBounds.x, mBounds.y, aWidth, aHeight);
+
+    mWidget->resize( aWidth, aHeight);
is wrong for containers in which case, you're essentially resizing twice. This
error is in two or three places.
3) -        offset = mWidget->mapToGlobal(offset);
+	if (mContainer)
+		offset = mContainer->mapToGlobal(offset);
+	else
+		offset = mWidget->mapToGlobal(offset);
is wrong. mContainer == mWidget for containers so the previous code was fine.
4) mCreateed also seems useless. it looks to me like mCreated == mWidget.
5) +    mWidget->setModal(aModal); is wrong (even though you ifdefed it). Qt has
that method only in a QDialog which we're not using. I might think about
implementing it for MozQWidget but for now this won't even compile.
6)
-            flags |= Qt::WType_Popup;
+            // flags |= Qt::WType_Popup;
why are you commenting this out? it's very important.

Comment 6

[lkcl]

Comment on attachment 161951 [details] [diff] [review]
add and use NativeShow and NativeResize

hi zack,

regarding your comments:

i was only vaguely becoming aware that mContainer fulfils the same role as
mIsTopLevel.

the code if (mcontainer) resize then resize mwidget yes, it might be why
without the offset = mContainer->MapToGlobal you end up with menus and the
urlbar "selector" being placed relative to the top left corner rather than the
main window.

i don't know why mWidget->SetModal() was originally there but when i commented
it out, things started working.  i also commented out flags = Qt::WType_Popup
because i thought it was part of the cvs committed stuff - evidently not.

anyway: regarding the use of mContainer->MapToGlobal() as i said if you don't
do that, you end up with menus being placed at the top left corner irrespective
of the location of the mozilla window.

it _could_ be due to the double-resize that you mentioned: if you look closely,
the original code appears to be doing a double-resize too (!) so i had just
copied that style of coding.

Comment 7

[Zack Rusin]

(In reply to comment #6) 
> the code if (mcontainer) resize then resize mwidget yes, it might be why 
> without the offset = mContainer->MapToGlobal you end up with menus and the 
> urlbar "selector" being placed relative to the top left corner rather than 
the 
> main window. 
 
They don't. I committed the right fix a few days ago. 
 
> anyway: regarding the use of mContainer->MapToGlobal() as i said if you 
don't 
> do that, you end up with menus being placed at the top left corner 
irrespective 
> of the location of the mozilla window. 
 
Like I said that was fixed properly a few days ago. And I was referring to the  
-        offset = mWidget->mapToGlobal(offset); 
+       if (mContainer) 
+               offset = mContainer->mapToGlobal(offset); 
+       else 
+               offset = mWidget->mapToGlobal(offset); 
which is simply wrong because like I said mContainer == mWidget. 
 
> it _could_ be due to the double-resize that you mentioned: if you look 
closely, 
> the original code appears to be doing a double-resize too (!) so i had just 
> copied that style of coding. 
 
No, the original code was creating a container which was different than the 
painting widget. That distinction has been gone for a while. 
BTW, this all begs a question, does your patch actually fix anything or is 
just experimenting with stuff? :) 

Comment 8

[lkcl]

zack, hi,

i just checked out the latest cvs (thu oct 14 appx 16:00 GMT) and the same
issues are present:

1) the entire set of toolbars, and the menu bar, are not present.

2) pressing ctrl-f - the keyboard shortcut for raising the file menu - results
in a dropdown menu appearing in the top left hand corner of the screen (!). 
pressing left and right arrows makes dropdown menus move across the top of the
screen - _not_ the mozilla window.

perhaps i should describe the repro circumstances, in case your setup is different:

1) i am running this over X-windows (DISPLAY=highfield:0.0, export DISPLAY).

2) i am running ./run_mozilla.sh ./firefox from ..../mozilla-cvs/mozilla/dist/bin

3) i am using fvwm [on highfield].

it takes over a minute for firefox to load up (yesterday it didn't, it took only
about 15 seconds) onto the remote X-display.

i _promise_ you that the above problem goes away if i do this [to the latest cvs
as of thu 14th 16:00]:

NS_IMETHODIMP
nsCommonWidget::ScreenToWidget(const nsRect &aOldRect, nsRect &aNewRect)
{
    if (mWidget) {
        PRInt32 X,Y;

        QPoint offset(0,0);
        if (mContainer)
                offset = mContainer->mapFromGlobal(offset);
        else
                offset = mWidget->mapFromGlobal(offset);
        X = offset.x();
        Y = offset.y();

        aNewRect.x = aOldRect.x + X;
        aNewRect.y = aOldRect.y + Y;
    }
    return NS_OK;
}


but what it doesn't fix (but my patch does, for some unknown reason) is that
when you type in the URLbar and you get the list of possible previously typed
URLs offered to you, the location of the drop-down is, again, relative to the
screen not the mozilla window.

Comment 9

[Zack Rusin]

(In reply to comment #8) 
> zack, hi, 
 
please stop it. These are not Qt port problems but the Mozilla HEAD bugs. This 
is the HEAD, why are you expecting everything to work? I'm really not a one 
person support line for all Mozilla problems... 
 
> i _promise_ you that the above problem goes away if i do this [to the latest 
cvs 
> as of thu 14th 16:00]: 
 
Don't be silly. Please, oh, please at least try to understand the code before 
sending me patches.: 
nsWindow::createQWidget 
... 
mWidget = mContainer; 
... 
 
So the diff which you posted: 
-        offset = mWidget->mapToGlobal(offset);  
+       if (mContainer)  
+               offset = mContainer->mapToGlobal(offset);  
+       else  
+               offset = mWidget->mapToGlobal(offset);  
does: 
if (mContainer) 
   offset = mWidget->mapToGlobal(offset); 
else 
   offset = mWidget->mapToGlobal(offset); 
So please don't tell me it fixes anything, this is not magic. You probably 
clicked on something, got a weird popup somewhere and decided that you fixed 
something. Don't let this discourage you, but please, try to understand what 
your code does before sending it to me and don't be just blantly copying code 
from other ports. 

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

missing toolbars is bug 264105, btw.

Comment 11

[Zack Rusin]

I fixed the issues which you were reporting so I'm closing this.

Comment 12

[Jason Bassford]

Reading through all of the comments, nothing new was introduced to Mozilla code
from this bug.  In fact, it wasn't even a case of other things fixing what was
reported - but that the report was unwarranted.

-> INVALID