macOS: Stop relying on balanced CGDisplay reconfiguration callbacks

We were using CGDisplay callbacks to determine when a screen reconfiguration
had happened, and when it had propagated to changes in NSScreen.screens,
so that we could update our QScreen view of the world.

Unfortunately the CGDisplay callbacks were not deterministic enough to
use as a signal for when a reconfigure had completed.

Since we can't rely on NSApplicationDidChangeScreenParametersNotification
either (it comes in too late), we're now resorting to updating our QScreens
at every chance we get:

 - On every CGDisplay reconfiguration ending
 - On QCocoaWindow::windowDidChangeScreen() as a result of AppKit
   moving the window.
 - On NSApplicationDidChangeScreenParametersNotification
 - On QCocoaScreen::get() as a last resort

Since the result of these updates are only reflected as QScreen property
updates or QGuiApplication signals if a change actually occurred, it should
be safe to update early and often.

Task-number: QTBUG-77656
Fixes: QTBUG-80193
Pick-to: 6.2 6.1 5.15
Change-Id: I98334a66767736d94ad2fcb169e65f0d8bc71a30
Reviewed-by: Morten Johan Sørvig <morten.sorvig@qt.io>

3 files changed, 30 insertions, 84 deletions

diff --git a/src/plugins/platforms/cocoa/qcocoascreen.h b/src/plugins/platforms/cocoa/qcocoascreen.h
index 0eb6d114e2..91c1e3c63c 100644
--- a/src/plugins/platforms/cocoa/qcocoascreen.h
+++ b/src/plugins/platforms/cocoa/qcocoascreen.h
@@ -106,9 +106,6 @@ private:
     static QMacNotificationObserver s_screenParameterObserver;
     static CGDisplayReconfigurationCallBack s_displayReconfigurationCallBack;
 
-    static bool updateScreensIfNeeded();
-    static NSArray *s_screenConfigurationBeforeUpdate;
-
     static void add(CGDirectDisplayID displayId);
     QCocoaScreen(CGDirectDisplayID displayId);
     void update(CGDirectDisplayID displayId);
diff --git a/src/plugins/platforms/cocoa/qcocoascreen.mm b/src/plugins/platforms/cocoa/qcocoascreen.mm
index d326d149f1..e419492202 100644
--- a/src/plugins/platforms/cocoa/qcocoascreen.mm
+++ b/src/plugins/platforms/cocoa/qcocoascreen.mm
@@ -74,7 +74,6 @@ namespace CoreGraphics {
     Q_ENUM_NS(DisplayChange)
 }
 
-NSArray *QCocoaScreen::s_screenConfigurationBeforeUpdate = nil;
 QMacNotificationObserver QCocoaScreen::s_screenParameterObserver;
 CGDisplayReconfigurationCallBack QCocoaScreen::s_displayReconfigurationCallBack = nullptr;
 
@@ -85,83 +84,23 @@ void QCocoaScreen::initializeScreens()
     s_displayReconfigurationCallBack = [](CGDirectDisplayID displayId, CGDisplayChangeSummaryFlags flags, void *userInfo) {
         Q_UNUSED(userInfo);
 
-        // Displays are reconfigured in batches, and we want to update our screens
-        // once a batch ends, so that all the states of the displays are up to date.
-        static int displayReconfigurationsInProgress = 0;
-
         const bool beforeReconfigure = flags & kCGDisplayBeginConfigurationFlag;
-        qCDebug(lcQpaScreen).verbosity(0).nospace() << "Display " << displayId
-                << (beforeReconfigure ? " about to reconfigure" : " was ")
-                << QFlags<CoreGraphics::DisplayChange>(flags)
-                << " with " << displayReconfigurationsInProgress
-                << " display configuration(s) in progress";
-
-        if (!flags) {
-            // CGDisplayRegisterReconfigurationCallback has been observed to be called
-            // with flags unset. This seems like a bug. The callback is not paired with
-            // a matching "completion" callback either, so we don't know whether to treat
-            // it as a begin or end of reconfigure.
-            return;
-        }
+        qCDebug(lcQpaScreen).verbosity(0) << "Display" << displayId
+                << (beforeReconfigure ? "beginning" : "finished") << "reconfigure"
+                << QFlags<CoreGraphics::DisplayChange>(flags);
 
-        if (beforeReconfigure) {
-            if (!displayReconfigurationsInProgress++) {
-                // There might have been a screen reconfigure before this that
-                // we didn't process yet, so do that now if that's the case.
-                updateScreensIfNeeded();
-
-                Q_ASSERT(!s_screenConfigurationBeforeUpdate);
-                s_screenConfigurationBeforeUpdate = NSScreen.screens;
-                qCDebug(lcQpaScreen, "Display reconfigure transaction started"
-                    " with screen configuration %p", s_screenConfigurationBeforeUpdate);
-
-                static void (^tryScreenUpdate)();
-                tryScreenUpdate = ^void () {
-                    qCDebug(lcQpaScreen) << "Attempting screen update from runloop block";
-                    if (!updateScreensIfNeeded())
-                        CFRunLoopPerformBlock(CFRunLoopGetMain(), kCFRunLoopCommonModes, tryScreenUpdate);
-                };
-                CFRunLoopPerformBlock(CFRunLoopGetMain(), kCFRunLoopCommonModes, tryScreenUpdate);
-            }
-        } else {
-            Q_ASSERT_X(displayReconfigurationsInProgress, "QCococaScreen",
-                "Display configuration transactions are expected to be balanced");
-
-            if (!--displayReconfigurationsInProgress) {
-                qCDebug(lcQpaScreen) << "Display reconfigure transaction completed";
-                // We optimistically update now, in case the NSScreens have changed
-                updateScreensIfNeeded();
-            }
-        }
+        if (!beforeReconfigure)
+            updateScreens();
     };
     CGDisplayRegisterReconfigurationCallback(s_displayReconfigurationCallBack, nullptr);
 
     s_screenParameterObserver = QMacNotificationObserver(NSApplication.sharedApplication,
         NSApplicationDidChangeScreenParametersNotification, [&]() {
             qCDebug(lcQpaScreen) << "Received screen parameter change notification";
-            updateScreensIfNeeded(); // As a last resort we update screens here
+            updateScreens();
         });
 }
 
