qtubuntu

Merge lp:~nick-dedekind/qtubuntu/1623861.window-focus into lp:qtubuntu

1623861.window-focus
Merge into trunk

Proposed by Nick Dedekind on 2016-09-21

Status:

Merged

Approved by:

Daniel d'Andrada on 2016-09-28

Approved revision:

346

Merged at revision:

344

Proposed branch:

lp:~nick-dedekind/qtubuntu/1623861.window-focus

Merge into:

lp:qtubuntu

Diff against target:

210 lines (+46/-41)

4 files modified

src/ubuntumirclient/input.cpp (+5/-31)
src/ubuntumirclient/input.h (+2/-4)
src/ubuntumirclient/window.cpp (+38/-5)
src/ubuntumirclient/window.h (+1/-1)

To merge this branch:

bzr merge lp:~nick-dedekind/qtubuntu/1623861.window-focus

Undecided

Invalid

Link a bug report

Reviewer	Review Type	Date Requested	Status
Daniel d'Andrada (community)		2016-09-21	Approve on 2016-09-28
Unity8 CI Bot	continuous-integration		Approve on 2016-09-28
Review via email: mp+306318@code.launchpad.net

Commit Message

Moved focus handling to UbuntuWindow to ensure focus optimization supports multiple windows rather than being application global.

Description of the Change

Moved focus handling to UbuntuWindow to ensure focus optimization supports multiple windows rather than being application global.

* Are there any related MPs required for this MP to build/function as expected? Please list.
No

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-09-21:

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

