Last Comment Bug 630186 - Ensure Rotation functionality in Qt Port after removing meegotouch
: Ensure Rotation functionality in Qt Port after removing meegotouch
Status: RESOLVED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 607936
Blocks: 626595
  Show dependency treegraph
 
Reported: 2011-01-31 06:40 PST by Jeremias Bosch (:jbos)
Modified: 2011-03-16 16:43 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP, adds functionality but include is somehow still wrong (4.51 KB, patch)
2011-02-03 12:47 PST, Jeremias Bosch (:jbos)
romaxa: feedback-
Details | Diff | Splinter Review
WIP, second WIP Version. (5.73 KB, patch)
2011-02-04 02:53 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
WIP, second WIP Version. Fixed Layout (5.17 KB, patch)
2011-02-04 02:56 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
WIP, second WIP Version. Fixed Layout (5.01 KB, patch)
2011-02-04 02:57 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
WIP, third WIP version. (5.30 KB, patch)
2011-02-04 04:56 PST, Jeremias Bosch (:jbos)
romaxa: feedback+
Details | Diff | Splinter Review
Still a WIP, forth Version (8.52 KB, patch)
2011-02-16 05:31 PST, Jeremias Bosch (:jbos)
romaxa: review-
Details | Diff | Splinter Review
Version Five, now with real rotation (9.86 KB, patch)
2011-02-22 05:00 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
Version Five, now with real rotation .1 (10.20 KB, patch)
2011-02-22 07:36 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
Version Six, now with rotated touch (5.30 KB, patch)
2011-02-24 07:01 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
Version Six, now with rotated touch v2 (10.37 KB, patch)
2011-02-24 08:01 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
Version Seven, now with rotated touch and resized (16.91 KB, patch)
2011-03-01 04:16 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
Version Seven, now with rotated touch and resized v2 (16.91 KB, patch)
2011-03-01 04:23 PST, Jeremias Bosch (:jbos)
romaxa: review-
Details | Diff | Splinter Review
again wip cause of changes wanted, does not really work yet. (16.81 KB, patch)
2011-03-04 10:19 PST, Jeremias Bosch (:jbos)
no flags Details | Diff | Splinter Review
Update patch, seems to work fine (20.19 KB, patch)
2011-03-06 23:05 PST, Oleg Romashin (:romaxa)
jeremias.bosch: feedback+
Details | Diff | Splinter Review
Fixed minor comments (19.95 KB, patch)
2011-03-07 07:42 PST, Oleg Romashin (:romaxa)
jeremias.bosch: review-
Details | Diff | Splinter Review
Fixed dialogs rotation, and ifdefed orientation function (15.96 KB, patch)
2011-03-08 13:58 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fixed nsScreenQt compilation on desktop (16.12 KB, patch)
2011-03-08 16:01 PST, Oleg Romashin (:romaxa)
mozilla: review+
mitchell.field: review+
Details | Diff | Splinter Review
Fixed config review comment, and license year (16.16 KB, patch)
2011-03-10 15:52 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fixed crash on QSensor destructor, and angle undefined variable on debug build (21.98 KB, patch)
2011-03-11 14:05 PST, Oleg Romashin (:romaxa)
mozilla: review+
khuey: review+
dougt: approval2.0+
Details | Diff | Splinter Review

Description Jeremias Bosch (:jbos) 2011-01-31 06:40:43 PST
Ensure Rotation functionality in Qt Port  after removing meegotouch by using QtMobility.
Comment 1 Jeremias Bosch (:jbos) 2011-01-31 12:19:10 PST
For this we should use something like described here:

http://doc.qt.nokia.com/qtmobility-1.2.0-tp1/sensors-api.html
http://doc.qt.nokia.com/qtmobility-1.2.0-tp1/qaccelerometer.html
http://doc.qt.nokia.com/qtmobility-1.2.0-tp1/qaccelerometerreading.html

With that we get live data about the sensor position (rotation in all 3 axis!)
Comment 2 Jeremias Bosch (:jbos) 2011-01-31 12:20:26 PST
Or this:

http://doc.qt.nokia.com/qtmobility-1.2.0-tp1/qorientationreading.html
Comment 3 Jeremias Bosch (:jbos) 2011-02-03 12:47:18 PST
Created attachment 509532 [details] [diff] [review]
WIP, adds functionality but include is somehow still wrong

What do you think, should qtmobility more separated from qt, its default since qt 4.7.2 


Currently this ends in: /usr/include/qt4/QtSensors/qsensor.h:45: fatal error: qmobilityglobal.h: No such file or directory
Comment 4 Jeremias Bosch (:jbos) 2011-02-03 13:47:00 PST
meant qt 4.7.0 not 4.7.2
Comment 5 Oleg Romashin (:romaxa) 2011-02-03 13:51:44 PST
Comment on attachment 509532 [details] [diff] [review]
WIP, adds functionality but include is somehow still wrong


> #include <QtGui/QStyleOptionGraphicsItem>
> #include <QPaintEngine>
> 
>+
Please no, empty lines...
>+        gfxMatrix matr;
>+        matr.Translate(gfxPoint(aPainter->transform().dx(), aPainter->transform().dy()));
>+#if (QT_VERSION >= QT_VERSION_CHECK(4, 7, 0))
>+        QOrientationReading* reading = mOrientation.reading();
IIUC this is works only with Qt mobility, so Qt 4.7 does not have it -> so we need Qt mobility define stuff...

Please move it into separate function, we would like to use it in GL backend too

>+#if (QT_VERSION >= QT_VERSION_CHECK(4, 7, 0))