-bool QCocoaScreen::updateScreensIfNeeded()
-{
-    if (!s_screenConfigurationBeforeUpdate) {
-        qCDebug(lcQpaScreen) << "QScreens have already been updated, all good";
-        return true;
-    }
-
-    if (s_screenConfigurationBeforeUpdate == NSScreen.screens) {
-        qCDebug(lcQpaScreen) << "Still waiting for NSScreen configuration change";
-        return false;
-    }
-
-    qCDebug(lcQpaScreen, "NSScreen configuration changed to %p", NSScreen.screens);
-    updateScreens();
-
-    s_screenConfigurationBeforeUpdate = nil;
-    return true;
-}
-
 /*
     Update the list of available QScreens, and the properties of existing screens.
 
@@ -321,15 +260,18 @@ void QCocoaScreen::update(CGDirectDisplayID displayId)
 
     Q_ASSERT(isOnline());
 
+    // Some properties are only available via NSScreen
+    NSScreen *nsScreen = nativeScreen();
+    if (!nsScreen) {
+        qCDebug(lcQpaScreen) << "Corresponding NSScreen not yet available. Deferring update";
+        return;
+    }
+
     const QRect previousGeometry = m_geometry;
     const QRect previousAvailableGeometry = m_availableGeometry;
     const QDpi previousLogicalDpi = m_logicalDpi;
     const qreal previousRefreshRate = m_refreshRate;
 
-    // Some properties are only available via NSScreen
-    NSScreen *nsScreen = nativeScreen();
-    Q_ASSERT(nsScreen);
-
     // The reference screen for the geometry is always the primary screen
     QRectF primaryScreenGeometry = QRectF::fromCGRect(CGDisplayBounds(CGMainDisplayID()));
     m_geometry = qt_mac_flip(QRectF::fromCGRect(nsScreen.frame), primaryScreenGeometry).toRect();
@@ -759,13 +701,17 @@ QList<QPlatformScreen*> QCocoaScreen::virtualSiblings() const
 
 QCocoaScreen *QCocoaScreen::get(NSScreen *nsScreen)
 {
-    if (s_screenConfigurationBeforeUpdate) {
-        qCWarning(lcQpaScreen) << "Trying to resolve screen while waiting for screen reconfigure!";
-        if (!updateScreensIfNeeded())
-            qCWarning(lcQpaScreen) << "Failed to do last minute screen update. Expect crashes.";
+    auto displayId = nsScreen.qt_displayId;
+    auto *cocoaScreen = get(displayId);
+    if (!cocoaScreen) {
+        qCWarning(lcQpaScreen) << "Failed to map" << nsScreen
+            << "to QCocoaScreen. Doing last minute update.";
+        updateScreens();
+        cocoaScreen = get(displayId);
+        if (!cocoaScreen)
+            qCWarning(lcQpaScreen) << "Last minute update failed!";
     }
-
-    return get(nsScreen.qt_displayId);
+    return cocoaScreen;
 }
 
 QCocoaScreen *QCocoaScreen::get(CGDirectDisplayID displayId)
diff --git a/src/plugins/platforms/cocoa/qcocoawindow.mm b/src/plugins/platforms/cocoa/qcocoawindow.mm
index ee1689c2f4..7270e7c23c 100644
--- a/src/plugins/platforms/cocoa/qcocoawindow.mm
+++ b/src/plugins/platforms/cocoa/qcocoawindow.mm
@@ -1268,11 +1268,15 @@ void QCocoaWindow::windowDidChangeScreen()
         return;
 
     // Note: When a window is resized to 0x0 Cocoa will report the window's screen as nil
-    auto *currentScreen = QCocoaScreen::get(m_view.window.screen);
+    NSScreen *nsScreen = m_view.window.screen;
+
+    qCDebug(lcQpaWindow) << window() << "did change" << nsScreen;
+    QCocoaScreen::updateScreens();
+
     auto *previousScreen = static_cast<QCocoaScreen*>(screen());
+    auto *currentScreen = QCocoaScreen::get(nsScreen);
 
-    Q_ASSERT_X(!m_view.window.screen || currentScreen,
-        "QCocoaWindow", "Failed to get QCocoaScreen for NSScreen");
+    qCDebug(lcQpaWindow) << "Screen changed for" << window() << "from" << previousScreen << "to" << currentScreen;
 
     // Note: The previous screen may be the same as the current screen, either because
     // a) the screen was just reconfigured, which still results in AppKit sending an
@@ -1285,7 +1289,6 @@ void QCocoaWindow::windowDidChangeScreen()
     // device-pixel ratio may have changed, and needs to be delivered to all
     // windows, both top level and child windows.
 
-    qCDebug(lcQpaWindow) << "Screen changed for" << window() << "from" << previousScreen << "to" << currentScreen;
     QWindowSystemInterface::handleWindowScreenChanged<QWindowSystemInterface::SynchronousDelivery>(
         window(), currentScreen ? currentScreen->screen() : nullptr);