Merge lp:~dandrader/qtmir/keyState into lp:qtmir

[Daniel d'Andrada]

Ensure the window that got a key down also gets the corresponding key up

Otherwise it will be left in a inconsistent state (with a pressed key hanging around).

QQuickWindow's input dispatching doesn't guarantee that for its QQuickItem.
So we have to do it ourselves.

This can happen when qml active focus changes in response to a key press.
Eg: client creates a child window in response to a Ctrl+O. By the time the user
releases the Ctrl, active focus will already be in the child window, so the child window
will get the release event instead of the top-level one.

* Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~dandrader/unity8/childWindowFocus/+merge/320044

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:619
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/592/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4555/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4583
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4410/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4410/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4410/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4410/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4410/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4410/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/592/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

If the surface looses active focus, don't we inform Mir? Why not send the key release event then? Would be more efficient than checking on every key_up

Also, please avoid using singleton pattern, it makes testing impossible. And speaking of which, can you add test?

review: Needs Information

[Daniel d'Andrada (dandrader)]

On 20/03/2017 14:08, Gerry Boland wrote:
> Review: Needs Information
>
> If the surface looses active focus, don't we inform Mir?

It's the other way around. Mir surface focus is what drives qml active
focus. Nowadays qml active focus by itself doesn't carry much meaning in
terms of window management.

> Why not send the key release event then? Would be more efficient than checking on every key_up

Because the key is actually still pressed. If the same window gets focus
again moments later and only then the key is lifted the client would
receive a second key_up event. Unless we keep track of what lies we have
told already to the clients or devise some other special rules.

> Also, please avoid using singleton pattern, it makes testing impossible.

Yeah, that was the easiest (laziest) way to get the two talking (event
feeder and policy). Will see if I can get something nicer. Suggestions?

But before I spend too much time on the implementation details of this
MP I would like to get feedback on the overall approach first.

> And speaking of which, can you add test?

For WindowManagementPolicy?

[lp:~dandrader/qtmir/keyState]

618. By Gerry Boland on 2017-03-20

Fix FTBFS against miral 1.3.1

Approved by: Daniel d'Andrada, Unity8 CI Bot, Alan Griffiths

619. By Daniel d'Andrada on 2017-03-20

Implement MirSurface::allowClientResize (LP: #1670390)

Approved by: Gerry Boland, Unity8 CI Bot

620. By Alan Griffiths on 2017-03-20

Reduce dependencies on mirserver by reworking ../Application/mirbuffersgtexture.cpp and ../Application/surfacemanager.cpp

Approved by: Gerry Boland, Unity8 CI Bot

621. By Gerry Boland on 2017-03-20

Stop MirSurface deleting itself, ensure SurfaceManager alone manages MirSurface lifetimes. Add SurfaceManager test suite.

This fixes bugs where a MirSurface would call deleteLater on itself, but SurfaceManager would have no idea and keep a pointer to that MirSurface in its internal list.

Instead SurfaceManager listens for signals from both miral and the MirSurface itself to decide when to delete it.

Add a test suite to verify MirSurface lifetimes. (LP: #1655644)

Approved by: Daniel d'Andrada, Unity8 CI Bot

622. By Albert Astals Cid on 2017-03-20

Check for find() result not being null before using it

We do it in onWindowReady, onWindowMoved, onWindowFocusChanged, etc so no reason to not do it in onWindowRemoved

Approved by: Lukáš Tinkl, Unity8 CI Bot

623. By Gerry Boland on 2017-03-20

Extend timeouts when running under valgrind

When QMirServer is starting up, it spawns a separate thread for Mir to startup and waits for it. As valgrind slows execution greatly, the QMirServer timeout triggers before Mir has started, causing the QMirServer to think it failed and exit.

This patch adds ability to detect when running under valgrind and extending timeouts to suit.

Approved by: Daniel d'Andrada, Unity8 CI Bot

624. By Andreas Pokorny on 2017-03-20

Attach MirInputDeviceId and the MirCookie to input events

Similar to the tracking of relative motion the cookie and input device ids of mir events are stored in the EventBuilder for later recovery (LP: #1536279, #1668692)

Approved by: Daniel d'Andrada, Unity8 CI Bot

625. By CI Train Bot Account on 2017-03-20

Releasing 0.5.1+17.04.20170320.1-0ubuntu1

[Gerry Boland (gerboland)]

> On 20/03/2017 14:08, Gerry Boland wrote:
> > Review: Needs Information
> >
> > If the surface looses active focus, don't we inform Mir?
>
> It's the other way around. Mir surface focus is what drives qml active
> focus. Nowadays qml active focus by itself doesn't carry much meaning in
> terms of window management.

There are more things in the QML scene that just MirSurfaces that can get activeFocus though. When activeFocus does switch to the search box in the AppDrawer (as example), we are indeed telling Mir somehow that the MirSurface has not got input focus (I just tested gedit, the cursor does stop flashing when the text box in the app drawer is focused). Since this bug occurs due to focus changing while modifier key is down, my initial thinking was we could use the focus change to implement a workaround.

What I don't know is if the WMpolicy if notified when the MirSurface it has selected to be focused has lost actual input focus (activeFocus). Do you know?

> > Why not send the key release event then? Would be more efficient than
> checking on every key_up
>
> Because the key is actually still pressed. If the same window gets focus
> again moments later and only then the key is lifted the client would
> receive a second key_up event. Unless we keep track of what lies we have
> told already to the clients or devise some other special rules.

This appears to be a QML subtlety we're working around, as a non-QML Qt shell would not suffer from this issue, right? So could the MirSurfaceItem keep track of the key state itself? If the MSI looses activeFocus, it send key-release to the client. Then if it gets focus again, check if key is still down (not sure how tbh, maybe QML sends the key to the Item again immediately??), and if so, re-send it to client.

I'm not 100% certain what is the best approach yet.

> > Also, please avoid using singleton pattern, it makes testing impossible.
>
> Yeah, that was the easiest (laziest) way to get the two talking (event
> feeder and policy). Will see if I can get something nicer. Suggestions?

Nothing better than the usual piping an object around, so everyone who needs it gets a pointer to it. Sorry :(

> But before I spend too much time on the implementation details of this
> MP I would like to get feedback on the overall approach first.

Ah ok, I hope I've supplied that at least

>
> > And speaking of which, can you add test?
>
> For WindowManagementPolicy?

More the logic you're adding. But please ignore me until you settle on the approach you want.

[Daniel d'Andrada (dandrader)]

On 21/03/2017 09:37, Gerry Boland wrote:
> This appears to be a QML subtlety we're working around, as a non-QML Qt shell would not suffer from this issue, right?

Yes. As I said, QQuickWindow input dispatching not perfectly suited for
shells.

> So could the MirSurfaceItem keep track of the key state itself?
Sure.

> If the MSI looses activeFocus, it send key-release to the client. Then if it gets focus again, check if key is still down (not sure how tbh, maybe QML sends the key to the Item again immediately??), and if so, re-send it to client.

It can't check the key state, but it can ignore a key-up for a key that
it didn't tell its surface it was down (because lied or whatever).

> I'm not 100% certain what is the best approach yet.

Working around QQuickWindow input dispatching will never be beautiful. So, should I go for the MirSurfaceItem-based key filtering/manipulation?

I'm fine either way. I put in the policy to avoid sending fake input to clients and as a general rule-of-thumb of "putting things in the miral policy"/"keep the policy in the loop".

[Gerry Boland (gerboland)]

I think I'd prefer using MirSurfaceItem-based key filtering. Since it's working around the QQuickWindow/QML problem, it makes sense to me to confine the workaround in code only used in code exposed to QQuickWindow/QML.

[lp:~dandrader/qtmir/keyState]

626. By Daniel d'Andrada on 2017-03-24

merge lp:~gerboland/qtmir/availableDesktopAreaTrial

627. By Daniel d'Andrada on 2017-03-24

Ensure the window that got a key down also gets the corresponding key up

Otherwise it will be left in a inconsistent state (with a pressed key hanging around).

QQuickWindow's input dispatching doesn't guarantee that for its QQuickItem.
So we have to do it ourselves.

This can happen when qml active focus changes in response to a key press.
Eg: client creates a child window in response to a Ctrl+O. By the time the user
releases the Ctrl, active focus will already be in the child window, so the child window
will get the release event instead of the top-level one.

628. By Daniel d'Andrada on 2017-03-24

Some refactoring as per review feedback

629. By Daniel d'Andrada on 2017-03-24

update copyright header

Unmerged revisions

=== modified file 'src/platforms/mirserver/qteventfeeder.cpp'
--- src/platforms/mirserver/qteventfeeder.cpp	2017-01-02 14:41:06 +0000
+++ src/platforms/mirserver/qteventfeeder.cpp	2017-03-20 15:13:20 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2016 Canonical, Ltd.
+ * Copyright (C) 2013-2017 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -22,6 +22,7 @@
 #include "tracepoints.h" // generated from tracepoints.tp
 #include "screen.h" // NEEDED?
 #include "screensmodel.h"
+#include "windowmanagementpolicy.h"
 
 #include <qpa/qplatforminputcontext.h>
 #include <qpa/qplatformintegration.h>
@@ -658,6 +659,7 @@
         keyType = QEvent::KeyPress;
         break;
     case mir_keyboard_action_up:
+        WindowManagementPolicy::instance()->dispatchKeyUp(kev);
         keyType = QEvent::KeyRelease;
         break;
     default:

=== modified file 'src/platforms/mirserver/windowmanagementpolicy.cpp'
--- src/platforms/mirserver/windowmanagementpolicy.cpp	2017-03-20 15:13:20 +0000
+++ src/platforms/mirserver/windowmanagementpolicy.cpp	2017-03-20 15:13:20 +0000
@@ -35,9 +35,17 @@
     }
 }
 
+WindowManagementPolicy *WindowManagementPolicy::m_instance = nullptr;
+
 using namespace qtmir;
 using namespace mir::geometry;
 
+
+WindowManagementPolicy *WindowManagementPolicy::instance()
+{
+    return m_instance;
+}
+
 WindowManagementPolicy::WindowManagementPolicy(const miral::WindowManagerTools &tools,
                                                qtmir::WindowModelNotifier &windowModel,
                                                qtmir::WindowController &windowController,
@@ -49,12 +57,18 @@
     , m_appNotifier(appNotifier)
     , m_eventFeeder(new QtEventFeeder(screensModel))
 {
+    m_instance = this;
     qRegisterMetaType<qtmir::NewWindow>();
     qRegisterMetaType<std::vector<miral::Window>>();
     qRegisterMetaType<miral::ApplicationInfo>();
     windowController.setPolicy(this);
 }
 
+WindowManagementPolicy::~WindowManagementPolicy()
+{
+    m_instance = nullptr;
+}
+
 /* Following are hooks to allow custom policy be imposed */
 miral::WindowSpecification WindowManagementPolicy::place_new_window(
     const miral::ApplicationInfo &appInfo,
@@ -233,9 +247,11 @@
 {
     if (mir_keyboard_event_action(event) == mir_keyboard_action_down) {
         ensureWindowIsActive(window);
+        m_keyOwners[mir_keyboard_event_key_code(event)] = window;
+        dispatchInputEvent(window, mir_keyboard_event_input_event(event));
+    } else {
+        // Ignore. We did it already in dispatchKeyUp().
     }
-
-    dispatchInputEvent(window, mir_keyboard_event_input_event(event));
 }
 
 void WindowManagementPolicy::deliver_touch_event(const MirTouchEvent *event,
@@ -354,3 +370,14 @@
         });
     }
 }
+
+void WindowManagementPolicy::dispatchKeyUp(const MirKeyboardEvent *kev)
+{
+    xkb_keysym_t keyCode = mir_keyboard_event_key_code(kev);
+
+    auto iterator = m_keyOwners.find(keyCode);
+    if (iterator != m_keyOwners.end()) {
+        miral::Window window = iterator.value();
+        dispatchInputEvent(window, mir_keyboard_event_input_event(kev));
+    }
+}

=== modified file 'src/platforms/mirserver/windowmanagementpolicy.h'
--- src/platforms/mirserver/windowmanagementpolicy.h	2017-02-06 09:10:20 +0000
+++ src/platforms/mirserver/windowmanagementpolicy.h	2017-03-20 15:13:20 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Canonical, Ltd.
+ * Copyright (C) 2016-2017 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -24,6 +24,7 @@
 #include "windowcontroller.h"
 #include "windowmodelnotifier.h"
 
+#include <QHash>
 #include <QScopedPointer>
 #include <QSize>
 
@@ -39,6 +40,7 @@
                            qtmir::WindowController &windowController,
                            qtmir::AppNotifier &appNotifier,
                            const QSharedPointer<ScreensModel> screensModel);
+    virtual ~WindowManagementPolicy();
 
     // From WindowManagementPolicy
     auto place_new_window(const miral::ApplicationInfo &app_info,
@@ -84,6 +86,10 @@
     void ask_client_to_close(const miral::Window &window);
     void forceClose(const miral::Window &window);
 
+    void dispatchKeyUp(const MirKeyboardEvent *kev);
+
+    static WindowManagementPolicy *instance();
+
 Q_SIGNALS:
 
 private:
@@ -93,6 +99,10 @@
     qtmir::WindowModelNotifier &m_windowModel;
     qtmir::AppNotifier &m_appNotifier;
     const QScopedPointer<QtEventFeeder> m_eventFeeder;
+
+    QHash<xkb_keysym_t, miral::Window> m_keyOwners;
+
+    static WindowManagementPolicy *m_instance;
 };
 
 #endif // WINDOWMANAGEMENTPOLICY_H