IIUC Qt mobility define needed here

>+#include <QtSensors/QOrientationSensor>
>+#endif // QT version check 4.7
>+
Comment 6 Jeremias Bosch (:jbos) 2011-02-04 00:52:31 PST
> 
> Please move it into separate function, we would like to use it in GL backend
> too
> 
Where do you like to place it?
Comment 7 Jeremias Bosch (:jbos) 2011-02-04 02:53:07 PST
Created attachment 509716 [details] [diff] [review]
WIP, second WIP Version.

Updated Version, it does compile now. Still needs testing. 

Added as supposed a MOZ_ENABLE_QTMOBILITY. The strange thing is that there is no QtMobility.pc, the way to get QtMobility include is by using QtLocation.pc...
A QtSensory.pc is also missing, there is no pc file defining -I/usr/include/qt4/QtSensors/
Comment 8 Jeremias Bosch (:jbos) 2011-02-04 02:56:36 PST
Created attachment 509718 [details] [diff] [review]
WIP, second WIP Version. Fixed Layout

Seen that there are some layout flaws... fixed.
Comment 9 Jeremias Bosch (:jbos) 2011-02-04 02:57:55 PST
Created attachment 509720 [details] [diff] [review]
WIP, second WIP Version. Fixed Layout

one more.
Comment 10 Jeremias Bosch (:jbos) 2011-02-04 04:56:34 PST
Created attachment 509736 [details] [diff] [review]
WIP, third WIP version.

Third version. I filed a bug against qt, for missing pc files 
http://bugreports.qt.nokia.com/browse/QTMOBILITY-1178

Added hardcoded pathes since the library path for qtsensors is nowhere to be found in any pc file.
Comment 11 Oleg Romashin (:romaxa) 2011-02-08 11:24:14 PST
Comment on attachment 509736 [details] [diff] [review]
WIP, third WIP version.


>-      AC_DEFINE(MOZ_ENABLE_QTNETWORK)
>+       MOZ_ENABLE_QTNETWORK=1
>+       AC_DEFINE(MOZ_ENABLE_QTNETWORK)

dont change indentation int code which is not related to your bugfix 

>+    fi
>+
>+    MOZ_ENABLE_QTNETWORK==
>+    PKG_CHECK_MODULES(_QTMOBILITY, QtDeclarative,
>+                      MOZ_ENABLE_QTMOBILITY=1,
>+                      MOZ_ENABLE_QTMOBILITY=)
>+    if test "$MOZ_ENABLE_QTMOBILITY"; then
>+       MOZ_ENABLE_QTMOBILITY=1
>+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/usr/include/qt4/QtMobility"
>+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/usr/include/qt4/QtSensors"

would it be possible to get mobility flags with pkg-config?

>+       MOZ_QT_LIBS="$MOZ_QT_LIBS -lQtSensors"
>+       AC_DEFINE(MOZ_ENABLE_QTMOBILITY)
>     fi
> fi
> 
>+
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+    mWindowRotationAngle = 0;

don't see why we need own variable here... we don't use it without GetWindowRotationAngle call, so just kill it.
or add signal listener which track orientation change and remember in mWindowRotationAngle



>+        gfxMatrix matr;
>+        matr.Translate(gfxPoint(aPainter->transform().dx(), aPainter->transform().dy()));
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+        // This is needed for rotate transformation on MeeGo
>+        // This will work very slow if pixman does not handle rotation very well
>+        matr.Rotate((M_PI/180)*GetWindowRotationAngle());
>+        NS_ASSERTION(PIXMAN_VERSION > PIXMAN_VERSION_ENCODE(0, 21, 2) ||
>+            !angle, "Old pixman and rotate transform, it is going to be slow");
>+#endif //MOZ_ENABLE_QTMOBILITY