review: Approve (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2016-09-27:

Looks good. just one minor thing:

"""
// Mir may have sent a pair of focus lost/gained events, so we need to "peek" into the queue
// so that we don't deactivate windows prematurely.
"""

Please move that comment from UbuntuInput to UbuntuWindow as well (instead of just deleting it). This mPendingFocusGainedEvents scheme is non-obvious and thus deserves a comment.

Your commit message is also too vague. What you did was making the focus-change optimization logic support multiple windows (by moving it to UbuntuWindow and thus making it per-window).

review: Needs Fixing

Nick Dedekind (nick-dedekind) wrote on 2016-09-28:

> Looks good. just one minor thing:
>
> """
> // Mir may have sent a pair of focus lost/gained events, so we need to "peek"
> into the queue
> // so that we don't deactivate windows prematurely.
> """
>
> Please move that comment from UbuntuInput to UbuntuWindow as well (instead of
> just deleting it). This mPendingFocusGainedEvents scheme is non-obvious and
> thus deserves a comment.

Done

>
> Your commit message is also too vague. What you did was making the focus-
> change optimization logic support multiple windows (by moving it to
> UbuntuWindow and thus making it per-window).

Done

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-09-28:

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

review: Approve (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2016-09-28:

Thanks!

review: Approve

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-09-28:

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

review: Approve (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2016-09-28:

Please either rebase on top of lp:~gerboland/qtubuntu/stricter-enums or remove the fix you did in the debug message of UbuntuWindow::handleSurfaceVisibilityChanged (the other branch as a better fix for that message).

Otherwise they will conflict.

review: Needs Fixing

lp:~nick-dedekind/qtubuntu/1623861.window-focus updated on 2016-09-28

346. By Nick Dedekind on 2016-09-28: removed debug fix

Nick Dedekind (nick-dedekind) wrote on 2016-09-28:

> Please either rebase on top of lp:~gerboland/qtubuntu/stricter-enums or remove
> the fix you did in the debug message of
> UbuntuWindow::handleSurfaceVisibilityChanged (the other branch as a better fix
> for that message).
>
> Otherwise they will conflict.

removed changed.

Nick Dedekind (nick-dedekind) wrote on 2016-09-28:

removed changed.

Daniel d'Andrada (dandrader) wrote on 2016-09-28:

Merges fine now, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel d'Andrada

Nick Dedekind

Ubuntu Phablet Team

 === modified file 'src/ubuntumirclient/input.cpp'
 --- src/ubuntumirclient/input.cpp	2016-08-24 12:40:30 +0000
 +++ src/ubuntumirclient/input.cpp	2016-09-28 10:34:24 +0000
@@ -188,7 +188,7 @@
      , mEventFilterType(static_cast<UbuntuNativeInterface*>(
          integration->nativeInterface())->genericEventFilterType())
      , mEventType(static_cast<QEvent::Type>(QEvent::registerEventType()))
--    , mLastFocusedWindow(nullptr)
++    , mLastInputWindow(nullptr)
+ {
      // Initialize touch device.
      mTouchDevice = new QTouchDevice;
@@ -300,17 +300,6 @@
+ {
      QWindow *window = platformWindow->window();
--    const auto eventType = mir_event_get_type(event);
--    if (mir_event_type_surface == eventType) {
--        auto surfaceEvent = mir_event_get_surface_event(event);
--        if (mir_surface_attrib_focus == mir_surface_event_get_attribute(surfaceEvent)) {
--            const bool focused = mir_surface_event_get_attribute_value(surfaceEvent) == mir_surface_focused;
--            if (focused) {
--                mPendingFocusGainedEvents++;
--            }
--        }
--    }
--
      QCoreApplication::postEvent(this, new UbuntuEvent(
              platformWindow, event, mEventType));
@@ -370,7 +359,7 @@
          switch (touch_action)
+         {
          case mir_touch_action_down:
--            mLastFocusedWindow = window;
++            mLastInputWindow = window;
              touchPoint.state = Qt::TouchPointPressed;
              break;
          case mir_touch_action_up:
@@ -452,7 +441,7 @@
          ? QEvent::KeyRelease : QEvent::KeyPress;
      if (action == mir_keyboard_action_down)
--        mLastFocusedWindow = window;
++        mLastInputWindow = window;
      QString text;
      QVarLengthArray<char, 32> chars(32);
@@ -605,23 +594,8 @@
      switch (surfaceEventAttribute) {
      case mir_surface_attrib_focus: {
--        const bool focused = mir_surface_event_get_attribute_value(event) == mir_surface_focused;
--        // Mir may have sent a pair of focus lost/gained events, so we need to "peek" into the queue
--        // so that we don't deactivate windows prematurely.
--        if (focused) {
--            mPendingFocusGainedEvents--;
--            QWindowSystemInterface::handleWindowActivated(window->window(), Qt::ActiveWindowFocusReason);
--
--            // NB: Since processing of system events is queued, never check qGuiApp->applicationState()
--            //     as it might be outdated. Always call handleApplicationStateChanged() with the latest
--            //     state regardless.
--            QWindowSystemInterface::handleApplicationStateChanged(Qt::ApplicationActive);
--
--        } else if(!mPendingFocusGainedEvents) {
--            qCDebug(ubuntumirclient, "No windows have focus");
--            QWindowSystemInterface::handleWindowActivated(nullptr, Qt::ActiveWindowFocusReason);
--            QWindowSystemInterface::handleApplicationStateChanged(Qt::ApplicationInactive);
--        }
++        window->handleSurfaceFocusChanged(
++                    mir_surface_event_get_attribute_value(event) == mir_surface_focused);
          break;
+     }
      case mir_surface_attrib_visibility:
 === modified file 'src/ubuntumirclient/input.h'
 --- src/ubuntumirclient/input.h	2016-04-28 14:58:55 +0000
 +++ src/ubuntumirclient/input.h	2016-09-28 10:34:24 +0000
@@ -19,7 +19,6 @@
  // Qt
  #include <qpa/qwindowsysteminterface.h>
--#include <QAtomicInt>
  #include <QLoggingCategory>
  #include <mir_toolkit/mir_client_library.h>
@@ -40,7 +39,7 @@
      void postEvent(UbuntuWindow* window, const MirEvent *event);
      UbuntuClientIntegration* integration() const { return mIntegration; }
--    UbuntuWindow *lastFocusedWindow() const {return mLastFocusedWindow; }
++    UbuntuWindow *lastInputWindow() const {return mLastInputWindow; }
  protected:
      void dispatchKeyEvent(UbuntuWindow *window, const MirInputEvent *event);
@@ -58,8 +57,7 @@
      const QByteArray mEventFilterType;
      const QEvent::Type mEventType;
--    UbuntuWindow *mLastFocusedWindow;
--    QAtomicInt mPendingFocusGainedEvents;
++    UbuntuWindow *mLastInputWindow;
  };
  #endif // UBUNTU_INPUT_H
 === modified file 'src/ubuntumirclient/window.cpp'
 --- src/ubuntumirclient/window.cpp	2016-08-31 01:51:15 +0000
 +++ src/ubuntumirclient/window.cpp	2016-09-28 10:34:24 +0000
@@ -27,8 +27,10 @@
  #include <qpa/qwindowsysteminterface.h>
  #include <QMutexLocker>
  #include <QSize>
++#include <QAtomicInt>
  #include <QtMath>
  #include <private/qeglconvenience_p.h>
++#include <private/qguiapplication_p.h>
  // Platform API
  #include <ubuntu/application/instance.h>
@@ -179,7 +181,7 @@
              //NOTE: We cannot have a parentless popup -
              //try using the last surface to receive input as that will most likely be
              //the one that caused this popup to be created
--            parent = input->lastFocusedWindow();
++            parent = input->lastInputWindow();
+         }
          if (parent) {
              auto pos = geom.topLeft();
@@ -399,6 +401,7 @@
      QSurfaceFormat format() const { return mFormat; }
      bool mNeedsExposeCatchup;
++    QAtomicInt mPendingFocusGainedEvents;
      QString persistentSurfaceId();
@@ -549,7 +552,8 @@
  void UbuntuSurface::postEvent(const MirEvent *event)
+ {
--    if (mir_event_type_resize == mir_event_get_type(event)) {
++    const auto eventType = mir_event_get_type(event);
++    if (mir_event_type_resize == eventType) {
          // TODO: The current event queue just accumulates all resize events;
          // It would be nicer if we could update just one event if that event has not been dispatched.
          // As a workaround, we use the width/height as an identifier of this latest event
@@ -562,6 +566,14 @@
          QMutexLocker lock(&mTargetSizeMutex);
          mTargetSize.rwidth() = width;
          mTargetSize.rheight() = height;
++    } else if (mir_event_type_surface == eventType) {
++        auto surfaceEvent = mir_event_get_surface_event(event);
++        if (mir_surface_attrib_focus == mir_surface_event_get_attribute(surfaceEvent)) {
++            const bool focused = mir_surface_event_get_attribute_value(surfaceEvent) == mir_surface_focused;
++            if (focused) {
++                mPendingFocusGainedEvents++;
++            }
++        }
+     }
      mInput->postEvent(mPlatformWindow, event);
@@ -647,10 +659,31 @@
      QWindowSystemInterface::handleExposeEvent(window(), QRect(QPoint(), geometry().size()));
+ }
--void UbuntuWindow::handleSurfaceFocused()
++void UbuntuWindow::handleSurfaceFocusChanged(bool focused)
+ {
--    qCDebug(ubuntumirclient, "handleSurfaceFocused(window=%p)", window());
--
++    qCDebug(ubuntumirclient, "handleSurfaceFocusChanged(window=%p, focused=%d, pending=%d)", window(), focused, mSurface->mPendingFocusGainedEvents.load());
++
++    // Mir may have sent a pair of focus lost/gained events, so we need to "peek" into the queue
++    // so that we don't deactivate windows prematurely.
++    if (focused) {
++        mSurface->mPendingFocusGainedEvents--;
++        QWindowSystemInterface::handleWindowActivated(window(), Qt::ActiveWindowFocusReason);
++
++        // NB: Since processing of system events is queued, never check qGuiApp->applicationState()
++        //     as it might be outdated. Always call handleApplicationStateChanged() with the latest
++        //     state regardless.
++        QWindowSystemInterface::handleApplicationStateChanged(Qt::ApplicationActive);
++
++        // Flush events so that we update QGuiApplicationPrivate::focus_window immediately
++        QWindowSystemInterface::flushWindowSystemEvents();
++
++    } else if (mSurface->mPendingFocusGainedEvents == 0 && window() == QGuiApplicationPrivate::focus_window) {
++        QWindowSystemInterface::handleWindowActivated(nullptr, Qt::ActiveWindowFocusReason);
++        QWindowSystemInterface::handleApplicationStateChanged(Qt::ApplicationInactive);
++
++        // Flush events so that we update QGuiApplicationPrivate::focus_window immediately
++        QWindowSystemInterface::flushWindowSystemEvents();
++    }
+ }
  void UbuntuWindow::handleSurfaceVisibilityChanged(bool visible)
 === modified file 'src/ubuntumirclient/window.h'
 --- src/ubuntumirclient/window.h	2016-08-24 12:40:30 +0000
 +++ src/ubuntumirclient/window.h	2016-09-28 10:34:24 +0000
@@ -63,7 +63,7 @@
      MirSurface *mirSurface() const;
      void handleSurfaceResized(int width, int height);
      void handleSurfaceExposeChange(bool exposed);
--    void handleSurfaceFocused();
++    void handleSurfaceFocusChanged(bool focused);
      void handleSurfaceVisibilityChanged(bool visible);
      void handleSurfaceStateChanged(Qt::WindowState state);
      void onSwapBuffersDone();