Merge lp:~gerboland/qtubuntu/fix_1351024 into lp:qtubuntu

[Gerry Boland]

Workaround for bug 1346633 broke QGuiApplication::topLevelWindows(), perform a different workaround.

This workaround guesses the window geometry relative to the screen (mainly setting y=panelHeight), defining it using QPlatformWindow::setGeometry and then Qt uses it to calculate mapToGlobal internally

Workaround for bug 1346633 broke QGuiApplication::topLevelWindows(), perform a different workaround

[Timo Jyrinki (timo-jyrinki)]

Tested with a local build, works with this branch! (ie. similar to #157)

[Timo Jyrinki (timo-jyrinki)]

review: Approve

[Daniel d'Andrada (dandrader)]

There's no need to store panelHeight as it's only used inside createWindow()

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

> There's no need to store panelHeight as it's only used inside createWindow()

And please put the code that calculates it into a helper method.

review: Needs Fixing

[Gerry Boland (gerboland)]

I wanted a small diff, but ok

[Daniel d'Andrada (dandrader)]

     } else if (d->state == Qt::WindowMaximized) {
         printf("UbuntuWindow - maximized geometry\n");
         geometry = screen()->availableGeometry();
+ /*
+ * FIXME: Autopilot relies on being able to convert coordinates relative of the window
+ * into absolute screen coordinates. Mir does not allow this, see bug lp:1346633
+ * Until there's a correct way to perform this transformation agreed, this horrible hack
+ * guesses the transformation heuristically.
+ *
+ * Assumption: this method only used on phone devices!
+ */
+ geometry.setY(d->panelHeight);

Shouldn't we deduct panelHeight from the geometry.height?
Ie, it's either

(0, 0, screenWidth, screenHeight) or (0, panelHeight, screenWidth, screenHeight - panelHeight),
but here it seems you're setting it to (0, panelHeight, screenWidth, screenHeight), which is wrong

review: Needs Fixing

[lp:~gerboland/qtubuntu/fix_1351024]

237. By Gerry Boland on 2014-08-08

Review comments addressed

[Gerry Boland (gerboland)]

Shouldn't we deduct panelHeight from the geometry.height?
Ie, it's either

(0, 0, screenWidth, screenHeight) or (0, panelHeight, screenWidth, screenHeight - panelHeight),
but here it seems you're setting it to (0, panelHeight, screenWidth, screenHeight), which is wrong

I disagree. The client is asking for a surface with a particular geometry, we should not shrink it. Shell is the one which decides to shrink it anyway

[Daniel d'Andrada (dandrader)]

    // Use client geometry if set explicitly, use available screen geometry otherwise.
    d->geometry = window()->geometry() != screen->geometry() ?
        window()->geometry() : screen->availableGeometry();

I would also move that to inside createWindow(), more specifically to here:

    } else {
        printf("UbuntuWindow - regular geometry\n");
        geometry = d->geometry;
        geometry.setY(d->panelHeight);
    }

review: Needs Fixing

[Gerry Boland (gerboland)]

[ubuntumirclient QPA] creating surface at (0, 58) with size (1800, 1382) with title 'qmlscene: calendar'

UbuntuWindowPrivate::createEGLSurface (this=0x2293838, nativeWindow=0xb2200998)
[ubuntumirclient QPA] created surface has size (768, 1280)

those are the log outputs that show the surface width/height the client asks for is inappropriate for phone, so shell overrides it with fullscreen (shell bug to be fixed)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:236
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/151/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-utopic-armhf-ci/17/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/151/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

> // Use client geometry if set explicitly, use available screen geometry
> otherwise.
> d->geometry = window()->geometry() != screen->geometry() ?
> window()->geometry() : screen->availableGeometry();
>
>
> I would also move that to inside createWindow(), more specifically to here:
>
> } else {
> printf("UbuntuWindow - regular geometry\n");
> geometry = d->geometry;
> geometry.setY(d->panelHeight);
> }

That kind of refactoring is unnecessary for this bug fix, we can leave it for later.

[Daniel d'Andrada (dandrader)]

review: Approve

=== modified file 'src/ubuntumirclient/input.cpp'
--- src/ubuntumirclient/input.cpp	2014-07-15 12:45:14 +0000
+++ src/ubuntumirclient/input.cpp	2014-08-08 14:04:28 +0000
@@ -278,8 +278,8 @@
     for (int i = 0; i < kPointerCount; ++i) {
         QWindowSystemInterface::TouchPoint touchPoint;
 
-        const float kX = event->motion.pointer_coordinates[i].raw_x;
-        const float kY = event->motion.pointer_coordinates[i].raw_y;
+        const float kX = event->motion.pointer_coordinates[i].raw_x + kWindowGeometry.x();
+        const float kY = event->motion.pointer_coordinates[i].raw_y + kWindowGeometry.y(); // see bug lp:1346633 workaround comments elsewhere
         const float kW = event->motion.pointer_coordinates[i].touch_major;
         const float kH = event->motion.pointer_coordinates[i].touch_minor;
         const float kP = event->motion.pointer_coordinates[i].pressure;

=== modified file 'src/ubuntumirclient/window.cpp'
--- src/ubuntumirclient/window.cpp	2014-07-21 23:47:20 +0000
+++ src/ubuntumirclient/window.cpp	2014-08-08 14:04:28 +0000
@@ -41,6 +41,7 @@
 public:
     void createEGLSurface(EGLNativeWindowType nativeWindow);
     void destroyEGLSurface();
+    int panelHeight();
 
     UbuntuScreen* screen;
     EGLSurface eglSurface;
@@ -54,7 +55,6 @@
     QSize bufferSize;
     QSize targetBufferSize;
     QMutex mutex;
-    int panelHeight; // FIXME - should be removed
 };
 
 static void eventCallback(void* context, const WindowEvent* event)
@@ -86,21 +86,6 @@
         window()->geometry() : screen->availableGeometry();
     createWindow();
     DLOG("UbuntuWindow::UbuntuWindow (this=%p, w=%p, screen=%p, input=%p)", this, w, screen, input);
-
-    // FIXME - in order to work around https://bugs.launchpad.net/mir/+bug/1346633
-    // we need to guess the panel height (3GU + 2DP)
-    const int defaultGridUnit = 8;
-    int gridUnit = defaultGridUnit;
-    QByteArray gridUnitString = qgetenv("GRID_UNIT_PX");
-    if (!gridUnitString.isEmpty()) {
-        bool ok;
-        gridUnit = gridUnitString.toInt(&ok);
-        if (!ok) {
-            gridUnit = defaultGridUnit;
-        }
-    }
-    qreal densityPixelRatio = static_cast<qreal>(gridUnit) / defaultGridUnit;
-    d->panelHeight = gridUnit * 3 + qFloor(densityPixelRatio) * 2;
 }
 
 UbuntuWindow::~UbuntuWindow()
@@ -131,6 +116,24 @@
     }
 }
 