bug 607936 already fixed, add please related rotation usage for GL backend.
Comment 12 Jeremias Bosch (:jbos) 2011-02-08 22:00:41 PST
(In reply to comment #11)
> Comment on attachment 509736 [details] [diff] [review]
> WIP, third WIP version.
> 
> 
> >-      AC_DEFINE(MOZ_ENABLE_QTNETWORK)
> >+       MOZ_ENABLE_QTNETWORK=1
> >+       AC_DEFINE(MOZ_ENABLE_QTNETWORK)
> 
> dont change indentation int code which is not related to your bugfix 
> 
ack

> >+    fi
> >+
> >+    MOZ_ENABLE_QTNETWORK==
> >+    PKG_CHECK_MODULES(_QTMOBILITY, QtDeclarative,
> >+                      MOZ_ENABLE_QTMOBILITY=1,
> >+                      MOZ_ENABLE_QTMOBILITY=)
> >+    if test "$MOZ_ENABLE_QTMOBILITY"; then
> >+       MOZ_ENABLE_QTMOBILITY=1
> >+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/usr/include/qt4/QtMobility"
> >+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/usr/include/qt4/QtSensors"
> 
> would it be possible to get mobility flags with pkg-config?
> 

no, see bug http://bugreports.qt.nokia.com/browse/QTMOBILITY-1178


> >+       MOZ_QT_LIBS="$MOZ_QT_LIBS -lQtSensors"
> >+       AC_DEFINE(MOZ_ENABLE_QTMOBILITY)
> >     fi
> > fi
> > 
> >+
> >+#ifdef MOZ_ENABLE_QTMOBILITY
> >+    mWindowRotationAngle = 0;
> 
> don't see why we need own variable here... we don't use it without
> GetWindowRotationAngle call, so just kill it.
> or add signal listener which track orientation change and remember in
> mWindowRotationAngle
> 

Because you also get Top/Down Rotation, see http://doc.qt.nokia.com/qtmobility-1.2.0-tp1/qorientationreading.html

For this case we do need to return the last rotation. since rotation angle gets polled each time... 

> 
> 
> >+        gfxMatrix matr;
> >+        matr.Translate(gfxPoint(aPainter->transform().dx(), aPainter->transform().dy()));
> >+#ifdef MOZ_ENABLE_QTMOBILITY
> >+        // This is needed for rotate transformation on MeeGo
> >+        // This will work very slow if pixman does not handle rotation very well
> >+        matr.Rotate((M_PI/180)*GetWindowRotationAngle());
> >+        NS_ASSERTION(PIXMAN_VERSION > PIXMAN_VERSION_ENCODE(0, 21, 2) ||
> >+            !angle, "Old pixman and rotate transform, it is going to be slow");
> >+#endif //MOZ_ENABLE_QTMOBILITY
> 
> bug 607936 already fixed, add please related rotation usage for GL backend.
Comment 13 Oleg Romashin (:romaxa) 2011-02-09 01:05:59 PST
Comment on attachment 509736 [details] [diff] [review]
WIP, third WIP version.


>+        switch (reading->orientation()) {
>+            //The Top edge of the device is pointing up.
>+            case QOrientationReading::TopUp:
          ^^^^ - no space here

would it be better to move mWindowRotationAngle and mOrientation into GetWindowRotationAngle, and make that function static?

and yeah, idea in general is good.
Comment 14 Oleg Romashin (:romaxa) 2011-02-09 13:05:06 PST
Comment on attachment 509736 [details] [diff] [review]
WIP, third WIP version.


>+        // This is needed for rotate transformation on MeeGo
>+        // This will work very slow if pixman does not handle rotation very well
>+        matr.Rotate((M_PI/180)*GetWindowRotationAngle());
                                ^ space needed here

one more question is how expensive mOrientation.reading->orientation call, should not we listen for some signals and update internal mWindowRotationAngle value only when real rotation happens?
Comment 15 Oleg Romashin (:romaxa) 2011-02-09 13:07:49 PST
ok, no worry, QOrientation caching this value internally.
Comment 16 Jeremias Bosch (:jbos) 2011-02-09 13:31:38 PST
:) will push updated patch tomorrow. than for review.
Comment 17 Oleg Romashin (:romaxa) 2011-02-09 13:35:37 PST
and don't forget required initialization for mOrientation.class.. like backend init, start/stop...
Comment 18 Oleg Romashin (:romaxa) 2011-02-09 13:35:58 PST
and test it on real device.
Comment 19 Jeremias Bosch (:jbos) 2011-02-09 13:42:16 PST
(In reply to comment #17)
> and don't forget required initialization for mOrientation.class.. like backend
> init, start/stop...

ok, now i am puzzled?? What are you talking about?

- there are no signals
- no init
- no start/stop
Comment 20 Jeremias Bosch (:jbos) 2011-02-09 13:43:10 PST
see http://doc.qt.nokia.com/qtmobility/qorientationreading.html
Comment 21 Oleg Romashin (:romaxa) 2011-02-09 13:45:50 PST
I was checking some APIs and qtmobility examples, and found there:
.connectToBackend();
.start();

.stop();

panoramaWithSense/orientationcontroller.cpp
Comment 22 Oleg Romashin (:romaxa) 2011-02-09 13:48:34 PST
hmm cannot get it working at all on maemo6... "reading" is always null for me...

have you tested it anywhere? btw, I think we should do something else together with orientation change... resize widget... don't we?
Comment 23 Jeremias Bosch (:jbos) 2011-02-09 13:51:02 PST
Ah i see, well that seems to be a qml / qtmobility example, Connecting QML to some c++ backend.

see http://qt.gitorious.org/qt-mobility/simulator-mobility/blobs/4f8e947f256a01a6398455b81a52cfc7f3e596df/examples/sensors/panoramaWithSense/orientationcontroller.cpp#line48

So what we do is using a prepared c++ class, no qml stuff involved.

----

Do you have libqtm-4-1.1 installed on our Maemo5?  (PR1.3)
Comment 24 Jeremias Bosch (:jbos) 2011-02-09 13:52:12 PST
(note that there a two libqtm versions on maemo,... important is the 1.1
Comment 25 Oleg Romashin (:romaxa) 2011-02-09 13:58:02 PST
ah, ok. got it.
Comment 26 Jeremias Bosch (:jbos) 2011-02-11 16:07:19 PST
Alright, it took me quite a while to figure out how this works. Basically the docs are pretty much useless and don't tell you what you need.

I had not yet the time to finally test on n900, but at least meego started to work/signaling rotation changes. (still something wrong ... for portrait its crashing...). The patch needs still time due to complete undocumented functionality. But it will come.
Comment 27 Jeremias Bosch (:jbos) 2011-02-16 05:31:15 PST
Created attachment 512779 [details] [diff] [review]
Still a WIP, forth Version

This actually get the events and send a signal, rotation basically works. But unlucky it is that window goes black for all angles != 0. I was not able to get it fixed. I don't think it has something todo with Size/Screen, but is a bug in the rendering code. 

Oleg, you have more insight - so maybe you can help to fix that.
Comment 28 Oleg Romashin (:romaxa) 2011-02-16 11:07:34 PST
Comment on attachment 512779 [details] [diff] [review]
Still a WIP, forth Version


>+#ifdef MOZ_ENABLE_QTMOBILITY
>+class OrientationSensorFilter : public QObject, public QOrientationFilter
move second public to next line


>+        switch (reading->orientation()) {
>+            //The Top edge of the device is pointing up.
>+            case QOrientationReading::TopUp:
          ^^^^ - no need this indentation space

see here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp#1850

>+    int gWindowRotationAngle;
Add empty line here
>+signals:
>+    void orientationChanged(int angle);
>+
No empty line here ;)
>+};
>+#endif


>+        if (!this->scene())
>+        {
>+            return;
>+        }
>+
>+        if (!this->scene()->views().size())
>+        {
>+            return;
>+        }

Combine this into one condition

also use  "if (...) {"

>+            qDebug() <<"Portrait Mode";
use MOZ logging, no qdebug

>+    if (!gOrientation.isActive()) {
>+        qWarning("Orientationsensor didn't start!");

NS_WARNING

>+        // This will work very slow if pixman does not handle rotation very well
>+        matr.Rotate((M_PI/180)*GetWindowRotationAngle());
                                ^ - spaces


>+nsWindow::GetWindowRotationAngle()
>+{
>+    return filter.gWindowRotationAngle;

this is not compiling... IIUC it should be gOrientationFilter.g...
Comment 29 Oleg Romashin (:romaxa) 2011-02-17 16:54:28 PST
IIUC when QOrientationReading::RightUp: happen we should set gWindowRotationAngle = 0;
Comment 30 Oleg Romashin (:romaxa) 2011-02-18 13:07:55 PST
Comment on attachment 512779 [details] [diff] [review]
Still a WIP, forth Version


>+    MOZ_ENABLE_QTNETWORK==
what is this?

>+    PKG_CHECK_MODULES(_QTMOBILITY, QtDeclarative,

Why we enabling QtMobility when QtDeclarative enabled? on desktop I have QtDeclarative, but don't have mobility

>+                      MOZ_ENABLE_QTMOBILITY=1,
Comment 31 Jeremias Bosch (:jbos) 2011-02-22 05:00:17 PST
Created attachment 514164 [details] [diff] [review]
Version Five, now with real rotation

Ok, this version finaly does rotate. in Portrait there is some issue still with resizeing the window. (sidebars height seem mostly ok - but width is somehow not perfect...) also parts of fennecs ui seem not to be resized...

Anyway. One step in the right direction.
Comment 32 Jeremias Bosch (:jbos) 2011-02-22 07:36:45 PST
Created attachment 514178 [details] [diff] [review]
Version Five, now with real rotation .1

 what does still not work on rotation:
 * fennec is only party resized in portrait
 * vkb and other dialogs do not appear rotated
 * fennec sidebar height is correct
 ** fennec width in portrait is a bit to much
 ** website size is not correctly resized in portrait
 ** touch-able area seem also not resized in portrait
Comment 33 Oleg Romashin (:romaxa) 2011-02-22 16:37:38 PST
Comment on attachment 514178 [details] [diff] [review]
Version Five, now with real rotation .1

Yep, this is does visible rotation change!
You need to resize Scene/View after rotation change (that should resize nsWindow)
Comment 34 Jeremias Bosch (:jbos) 2011-02-24 07:01:27 PST
Created attachment 514786 [details] [diff] [review]
Version Six, now with rotated touch

Touch rotated now to, but still missing:

* fennec ui not fully resized
* vkb appears still on wrong edge
Comment 35 Jeremias Bosch (:jbos) 2011-02-24 08:01:37 PST
Created attachment 514799 [details] [diff] [review]
Version Six, now with rotated touch v2
Comment 36 Oleg Romashin (:romaxa) 2011-02-25 13:17:58 PST
> * fennec ui not fully resized

Ok, I noticed that you have missing nsScreenQt.cpp, that is need to be fixed in order to get content sized properly.

> * vkb appears still on wrong edge

Any ideas what need to be done to make that work? 

Tested this patch with additional 
QTransform transform.rotate(gOrientationFilter.GetWindowRotationAngle());
QRect r = transform.mapRect(rn);

and content sized properly.
Comment 37 Jeremias Bosch (:jbos) 2011-02-25 14:41:50 PST
Thanks! This was driving me crazy. I had some mail / irc contact to VKB Devs, (Dominik - which was contributing also to Firefox Mobile) is working on 'vkb selfrotation'.)

This means currently you can not rotate the meegotouch VKB without depending to MeegoTouch. They plan to introduce some special DBUS Signal for Applications to cause VKB rotation 

So, the answer is, there is simply no way to fix VKB currently. 

(I suggested to them to use qt mobility sensors - like we do here -. So they would not depend on apps, but than they seem to miss interfaces to cover cases when an application does not rotate at all. So how ever you look on it, there seem always be a hack involved...) 

Do you finalize the patch, or should I do it?
Comment 38 Oleg Romashin (:romaxa) 2011-02-25 16:28:04 PST
> So, the answer is, there is simply no way to fix VKB currently. 

But how then VKB work with meegotuch application? do they call activeWindow API directly?

> Do you finalize the patch, or should I do it?

you  do it ;)
Comment 39 Jeremias Bosch (:jbos) 2011-02-25 16:30:07 PST
Currently they use some magic within the MSceneManager, which seems to be 'kind of global' and related with MWindows...
Comment 40 Oleg Romashin (:romaxa) 2011-02-25 18:31:31 PST
Can we report bug for them and ask them to fix it?
Comment 42 Dominik Röttsches 2011-02-27 09:25:08 PST
This branch is not related to keyboard orientation signaling. It's meant for VKB rotation animation and relies on such previously existing signals from the Meegotouch app. As I mentioned in my private email to you, Jeremias, as of now orientation changes are signaled to VKB using MInputMethod and MInputMethodState. 

> "They plan to introduce some special DBUS Signal for Applications to cause VKB rotation."

I don't know where this information comes from. That wasn't in my mail at least.
Comment 43 Jeremias Bosch (:jbos) 2011-02-27 09:31:22 PST
> orientation changes are signaled to VKB using MInputMethod and
> MInputMethodState. 

Which we can not use, since we must not link against MeegoTouch.
Comment 44 Jeremias Bosch (:jbos) 2011-03-01 04:16:48 PST
Created attachment 515879 [details] [diff] [review]
Version Seven, now with rotated touch and resized

Ok finally got it cleaned up,
Comment 45 Jeremias Bosch (:jbos) 2011-03-01 04:23:01 PST
Created attachment 515881 [details] [diff] [review]
Version Seven, now with rotated touch and resized v2
Comment 46 Oleg Romashin (:romaxa) 2011-03-01 10:25:22 PST
Comment on attachment 515881 [details] [diff] [review]
Version Seven, now with rotated touch and resized v2


>+       MOZ_ENABLE_QTNETWORK=1
>+       AC_DEFINE(MOZ_ENABLE_QTNETWORK)
>+    fi
>+
>+    MOZ_ENABLE_QTMOBILITY==
>+    PKG_CHECK_MODULES(_QTMOBILITY, QtDeclarative,
this is wrong, see comments above


>+bool
>+MozQOrientationSensorFilter::filter(QOrientationReading *reading)
>+{
>+    switch (reading->orientation()) {
>+        //The Top edge of the device is pointing up.
      ^^^^ - no need for this space, compact it

>+        case QOrientationReading::Undefined:
>+        default: break;
>+    }
Add line here
>+    return true; // don't store the reading in the sensor
>+}

>+ *
>+ * ***** END LICENSE BLOCK ***** */
Add empty Line here
>+#ifndef MOZQORIENTATIONSENSORFILTER_H
>+#define MOZQORIENTATIONSENSORFILTER_H
>+

>+
>+#endif
>\ No newline at end of file

get rid of this warning, add line

>  *
>  * ***** END LICENSE BLOCK ***** */
>-
don't remove this 
> #ifndef MOZQWIDGET_H
> #define MOZQWIDGET_H
> 
>@@ -42,12 +41,26 @@
> #include <QtGui/QGraphicsView>
> #include <QtGui/QGraphicsWidget>
> 
>+
>+
don't add this

> #include "nsIWidget.h"
> #include "prenv.h"
> 
> #include "nsIObserverService.h"
> #include "mozilla/Services.h"
> 
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+#include "mozqorientationsensorfilter.h"
>+#ifdef MOZ_X11
>+#include <QX11Info>
>+#include <X11/Xlib.h>
>+#include <X11/Xatom.h>
>+# undef KeyPress
>+# undef KeyRelease
>+# undef CursorShape
do you really need this? can you just move it up/down, or even move function impl into cpp file

> 
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+    void orientationChanged(int orientation)
>+    {
..................
>+
>+        if (orientation == 90 || orientation ==  270) {
>+            screenSize = QSizeF(screenSize.height() < screenSize.width() ? screenSize.height() : screenSize.width(),
>+                                screenSize.height() < screenSize.width() ? screenSize.width() : screenSize.height());
>+
I don't like these hacks, can you create Transform matrix and apply it to Rect/Size/Points?


>+#ifdef MOZ_X11
>+        Display *display = QX11Info::display();
>+        if (!display)
>+            return;
{}

>+
>+        Atom orientationAngleAtom = XInternAtom(display, "_MEEGOTOUCH_ORIENTATION_ANGLE", False);
>+        XChangeProperty(display, scene()->views()[0]->effectiveWinId(), orientationAngleAtom, XA_CARDINAL, 32,
>+                    PropModeReplace, (unsigned char*)&orientation, 1);
                  fix indent here


> 
>     *outTop = r.x();
>     *outLeft = r.y();
>-    *outWidth = r.width();
>-    *outHeight = r.height();
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+    if (MozQOrientationSensorFilter::GetWindowRotationAngle() == 90 ||
>+        MozQOrientationSensorFilter::GetWindowRotationAngle() == 270) {
>+        *outWidth = r.height();
>+        *outHeight = r.width();
>+    } else
>+#endif
>+    {
>+        *outWidth = r.width();
>+        *outHeight = r.height();
>+    }

Same here, use transform matrix and apply it... probably instead of GetWindowRotationAngle make sense to create GetTransform()


>@@ -80,11 +94,21 @@
> nsScreenQt::GetAvailRect(PRInt32 *outLeft,PRInt32 *outTop,
>                          PRInt32 *outWidth,PRInt32 *outHeight)
> {
>-    QRect r = QApplication::desktop()->availableGeometry(mScreen);
>+    QRect r = QApplication::desktop()->screenGeometry(mScreen);
>+
>     *outTop = r.x();
>     *outLeft = r.y();
>-    *outWidth = r.width();
>-    *outHeight = r.height();
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+     if (MozQOrientationSensorFilter::GetWindowRotationAngle() == 90 ||
>+         MozQOrientationSensorFilter::GetWindowRotationAngle() == 270) {
>+         *outWidth = r.height();
>+         *outHeight = r.width();
>+     } else
>+#endif
>+     {
>+        *outWidth = r.width();
>+        *outHeight = r.height();
>+     }
same here, use transform matrix


layers::LayerManagerOGL*>(GetLayerManager(nsnull))->
>             SetClippingRegion(event.region);
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+        gfxMatrix matr;
>+        matr.Translate(gfxPoint(aPainter->transform().dx(), aPainter->transform().dy()));
Translate should be out of this define... see SW-Rendering code below


>+#ifdef MOZ_ENABLE_QTMOBILITY
>+         // This is needed for rotate transformation on MeeGo
>+         // This will work very slow if pixman does not handle rotation very well
>+         matr.Rotate((M_PI/180)*GetWindowRotationAngle());
>+         NS_ASSERTION(PIXMAN_VERSION > PIXMAN_VERSION_ENCODE(0, 21, 2) ||
>+             !angle, "Old pixman and rotate transform, it is going to be slow");
                ^ - this will break debug build

>@@ -1202,6 +1239,7 @@
>     return status;
> }
> 
>+
no empty lines

> nsEventStatus
> nsWindow::OnCloseEvent(QCloseEvent *aEvent)
> {
>@@ -2592,6 +2630,9 @@
>             newView->viewport()->setAttribute(Qt::WA_PaintOnScreen, true);
>             newView->viewport()->setAttribute(Qt::WA_NoSystemBackground, true);
>         }
>+#ifdef MOZ_ENABLE_QTMOBILITY
>+        QObject::connect((QObject*) &gOrientationFilter, SIGNAL(orientationChanged(int)), widget, SLOT(orientationChanged(int)));
move arguments to next line, try keep code ~80 symbols
Comment 47 Jeremias Bosch (:jbos) 2011-03-04 10:19:06 PST
Created attachment 516923 [details] [diff] [review]
again wip cause of changes wanted, does not really work yet.

See commented code, i was not able to make it work / rotate correctly with transformation. 

Romaxa, take a look, what do i miss...
Comment 48 Oleg Romashin (:romaxa) 2011-03-04 10:47:29 PST
Comment on attachment 516923 [details] [diff] [review]
again wip cause of changes wanted, does not really work yet.


>+
>+dnl we need to check for QtMultimedia since package config of qtmobility 1.1 is broken
>+dnl this is why we also need to set unclude pathes and lib by hand
>+    MOZ_ENABLE_QTMOBILITY==
>+    PKG_CHECK_MODULES(_QTMOBILITY, QtMultimedia,
>+                      MOZ_ENABLE_QTMOBILITY=1,
>+                      MOZ_ENABLE_QTMOBILITY=)

Still did not get why QtMultimedia defining mobility sensors...
Practically we need only libqtm-sensors-dev, and that package contain 
/usr/lib/pkgconfig/QtSensors.pc
/usr/lib/libQtSensors.so

why we are not using this? Probably it better to change define from QTMOBILITY -> QTSENSORS or QTMOBILITY_SENSORS

>+    if test "$MOZ_ENABLE_QTMOBILITY"; then
>+       MOZ_ENABLE_QTMOBILITY=1
>+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/usr/include/qt4/QtMobility"

And we don't really need QtMobility, we need only sensors here...

>+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/us
>+
>+//         if (orientation == 90 ||
>+//             orientation ==  270) {
>+//             screenSize = QSizeF(screenSize.height() < screenSize.width() ? screenSize.height() : screenSize.width(),
>+//                                 screenSize.height() < screenSize.width() ? screenSize.width() : screenSize.height());
>+// 
>+//             QPointF position((screenSize.width() - screenSize.height()) / 2, (screenSize.width() - screenSize.height()) / 2);
>+// 
>+//             setTransformOriginPoint(screenSize.height() / 2, screenSize.width() / 2);
>+//             setPos(position);
>+//         } else {
>+//             setPos(0,0);
>+//             screenSize = QSizeF(screenSize.height() < screenSize.width() ? screenSize.width() : screenSize.height(),
>+//                                 screenSize.height() < screenSize.width() ? screenSize.height() : screenSize.width());
>+//             setTransformOriginPoint(screenSize.width() / 2, screenSize.height() / 2);
>+//         }
>+//      resize(screenSize);
>+//      scene()->setSceneRect(QRectF(pos(), screenSize);

Can we just kill this stuff now? don't keep commented code if it is obsolete


The rest of patch looks good now
Comment 49 Jeremias Bosch (:jbos) 2011-03-04 11:10:51 PST
(In reply to comment #48)
> Comment on attachment 516923 [details] [diff] [review]
> again wip cause of changes wanted, does not really work yet.
> 
> 
> >+
> >+dnl we need to check for QtMultimedia since package config of qtmobility 1.1 is broken
> >+dnl this is why we also need to set unclude pathes and lib by hand
> >+    MOZ_ENABLE_QTMOBILITY==
> >+    PKG_CHECK_MODULES(_QTMOBILITY, QtMultimedia,
> >+                      MOZ_ENABLE_QTMOBILITY=1,
> >+                      MOZ_ENABLE_QTMOBILITY=)
> 
> Still did not get why QtMultimedia defining mobility sensors...

See: http://bugreports.qt.nokia.com/browse/QTMOBILITY-1178

There is no sensors in 1.1

> Practically we need only libqtm-sensors-dev, and that package contain 
> /usr/lib/pkgconfig/QtSensors.pc
> /usr/lib/libQtSensors.so
>
Its not there in 1.1
 
> why we are not using this? Probably it better to change define from QTMOBILITY
> -> QTSENSORS or QTMOBILITY_SENSORS
> 
> >+    if test "$MOZ_ENABLE_QTMOBILITY"; then
> >+       MOZ_ENABLE_QTMOBILITY=1
> >+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/usr/include/qt4/QtMobility"
> 
> And we don't really need QtMobility, we need only sensors here...
>

We need, sensors headers include file which need QtMobility in include path.
 

> >+       MOZ_QT_CFLAGS="$MOZ_QT_CFLAGS -I/us
> >+
> >+//         if (orientation == 90 ||
> >+//             orientation ==  270) {
> >+//             screenSize = QSizeF(screenSize.height() < screenSize.width() ? screenSize.height() : screenSize.width(),
> >+//                                 screenSize.height() < screenSize.width() ? screenSize.width() : screenSize.height());
> >+// 
> >+//             QPointF position((screenSize.width() - screenSize.height()) / 2, (screenSize.width() - screenSize.height()) / 2);
> >+// 
> >+//             setTransformOriginPoint(screenSize.height() / 2, screenSize.width() / 2);
> >+//             setPos(position);
> >+//         } else {
> >+//             setPos(0,0);
> >+//             screenSize = QSizeF(screenSize.height() < screenSize.width() ? screenSize.width() : screenSize.height(),
> >+//                                 screenSize.height() < screenSize.width() ? screenSize.height() : screenSize.width());
> >+//             setTransformOriginPoint(screenSize.width() / 2, screenSize.height() / 2);
> >+//         }
> >+//      resize(screenSize);
> >+//      scene()->setSceneRect(QRectF(pos(), screenSize);
> 
> Can we just kill this stuff now? don't keep commented code if it is obsolete
>

Its not yet working (transform) as i wrote, i did not find a working tranform approach. Please take a look for fix.
 
> 
> The rest of patch looks good now
Comment 50 Oleg Romashin (:romaxa) 2011-03-05 15:35:39 PST
> /usr/lib/pkgconfig/QtSensors.pc
> /usr/lib/libQtSensors.so
>
Its not there in 1.1

why do we care about broken packages? can we ask related SDK to upgrade that package, and set dependency with version requirement?
Comment 51 Oleg Romashin (:romaxa) 2011-03-05 15:42:29 PST
please don't mix package .pc files to the libraries which are not existing in those packages.... it will break desktop compilation.
it is better to use in this case AC_CHECK_LIB AC_CHECK_HEADER macros in such cases
Comment 52 Oleg Romashin (:romaxa) 2011-03-06 23:05:41 PST
Created attachment 517366 [details] [diff] [review]
Update patch, seems to work fine

I've updated configure stuff and cleaned up a bit transform stuff... still a bit hacky but now it looks more readable.
Comment 53 Oleg Romashin (:romaxa) 2011-03-06 23:08:44 PST
I found one problem: if device in portrait mode on moment of Fennec start then fennec started in Landscape mode.... can we fix it somehow?
Comment 54 Jeremias Bosch (:jbos) 2011-03-07 03:31:57 PST
(In reply to comment #53)
> I found one problem: if device in portrait mode on moment of Fennec start then
> fennec started in Landscape mode.... can we fix it somehow?

http://bugreports.qt.nokia.com/browse/QTMOBILITY-1385
Comment 55 Jeremias Bosch (:jbos) 2011-03-07 04:13:07 PST
Comment on attachment 517366 [details] [diff] [review]
Update patch, seems to work fine

> +
> +#include "mozqorientationsensorfilter.h"
> +
> +#include <stdio.h>
> +
> +int MozQOrientationSensorFilter::mWindowRotationAngle = 0;
> +QTransform MozQOrientationSensorFilter::mWindowRotationTransform;
> +
> +bool
> +MozQOrientationSensorFilter::filter(QOrientationReading* reading)
> +{

Do we need stdio.h here?


> +    r = MozQOrientationSensorFilter::GetRotationTransform().mapRect(r);

should be in #ifdef MOZ_ENABLE_QTMOBILITY



> @@ -3023,9 +3066,8 @@ nsWindow::UserActivity()
>    if (!mIdleService) {
>      mIdleService = do_GetService("@mozilla.org/widget/idleservice;1");
>    }
>  
>    if (mIdleService) {
>      mIdleService->ResetIdleTimeOut();
>    }
>  }
> -

Not really needed.
Comment 56 Jeremias Bosch (:jbos) 2011-03-07 04:16:28 PST
I wonder about one other thing - its seems a related bug - but well have not focused on this at all. The website in portrait mode seem to have render bugs (i.e parts disappear, huge part is black and become even bigger when panning / scrolling. 

Any idea where this come from?
Comment 57 Oleg Romashin (:romaxa) 2011-03-07 07:42:21 PST
Created attachment 517422 [details] [diff] [review]
Fixed minor comments

Have not found any problems in portrait mode, only some black squares in preference page, but that more looks like common problem, possibly cairo related (I see the same problem with HW acceleration enabled), "checkbox" half of item sometime blinking while scrolling... We need separate bug about it.
Comment 58 Jeremias Bosch (:jbos) 2011-03-08 06:13:29 PST
Comment on attachment 517422 [details] [diff] [review]
Fixed minor comments



+void MozQWidget::orientationChanged()
+{
+    if (!scene() || !scene()->views().size()) {
+        return;
+    }
+
+    NS_ASSERTION(scene()->views().size() == 1, "Not exactly one view for our scene!");
+    QTransform& transform = MozQOrientationSensorFilter::GetRotationTransform();
+    QRect scrTrRect = transform.mapRect(scene()->views()[0]->rect());
+
+    setTransformOriginPoint(scene()->views()[0]->size().width() / 2, scene()->views()[0]->size().height() / 2);
+    scene()->views()[0]->setTransform(transform);
+    int orientation = MozQOrientationSensorFilter::GetWindowRotationAngle();
+    if (orientation == 0 || orientation ==  180) {
+        setPos(0,0);
+    } else {
+        setPos(-(scrTrRect.size().width() - scrTrRect.size().height()) / 2,
+               (scrTrRect.size().width() - scrTrRect.size().height()) / 2);
+    }
+    resize(scrTrRect.size());
+    scene()->setSceneRect(QRectF(QPointF(0, 0), scrTrRect.size()));
+}
+

This should be in qt mobility ifdefs.



I found one issue. With this we do not rotate i.e. the Connection Dialogs. In order to do this you need to set the x window property.
above code should include: 


#ifdef MOZ_X11
        Display *display = QX11Info::display();
        if (!display) {
            return;
        }

        Atom orientationAngleAtom = XInternAtom(display, "_MEEGOTOUCH_ORIENTATION_ANGLE", False);
        XChangeProperty(display, scene()->views()[0]->effectiveWinId(), orientationAngleAtom, XA_CARDINAL, 32,
                        PropModeReplace, (unsigned char*)& orientation, 1);
#endif


and ontop in mozqwidget.cpp you need to add 
#ifdef MOZ_ENABLE_QTMOBILITY
#ifdef MOZ_X11
#include <QX11Info>
#include <X11/Xlib.h>
#include <X11/Xatom.h>
# undef KeyPress
# undef KeyRelease
# undef CursorShape
#endif //MOZ_X11
#endif //MOZ_ENABLE_QTMOBILITY
Comment 59 Oleg Romashin (:romaxa) 2011-03-08 13:58:40 PST
Created attachment 517863 [details] [diff] [review]
Fixed dialogs rotation, and ifdefed orientation function
Comment 60 Oleg Romashin (:romaxa) 2011-03-08 16:01:27 PST
Created attachment 517908 [details] [diff] [review]
Fixed nsScreenQt compilation on desktop
Comment 61 Wolfgang Rosenauer [:wolfiR] 2011-03-09 06:11:14 PST
Comment on attachment 517908 [details] [diff] [review]
Fixed nsScreenQt compilation on desktop

>diff --git a/widget/src/qt/mozqorientationsensorfilter.cpp b/widget/src/qt/mozqorientationsensorfilter.cpp
>new file mode 100644
>--- /dev/null
>+++ b/widget/src/qt/mozqorientationsensorfilter.cpp
>@@ -0,0 +1,79 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim: set ts=4 et sw=4 tw=80: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Nokia.
>+ *
>+ * The Initial Developer of the Original Code is Nokia Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010

2011, right? Same for the other new file.

Apart from that I trust you guys that it actually works on hardware ;-)
Comment 62 Doug Turner (:dougt) 2011-03-10 13:57:22 PST
Comment on attachment 517908 [details] [diff] [review]
Fixed nsScreenQt compilation on desktop

get ted or another peer to review the build/configure, then request approval on the patch.
Comment 63 Oleg Romashin (:romaxa) 2011-03-10 13:59:20 PST
Comment on attachment 517908 [details] [diff] [review]
Fixed nsScreenQt compilation on desktop

Ted could you check build/configure part plz?
Comment 64 Mitchell Field [:Mitch] 2011-03-10 15:41:10 PST
Comment on attachment 517908 [details] [diff] [review]
Fixed nsScreenQt compilation on desktop

>--- a/configure.in
>+++ b/configure.in
>@@ -5505,6 +5505,26 @@ incorrect])
>       MOZ_ENABLE_QTNETWORK=1
>       AC_DEFINE(MOZ_ENABLE_QTNETWORK)
>     fi
>+
>+    MOZ_ENABLE_QTMOBILITY==
>+    PKG_CHECK_MODULES(_QTMOBILITY, QtSensors,

There's an extra equals sign there. r=Mitch with that fixed.
Comment 65 Oleg Romashin (:romaxa) 2011-03-10 15:52:49 PST
Created attachment 518552 [details] [diff] [review]
Fixed config review comment, and license year
Comment 66 Oleg Romashin (:romaxa) 2011-03-11 14:05:47 PST
Created attachment 518818 [details] [diff] [review]
Fixed crash on QSensor destructor, and angle undefined variable on debug build

Found problem with previous patch and qtmobility sensor implementation...
it does not check private variable and crashes on fennec close (QSensor destructor called after QApplication destroy)
Here is the a bit modified version with QSensor allocation on heap. and earlier destruction.
The rest of patch is the same
Comment 67 Wolfgang Rosenauer [:wolfiR] 2011-03-13 23:48:18 PDT
Comment on attachment 518818 [details] [diff] [review]
Fixed crash on QSensor destructor, and angle undefined variable on debug build

checked interdiff and it looks fine
Comment 68 Oleg Romashin (:romaxa) 2011-03-14 12:11:46 PDT
Comment on attachment 518818 [details] [diff] [review]
Fixed crash on QSensor destructor, and angle undefined variable on debug build

I think it safe enough for default builds.
Comment 69 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-03-15 07:54:31 PDT
Comment on attachment 518818 [details] [diff] [review]
Fixed crash on QSensor destructor, and angle undefined variable on debug build

I think Mitch already r+d the relevant bits, but regardless, r=me.
Comment 70 Oleg Romashin (:romaxa) 2011-03-16 16:43:44 PDT
http://hg.mozilla.org/mozilla-central/rev/02e5836b10c2

Note You need to log in before you can comment on or make changes to this bug.