x11: Rework how X11 outputs are queried

Currently, there are a couple of issues with output querying on X11:

(a) if an output is changed, for example its transform has been changed,
    then all outputs will be destroyed and created again

(b) it's possible to encounter the case where the platform has no
    outputs. The X11Platform destroys all outputs, then queries new
    outputs. The Workspace and AbstractClient sub-classes handle having
    no outputs very poorly! It's even possible to hit a crash.

With this change, outputs will be queried similar to how it's done on
Wayland.

src/plugins/platforms/x11/standalone/x11_output.cpp

@@ -37,7 +37,10 @@ QRect X11Output::geometry() const
 
 void X11Output::setGeometry(QRect set)
 {
+    if (m_geometry != set) {
         m_geometry = set;
+        Q_EMIT geometryChanged();
+    }
 }
 
 int X11Output::refreshRate() const

src/plugins/platforms/x11/standalone/x11_platform.cpp

@@ -461,36 +461,15 @@ void X11StandalonePlatform::updateOutputs()
 template <typename T>
 void X11StandalonePlatform::doUpdateOutputs()
 {
-    auto fallback = [this]() {
+    QVector<AbstractOutput *> changed;
-        X11PlaceholderOutput *dummyOutput = new X11PlaceholderOutput();
+    QVector<AbstractOutput *> added;
-        m_outputs << dummyOutput;
+    QVector<AbstractOutput *> removed = m_outputs;
-        Q_EMIT outputAdded(dummyOutput);
-        Q_EMIT outputEnabled(dummyOutput);
-    };
 
-    // TODO: instead of resetting all outputs, check if new output is added/removed
+    if (Xcb::Extensions::self()->isRandrAvailable()) {
-    //       or still available and leave still available outputs in m_outputs
-    //       untouched (like in DRM backend)
-    while (!m_outputs.isEmpty()) {
-        AbstractOutput *output = m_outputs.takeLast();
-        Q_EMIT outputDisabled(output);
-        Q_EMIT outputRemoved(output);
-        delete output;
-    }
-
-    if (!Xcb::Extensions::self()->isRandrAvailable()) {
-        fallback();
-        Q_EMIT screensQueried();
-        return;
-    }
         T resources(rootWindow());
-    if (resources.isNull()) {
+        if (!resources.isNull()) {
-        fallback();
-        Q_EMIT screensQueried();
-        return;
-    }
             xcb_randr_crtc_t *crtcs = resources.crtcs();
-    xcb_randr_mode_info_t *modes = resources.modes();
+            const xcb_randr_mode_info_t *modes = resources.modes();
 
             QVector<Xcb::RandR::CrtcInfo> infos(resources->num_crtcs);
             for (int i = 0; i < resources->num_crtcs; ++i) {
@@ -500,6 +479,11 @@ void X11StandalonePlatform::doUpdateOutputs()
             for (int i = 0; i < resources->num_crtcs; ++i) {
                 Xcb::RandR::CrtcInfo info(infos.at(i));
 
+                const QRect geometry = info.rect();
+                if (!geometry.isValid()) {
+                    continue;
+                }
+
                 xcb_randr_output_t *outputs = info.outputs();
                 QVector<Xcb::RandR::OutputInfo> outputInfos(outputs ? resources->num_outputs : 0);
                 if (outputs) {
@@ -525,26 +509,32 @@ void X11StandalonePlatform::doUpdateOutputs()
                     }
                 }
 
-        const QRect geo = info.rect();
+                for (int j = 0; j < info->num_outputs; ++j) {
-        if (geo.isValid()) {
+                    Xcb::RandR::OutputInfo outputInfo(outputInfos.at(j));
-            xcb_randr_crtc_t crtc = crtcs[i];
+                    if (outputInfo->crtc != crtcs[i]) {
+                        continue;
+                    }
+
+                    X11Output *output = findX11Output(outputInfo.name());
+                    if (output) {
+                        changed.append(output);
+                        removed.removeOne(output);
+                    } else {
+                        output = new X11Output();
+                        output->setName(outputInfo.name());
+                        added.append(output);
+                    }
 
                     // TODO: Perhaps the output has to save the inherited gamma ramp and
                     // restore it during tear down. Currently neither standalone x11 nor
                     // drm platform do this.
-            Xcb::RandR::CrtcGamma gamma(crtc);
+                    Xcb::RandR::CrtcGamma gamma(crtcs[i]);
 
-            auto *o = new X11Output(this);
+                    output->setCrtc(crtcs[i]);
-            o->setCrtc(crtc);
+                    output->setGammaRampSize(gamma.isNull() ? 0 : gamma->size);
-            o->setGammaRampSize(gamma.isNull() ? 0 : gamma->size);
+                    output->setGeometry(geometry);
-            o->setGeometry(geo);
+                    output->setRefreshRate(refreshRate * 1000);
-            o->setRefreshRate(refreshRate * 1000);
 
-            for (int j = 0; j < info->num_outputs; ++j) {
-                Xcb::RandR::OutputInfo outputInfo(outputInfos.at(j));
-                if (outputInfo->crtc != crtc) {
-                    continue;
-                }
                     QSize physicalSize(outputInfo->mm_width, outputInfo->mm_height);
                     switch (info->rotation) {
                     case XCB_RANDR_ROTATION_ROTATE_0:
@@ -558,24 +548,50 @@ void X11StandalonePlatform::doUpdateOutputs()
                     case XCB_RANDR_ROTATION_REFLECT_Y:
                         break;
                     }
-                o->setName(outputInfo.name());
+                    output->setPhysicalSize(physicalSize);
-                o->setPhysicalSize(physicalSize);
                     break;
                 }
-
+            }
-            m_outputs << o;
-            Q_EMIT outputAdded(o);
-            Q_EMIT outputEnabled(o);
         }
     }
 
-    if (m_outputs.isEmpty()) {
+    // The workspace handles having no outputs poorly. If the last output is about to be
-        fallback();
+    // removed, create a dummy output to avoid crashing.
+    if (changed.isEmpty() && added.isEmpty()) {
+        auto dummyOutput = new X11PlaceholderOutput();
+        m_outputs << dummyOutput;
+        Q_EMIT outputAdded(dummyOutput);
+        Q_EMIT outputEnabled(dummyOutput);
+    }
+
+    // Process new outputs. Note new outputs must be introduced before removing any other outputs.
+    for (AbstractOutput *output : qAsConst(added)) {
+        m_outputs.append(output);
+        Q_EMIT outputAdded(output);
+        Q_EMIT outputEnabled(output);
+    }
+
+    // Outputs have to be removed last to avoid the case where there are no enabled outputs.
+    for (AbstractOutput *output : qAsConst(removed)) {
+        m_outputs.removeOne(output);
+        Q_EMIT outputDisabled(output);
+        Q_EMIT outputRemoved(output);
+        delete output;
     }
 
     Q_EMIT screensQueried();
 }
 
+X11Output *X11StandalonePlatform::findX11Output(const QString &name) const
+{
+    for (AbstractOutput *output : m_outputs) {
+        if (output->name() == name) {
+            return qobject_cast<X11Output *>(output);
+        }
+    }
+    return nullptr;
+}
+
 Outputs X11StandalonePlatform::outputs() const
 {
     return m_outputs;

src/plugins/platforms/x11/standalone/x11_platform.h

@@ -84,6 +84,7 @@ private:
      */
     static bool hasGlx();
 
+    X11Output *findX11Output(const QString &name) const;
     template <typename T>
     void doUpdateOutputs();
     void updateRefreshRate();