+// FIXME - in order to work around https://bugs.launchpad.net/mir/+bug/1346633
+// we need to guess the panel height (3GU + 2DP)
+int UbuntuWindowPrivate::panelHeight()
+{
+    const int defaultGridUnit = 8;
+    int gridUnit = defaultGridUnit;
+    QByteArray gridUnitString = qgetenv("GRID_UNIT_PX");
+    if (!gridUnitString.isEmpty()) {
+        bool ok;
+        gridUnit = gridUnitString.toInt(&ok);
+        if (!ok) {
+            gridUnit = defaultGridUnit;
+        }
+    }
+    qreal densityPixelRatio = static_cast<qreal>(gridUnit) / defaultGridUnit;
+    return gridUnit * 3 + qFloor(densityPixelRatio) * 2;
+}
+
 void UbuntuWindow::createWindow()
 {
     DLOG("UbuntuWindow::createWindow (this=%p)", this);
@@ -147,8 +150,10 @@
     flags |= static_cast<uint>(IS_OPAQUE_FLAG);
 
     const QByteArray title = (!window()->title().isNull()) ? window()->title().toUtf8() : "Window 1"; // legacy title
+    const int panelHeight = d->panelHeight();
 
     #if !defined(QT_NO_DEBUG)
+    LOG("panelHeight: '%d'", panelHeight);
     LOG("role: '%d'", role);
     LOG("flags: '%s'", (flags & static_cast<uint>(1)) ? "Opaque" : "NotOpaque");
     LOG("title: '%s'", title.constData());
@@ -162,9 +167,19 @@
     } else if (d->state == Qt::WindowMaximized) {
         printf("UbuntuWindow - maximized geometry\n");
         geometry = screen()->availableGeometry();
+        /*
+         * FIXME: Autopilot relies on being able to convert coordinates relative of the window
+         * into absolute screen coordinates. Mir does not allow this, see bug lp:1346633
+         * Until there's a correct way to perform this transformation agreed, this horrible hack
+         * guesses the transformation heuristically.
+         *
+         * Assumption: this method only used on phone devices!
+         */
+        geometry.setY(panelHeight);
     } else {
         printf("UbuntuWindow - regular geometry\n");
         geometry = d->geometry;
+        geometry.setY(panelHeight);
     }
 
     DLOG("[ubuntumirclient QPA] creating surface at (%d, %d) with size (%d, %d) with title '%s'\n",
@@ -379,21 +394,3 @@
         }
     }
 }
-
-QPoint UbuntuWindow::mapToGlobal(const QPoint &position) const
-{
-    /*
-     * FIXME: Autopilot relies on being able to convert coordinates relative of the window
-     * into absolute screen coordinates. Mir does not allow this, see bug lp:1346633
-     * Until there's a correct way to perform this transformation agreed, this horrible hack
-     * guesses the transformation heuristically.
-     *
-     * Assumption: this method only used on phone devices!
-     */
-
-    if (d->state == Qt::WindowFullScreen)
-        return position;
-
-    // FIXME: update when enabling rotation in shell
-    return QPoint(position.x(), position.y() + d->panelHeight);
-}

=== modified file 'src/ubuntumirclient/window.h'
--- src/ubuntumirclient/window.h	2014-07-21 23:47:20 +0000
+++ src/ubuntumirclient/window.h	2014-08-08 14:04:28 +0000
@@ -45,11 +45,6 @@
 
     UbuntuWindowPrivate* priv() { return d; }
 
-    // Begin methods implemented as workaround for lp:1346633
-    bool isEmbedded(const QPlatformWindow *) const override { return true; }
-    QPoint mapToGlobal(const QPoint &position) const override;
-    // End methods implemented as workaround for lp:1346633
-
 public Q_SLOTS:
     void handleBufferResize(int width, int height);
     void forceRedraw();