Add logging, clarifications, and tests for QWidget show/hide behavior

Task-number: QTBUG-121398
Pick-to: 6.7
Change-Id: I94b4c90c3bd515279417c88497d7b9bd5a362eae
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>

4 files changed, 180 insertions, 10 deletions

diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp
index e5e0ef478d..b8f38f0fc6 100644
--- a/src/widgets/kernel/qwidget.cpp
+++ b/src/widgets/kernel/qwidget.cpp
@@ -82,6 +82,14 @@ using namespace QNativeInterface::Private;
 using namespace Qt::StringLiterals;
 
 Q_LOGGING_CATEGORY(lcWidgetPainting, "qt.widgets.painting", QtWarningMsg);
+Q_LOGGING_CATEGORY(lcWidgetShowHide, "qt.widgets.showhide", QtWarningMsg);
+
+#ifndef QT_NO_DEBUG_STREAM
+namespace {
+    struct WidgetAttributes { const QWidget *widget; };
+    QDebug operator<<(QDebug debug, const WidgetAttributes &attributes);
+}
+#endif
 
 static inline bool qRectIntersects(const QRect &r1, const QRect &r2)
 {
@@ -8319,13 +8327,17 @@ void QWidgetPrivate::hide_sys()
 
 void QWidget::setVisible(bool visible)
 {
+    Q_D(QWidget);
+    qCDebug(lcWidgetShowHide) << "Setting visibility of" << this
+                              << "with attributes" << WidgetAttributes{this}
+                              << "to" << visible << "via QWidget";
+
     if (testAttribute(Qt::WA_WState_ExplicitShowHide) && testAttribute(Qt::WA_WState_Hidden) == !visible)
         return;
 
     // Remember that setVisible was called explicitly
     setAttribute(Qt::WA_WState_ExplicitShowHide);
 
-    Q_D(QWidget);
     d->setVisible(visible);
 }
 
@@ -8335,6 +8347,10 @@ void QWidget::setVisible(bool visible)
 void QWidgetPrivate::setVisible(bool visible)
 {
     Q_Q(QWidget);
+    qCDebug(lcWidgetShowHide) << "Setting visibility of" << q
+                              << "with attributes" << WidgetAttributes{q}
+                              << "to" << visible << "via QWidgetPrivate";
+
     if (visible) { // show
         // Designer uses a trick to make grabWidget work without showing
         if (!q->isWindow() && q->parentWidget() && q->parentWidget()->isVisible()
@@ -8454,14 +8470,20 @@ void QWidgetPrivate::_q_showIfNotHidden()
 
 void QWidgetPrivate::showChildren(bool spontaneous)
 {
+    Q_Q(QWidget);
+    qCDebug(lcWidgetShowHide) << "Showing children of" << q
+                              << "spontaneously" << spontaneous;
+
     QList<QObject*> childList = children;
     for (int i = 0; i < childList.size(); ++i) {
         QWidget *widget = qobject_cast<QWidget*>(childList.at(i));
-        if (widget && widget->windowHandle() && !widget->testAttribute(Qt::WA_WState_ExplicitShowHide))
+        if (!widget)
+            continue;
+        qCDebug(lcWidgetShowHide) << "Considering" << widget
+              << "with attributes" << WidgetAttributes{widget};
+        if (widget->windowHandle() && !widget->testAttribute(Qt::WA_WState_ExplicitShowHide))
             widget->setAttribute(Qt::WA_WState_Hidden, false);
-        if (!widget
-            || widget->isWindow()
-            || widget->testAttribute(Qt::WA_WState_Hidden))
+        if (widget->isWindow() || widget->testAttribute(Qt::WA_WState_Hidden))
             continue;
         if (spontaneous) {
             widget->setAttribute(Qt::WA_Mapped);
@@ -8480,10 +8502,17 @@ void QWidgetPrivate::showChildren(bool spontaneous)
 void QWidgetPrivate::hideChildren(bool spontaneous)
 {
     Q_Q(QWidget);
+    qCDebug(lcWidgetShowHide) << "Hiding children of" << q
+                              << "spontaneously" << spontaneous;
+
     QList<QObject*> childList = children;
     for (int i = 0; i < childList.size(); ++i) {
         QWidget *widget = qobject_cast<QWidget*>(childList.at(i));
-        if (!widget || widget->isWindow() || widget->testAttribute(Qt::WA_WState_Hidden))
+        if (!widget)
+            continue;
+        qCDebug(lcWidgetShowHide) << "Considering" << widget
+              << "with attributes" << WidgetAttributes{widget};
+        if (widget->isWindow() || widget->testAttribute(Qt::WA_WState_Hidden))
             continue;
 
         if (spontaneous)
@@ -8541,13 +8570,15 @@ void QWidgetPrivate::hideChildren(bool spontaneous)
 */
 bool QWidgetPrivate::handleClose(CloseMode mode)
 {
+    Q_Q(QWidget);
+    qCDebug(lcWidgetShowHide) << "Handling close event for" << q;
+
     if (data.is_closing)
         return true;
 
     // We might not have initiated the close, so update the state now that we know
     data.is_closing = true;
 
-    Q_Q(QWidget);
     QPointer<QWidget> that = q;
 
     if (data.in_destructor)
@@ -10714,7 +10745,20 @@ void QWidget::setParent(QWidget *parent, Qt::WindowFlags f)
 
     if (wasCreated) {
         if (!testAttribute(Qt::WA_WState_Hidden)) {
+            // Hiding the widget will set WA_WState_Hidden as well, which would
+            // normally require the widget to be explicitly shown again to become
+            // visible, even as a child widget. But we refine this value later in
+            // setParent_sys(), applying WA_WState_Hidden based on whether the
+            // widget is a top level or not.
             hide();
+
+            // We reset WA_WState_ExplicitShowHide here, likely as a remnant of
+            // when we only had QWidget::setVisible(), which is treated as an
+            // explicit show/hide. Nowadays we have QWidgetPrivate::setVisible(),
+            // that allows us to hide a widget without affecting ExplicitShowHide.
+            // Though it can be argued that ExplicitShowHide should reflect the
+            // last update of the widget's state, so if we hide the widget as a
+            // side effect of changing parent, perhaps we _should_ reset it?
             setAttribute(Qt::WA_WState_ExplicitShowHide, false);
         }
         if (newParent) {
@@ -13165,11 +13209,15 @@ void QWidgetPrivate::setNetWmWindowTypes(bool skipIfMissing)
 
 #ifndef QT_NO_DEBUG_STREAM
 
-static inline void formatWidgetAttributes(QDebug debug, const QWidget *widget)
+namespace {
+QDebug operator<<(QDebug debug, const WidgetAttributes &attributes)
 {
+    const QDebugStateSaver saver(debug);
+    debug.nospace();
+    const QWidget *widget = attributes.widget;
     const QMetaObject *qtMo = qt_getEnumMetaObject(Qt::WA_AttributeCount);
     const QMetaEnum me = qtMo->enumerator(qtMo->indexOfEnumerator("WidgetAttribute"));
-    debug << ", attributes=[";
+    debug << '[';
     int count = 0;
     for (int a = 0; a < Qt::WA_AttributeCount; ++a) {
         if (widget->testAttribute(static_cast<Qt::WidgetAttribute>(a))) {
@@ -13179,6 +13227,8 @@ static inline void formatWidgetAttributes(QDebug debug, const QWidget *widget)
         }
     }
     debug << ']';
+    return debug;
+}
 }
 
 QDebug operator<<(QDebug debug, const QWidget *widget)
@@ -13198,7 +13248,7 @@ QDebug operator<<(QDebug debug, const QWidget *widget)
                 debug << ", disabled";
             debug << ", states=" << widget->windowState()
                 << ", type=" << widget->windowType() << ", flags=" <<  widget->windowFlags();
-            formatWidgetAttributes(debug, widget);
+            debug << ", attributes=" << WidgetAttributes{widget};
             if (widget->isWindow())
                 debug << ", window";
             debug << ", " << geometry.width() << 'x' << geometry.height()
diff --git a/src/widgets/kernel/qwidget_p.h b/src/widgets/kernel/qwidget_p.h
index 942d284078..2c65aac175 100644
--- a/src/widgets/kernel/qwidget_p.h
+++ b/src/widgets/kernel/qwidget_p.h
@@ -50,6 +50,7 @@
 QT_BEGIN_NAMESPACE
 
 Q_DECLARE_LOGGING_CATEGORY(lcWidgetPainting);
+Q_DECLARE_LOGGING_CATEGORY(lcWidgetShowHide);
 
 // Extra QWidget data
 //  - to minimize memory usage for members that are seldom used.
diff --git a/src/widgets/kernel/qwidgetwindow.cpp b/src/widgets/kernel/qwidgetwindow.cpp
index ff666a7e49..26f1afbca6 100644
--- a/src/widgets/kernel/qwidgetwindow.cpp
+++ b/src/widgets/kernel/qwidgetwindow.cpp
@@ -39,6 +39,8 @@ public:
     void setVisible(bool visible) override
     {
         Q_Q(QWidgetWindow);
+        qCDebug(lcWidgetShowHide) << "Setting visibility of" << q->widget()
+               << "to" << visible << "via QWidgetWindowPrivate";
         if (QWidget *widget = q->widget()) {
             // Check if the widget was already hidden, as this indicates it was done
             // explicitly and not because the parent window in this case made it hidden.
@@ -196,6 +198,9 @@ QObject *QWidgetWindow::focusObject() const
 void QWidgetWindow::setNativeWindowVisibility(bool visible)
 {
     Q_D(QWidgetWindow);
+    qCDebug(lcWidgetShowHide) << "Setting visibility of" << this
+        << "to" << visible << "via QWidgetWindow::setNativeWindowVisibility";
+
     // Call base class setVisible() implementation to run the QWindow
     // visibility logic. Don't call QWidgetWindowPrivate::setVisible()
     // since that will recurse back into QWidget code.
diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
index d06cc547cd..7216a789de 100644
--- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
+++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
@@ -439,6 +439,8 @@ private slots:
 
     void setVisibleDuringDestruction();
 
+    void explicitShowHide();
+
     void dragEnterLeaveSymmetry();
 
 private:
@@ -13462,6 +13464,118 @@ void tst_QWidget::setVisibleDuringDestruction()
     QTRY_COMPARE(signalSpy.count(), 4);
 }
 
+void tst_QWidget::explicitShowHide()
+{
+    {
+        QWidget parent;
+        parent.setObjectName("Parent");
+        QWidget child(&parent);
+        child.setObjectName("Child");
+
+        QCOMPARE(parent.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(parent.testAttribute(Qt::WA_WState_Hidden), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+
+        parent.show();
+        QCOMPARE(parent.testAttribute(Qt::WA_WState_ExplicitShowHide), true);
+        QCOMPARE(parent.testAttribute(Qt::WA_WState_Hidden), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+
+        // Fix up earlier expected failure
+        child.setAttribute(Qt::WA_WState_ExplicitShowHide, false);
+
+        parent.hide();
+        QCOMPARE(parent.testAttribute(Qt::WA_WState_ExplicitShowHide), true);
+        QCOMPARE(parent.testAttribute(Qt::WA_WState_Hidden), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+    }
+
+    {
+        // Test what happens when a child is reparented after showing it
+
+        QWidget parent;
+        parent.setObjectName("Parent");
+        QWidget child;
+        child.setObjectName("Child");
+
+        child.show();
+        QCOMPARE(child.isVisible(), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+
+        child.setParent(&parent);
+        // As documented, a widget becomes invisible as part of changing
+        // its parent, even if it was previously visible. The user must call
+        // show() to make the widget visible again.
+        QCOMPARE(child.isVisible(), false);
+
+        // However, the widget does not end up with Qt::WA_WState_Hidden,
+        // as QWidget::setParent treats it as a child, which normally will
+        // not get Qt::WA_WState_Hidden out of the box.
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+
+        // For some reason we reset WA_WState_ExplicitShowHide, and it's
+        // not clear whether this is correct or not See QWidget::setParent()
+        // for a comment with more details.
+        QEXPECT_FAIL("", "We reset WA_WState_ExplicitShowHide on widget re-parent", Continue);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), true);
+
+        // The fact that the child doesn't have Qt::WA_WState_Hidden means
+        // it's sufficient to show the parent widget. We don't need to
+        // explicitly show the child.
+        parent.show();
+        QCOMPARE(child.isVisible(), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+    }
+
+    {
+        QWidget parent;
+        parent.setObjectName("Parent");
+        QWidget child(&parent);
+        child.setObjectName("Child");
+
+        parent.show();
+
+        // If a non-native child ends up being closed, we will hide the
+        // widget, but do so via QWidget::hide(), which marks the widget
+        // as explicitly hidden.
+
+        child.setAttribute(Qt::WA_WState_ExplicitShowHide, false);
+        QCOMPARE(child.close(), true);
+        QEXPECT_FAIL("", "Closing a non-native child is treated as an explicit hide", Continue);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), true);
+
+        child.show();
+        child.setAttribute(Qt::WA_NativeWindow);
+        child.setAttribute(Qt::WA_WState_ExplicitShowHide, false);
+        QCOMPARE(child.close(), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), true);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), true);
+
+        child.show();
+        child.setAttribute(Qt::WA_WState_ExplicitShowHide, false);
+        QCOMPARE(child.windowHandle()->close(), false); // Can't close non-top level QWindows
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), false);
+
+        // If we end up in QWidgetPrivate::handleClose via QWidgetWindow::closeEvent,
+        // either through QWindow::close(), or via QWSI::handleCloseEvent, we'll still
+        // do the explicit hide.
+
+        child.show();
+        child.setAttribute(Qt::WA_WState_ExplicitShowHide, false);
+        QCOMPARE(QWindowSystemInterface::handleCloseEvent<QWindowSystemInterface::SynchronousDelivery>(
+            child.windowHandle()), true);
+        QEXPECT_FAIL("", "Closing a native child via QWSI is treated as an explicit hide", Continue);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_ExplicitShowHide), false);
+        QCOMPARE(child.testAttribute(Qt::WA_WState_Hidden), true);
+    }
+}
+
 /*!
     Verify that we deliver DragEnter/Leave events symmetrically, even if the
     widget entered didn't accept the DragEnter event.