From 0525d640cd11ddced2ec418be182c585204fc45f Mon Sep 17 00:00:00 2001 From: Mitch Curtis Date: Fri, 12 Apr 2019 15:21:42 +0200 Subject: Fix MenuItem width not matching Menu's available width Short version: There are currently two problems with MenuItems: - Mirrored MenuItems don't fill the Menu's available width. - MenuItem does not fill the Menu's available width when changed after Component completion. This patch fixes both of them by listening to geometry changes in both the contentItem and individual menu items, and setting the explicit width of those menu items when either changes. Longer version: The first problem can be seen whenever the MenuItem's implicitWidth changes: - QQmlEngine::retranslate() is called, causing all bindings to be re-evaluated - The MenuItem's font size changes - The MenuItem's icon size changes - etc. We fix this by making Menu listen to the width of each of its MenuItems and call resizeItem() if it doesn't have an explicit width. The second problem can be seen when e.g. resizing a Menu to account for new items that are wider and hence require more space. This can be fixed by listening to width changes in Menu's contentItem, which was actually done in earlier versions but (probably accidentally) removed in 482ecb0f. I had tried to solve both issues by setting the explicit width of MenuItem to the width of its Menu, or undefined if it has none (which means it reverts to its implicit width). However, this does not account for e.g. MenuSeparator and custom items that can be added to Menu - they should also have their width fill the Menu automatically if they don't have an explicit width set. Change-Id: I95dd0da0919a1e297f2e2030da746ff1f1a17644 Fixes: QTBUG-75051 Fixes: QTBUG-75142 Reviewed-by: Richard Moe Gustavsen --- src/quicktemplates2/qquickmenu.cpp | 24 ++++- tests/auto/qquickmenu/data/menuItemWidths.qml | 103 ++++++++++++++++++ tests/auto/qquickmenu/tst_qquickmenu.cpp | 150 ++++++++++++++++++++++++++ 3 files changed, 273 insertions(+), 4 deletions(-) create mode 100644 tests/auto/qquickmenu/data/menuItemWidths.qml diff --git a/src/quicktemplates2/qquickmenu.cpp b/src/quicktemplates2/qquickmenu.cpp index f91d15a5..aa44e845 100644 --- a/src/quicktemplates2/qquickmenu.cpp +++ b/src/quicktemplates2/qquickmenu.cpp @@ -213,6 +213,7 @@ void QQuickMenuPrivate::insertItem(int index, QQuickItem *item) if (complete) resizeItem(item); QQuickItemPrivate::get(item)->addItemChangeListener(this, QQuickItemPrivate::Destroyed | QQuickItemPrivate::Parent); + QQuickItemPrivate::get(item)->updateOrAddGeometryChangeListener(this, QQuickGeometryChange::Width); contentModel->insert(index, item); QQuickMenuItem *menuItem = qobject_cast(item); @@ -237,6 +238,7 @@ void QQuickMenuPrivate::removeItem(int index, QQuickItem *item) contentData.removeOne(item); QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Destroyed | QQuickItemPrivate::Parent); + QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::Geometry); item->setParentItem(nullptr); contentModel->remove(index); @@ -358,10 +360,20 @@ void QQuickMenuPrivate::itemDestroyed(QQuickItem *item) removeItem(index, item); } -void QQuickMenuPrivate::itemGeometryChanged(QQuickItem *, QQuickGeometryChange, const QRectF &) +void QQuickMenuPrivate::itemGeometryChanged(QQuickItem *item, QQuickGeometryChange, const QRectF &) { - if (complete) + if (!complete) + return; + + if (item == contentItem) { + // The contentItem's geometry changed, so resize any items + // that don't have explicit widths set so that they fill the width of the menu. resizeItems(); + } else { + // The geometry of an item in the menu changed. If the item + // doesn't have an explicit width set, make it fill the width of the menu. + resizeItem(item); + } } QQuickPopupPositioner *QQuickMenuPrivate::getPositioner() @@ -1382,10 +1394,14 @@ void QQuickMenu::contentItemChange(QQuickItem *newItem, QQuickItem *oldItem) Q_D(QQuickMenu); QQuickPopup::contentItemChange(newItem, oldItem); - if (oldItem) + if (oldItem) { QQuickItemPrivate::get(oldItem)->removeItemChangeListener(d, QQuickItemPrivate::Children); - if (newItem) + QQuickItemPrivate::get(oldItem)->removeItemChangeListener(d, QQuickItemPrivate::Geometry); + } + if (newItem) { QQuickItemPrivate::get(newItem)->addItemChangeListener(d, QQuickItemPrivate::Children); + QQuickItemPrivate::get(newItem)->updateOrAddGeometryChangeListener(d, QQuickGeometryChange::Width); + } d->contentItem = newItem; } diff --git a/tests/auto/qquickmenu/data/menuItemWidths.qml b/tests/auto/qquickmenu/data/menuItemWidths.qml new file mode 100644 index 00000000..6d2baa32 --- /dev/null +++ b/tests/auto/qquickmenu/data/menuItemWidths.qml @@ -0,0 +1,103 @@ +/**************************************************************************** +** +** Copyright (C) 2019 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:BSD$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** BSD License Usage +** Alternatively, you may use this file under the terms of the BSD license +** as follows: +** +** "Redistribution and use in source and binary forms, with or without +** modification, are permitted provided that the following conditions are +** met: +** * Redistributions of source code must retain the above copyright +** notice, this list of conditions and the following disclaimer. +** * Redistributions in binary form must reproduce the above copyright +** notice, this list of conditions and the following disclaimer in +** the documentation and/or other materials provided with the +** distribution. +** * Neither the name of The Qt Company Ltd nor the names of its +** contributors may be used to endorse or promote products derived +** from this software without specific prior written permission. +** +** +** THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +** "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +** LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +** A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +** OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +** SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +** LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +** DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +** THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +** (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +** OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE." +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +import QtQuick 2.12 +import QtQuick.Controls 2.12 + +ApplicationWindow { + width: 600 + height: 600 + + property alias menu: menu + + Menu { + id: menu + MenuItem { + objectName: "MenuItem" + text: "MenuItem" + } + MenuSeparator { + objectName: "MenuSeparator" + } + Menu { + title: "Sub-menu" + objectName: "Sub-menu" + + MenuItem { + objectName: "SubMenuItem" + text: "SubMenuItem" + } + } + Rectangle { + objectName: "CustomSeparator" + height: 2 + color: "salmon" + } + Rectangle { + // Use a binding to test retranslate(), which re-evaluates all bindings. + implicitWidth: someValue + objectName: "CustomRectangleSeparator" + height: 2 + color: "salmon" + + property int someValue: 120 + } + Control { + objectName: "CustomControlSeparator" + implicitWidth: someOtherValue + height: 2 + background: Rectangle { + color: "navajowhite" + } + + property int someOtherValue: 180 + } + } +} diff --git a/tests/auto/qquickmenu/tst_qquickmenu.cpp b/tests/auto/qquickmenu/tst_qquickmenu.cpp index 1b00817f..49fdc066 100644 --- a/tests/auto/qquickmenu/tst_qquickmenu.cpp +++ b/tests/auto/qquickmenu/tst_qquickmenu.cpp @@ -94,6 +94,13 @@ private slots: void scrollable(); void disableWhenTriggered_data(); void disableWhenTriggered(); + void menuItemWidth_data(); + void menuItemWidth(); + void menuItemWidthAfterMenuWidthChanged_data(); + void menuItemWidthAfterMenuWidthChanged(); + void menuItemWidthAfterImplicitWidthChanged_data(); + void menuItemWidthAfterImplicitWidthChanged(); + void menuItemWidthAfterRetranslate(); }; void tst_QQuickMenu::defaults() @@ -1668,6 +1675,149 @@ void tst_QQuickMenu::disableWhenTriggered() } } +void tst_QQuickMenu::menuItemWidth_data() +{ + QTest::addColumn("mirrored"); + + QTest::newRow("non-mirrored") << false; + QTest::newRow("mirrored") << true; +} + +void tst_QQuickMenu::menuItemWidth() +{ + QFETCH(bool, mirrored); + + QQuickApplicationHelper helper(this, QLatin1String("menuItemWidths.qml")); + QQuickApplicationWindow *window = helper.appWindow; + window->show(); + QVERIFY(QTest::qWaitForWindowActive(window)); + + if (mirrored) + window->setLocale(QLocale("ar_EG")); + + QQuickMenu *menu = window->property("menu").value(); + QVERIFY(menu); + menu->open(); + QTRY_VERIFY(menu->isOpened()); + for (int i = 0; i < menu->count(); ++i) + QCOMPARE(menu->itemAt(i)->width(), menu->availableWidth()); +} + +void tst_QQuickMenu::menuItemWidthAfterMenuWidthChanged_data() +{ + QTest::addColumn("mirrored"); + + QTest::newRow("non-mirrored") << false; + QTest::newRow("mirrored") << true; +} + +void tst_QQuickMenu::menuItemWidthAfterMenuWidthChanged() +{ + QFETCH(bool, mirrored); + + QQuickApplicationHelper helper(this, QLatin1String("menuItemWidths.qml")); + QQuickApplicationWindow *window = helper.appWindow; + window->show(); + QVERIFY(QTest::qWaitForWindowActive(window)); + + if (mirrored) + window->setLocale(QLocale("ar_EG")); + + QQuickMenu *menu = window->property("menu").value(); + QVERIFY(menu); + menu->open(); + QTRY_VERIFY(menu->isOpened()); + for (int i = 0; i < menu->count(); ++i) { + // Check that the width of menu items is correct before we resize the menu. + const QQuickItem *item = menu->itemAt(i); + QVERIFY2(qFuzzyCompare(item->width(), menu->availableWidth()), + qPrintable(QString::fromLatin1("Expected width of %1 to be %2, but it's %3") + .arg(item->objectName()).arg(menu->availableWidth()).arg(item->width()))); + } + + menu->setWidth(menu->width() + 10); + + // Check that the width of menu items is correct after we resize the menu. + for (int i = 0; i < menu->count(); ++i) { + // Check that the width of menu items is correct after we resize the menu. + const QQuickItem *item = menu->itemAt(i); + QVERIFY2(qFuzzyCompare(item->width(), menu->availableWidth()), + qPrintable(QString::fromLatin1("Expected width of %1 to be %2, but it's %3") + .arg(item->objectName()).arg(menu->availableWidth()).arg(item->width()))); + } +} + +void tst_QQuickMenu::menuItemWidthAfterImplicitWidthChanged_data() +{ + QTest::addColumn("mirrored"); + + QTest::newRow("non-mirrored") << false; + QTest::newRow("mirrored") << true; +} + +void tst_QQuickMenu::menuItemWidthAfterImplicitWidthChanged() +{ + QFETCH(bool, mirrored); + + QQuickApplicationHelper helper(this, QLatin1String("menuItemWidths.qml")); + QQuickApplicationWindow *window = helper.appWindow; + window->show(); + QVERIFY(QTest::qWaitForWindowActive(window)); + + if (mirrored) + window->setLocale(QLocale("ar_EG")); + + QQuickMenu *menu = window->property("menu").value(); + QVERIFY(menu); + menu->open(); + QTRY_VERIFY(menu->isOpened()); + // Check that the width of the menu item is correct before we change its font size. + QQuickMenuItem *menuItem = qobject_cast(menu->itemAt(0)); + QCOMPARE(menuItem->width(), menu->availableWidth()); + + // Add some text to increase the implicitWidth of the MenuItem. + const qreal oldImplicitWidth = menuItem->implicitWidth(); + for (int i = 0; menuItem->implicitWidth() <= oldImplicitWidth; ++i) { + menuItem->setText(menuItem->text() + QLatin1String("---")); + if (i == 100) + QFAIL("Shouldn't need 100 iterations to increase MenuItem's implicitWidth; something is wrong here"); + } + + // Check that the width of the menu item is correct after we change its font size. + QCOMPARE(menuItem->width(), menu->availableWidth()); +} + +void tst_QQuickMenu::menuItemWidthAfterRetranslate() +{ + QQuickApplicationHelper helper(this, QLatin1String("menuItemWidths.qml")); + QQuickApplicationWindow *window = helper.appWindow; + window->show(); + QVERIFY(QTest::qWaitForWindowActive(window)); + + QQuickMenu *menu = window->property("menu").value(); + QVERIFY(menu); + menu->open(); + QTRY_VERIFY(menu->isOpened()); + for (int i = 0; i < menu->count(); ++i) { + // Check that the width of each menu item is correct before we retranslate. + const QQuickItem *item = menu->itemAt(i); + QVERIFY2(qFuzzyCompare(item->width(), menu->availableWidth()), + qPrintable(QString::fromLatin1("Expected width of %1 to be %2, but it's %3") + .arg(item->objectName()).arg(menu->availableWidth()).arg(item->width()))); + } + + // Call retranslate() and cause all bindings to be re-evaluated. + helper.engine.retranslate(); + + for (int i = 0; i < menu->count(); ++i) { + // Check that the width of each menu item is correct after we retranslate. + const QQuickItem *item = menu->itemAt(i); + QVERIFY2(qFuzzyCompare(item->width(), menu->availableWidth()), + qPrintable(QString::fromLatin1("Expected width of %1 to be %2, but it's %3") + .arg(item->objectName()).arg(menu->availableWidth()).arg(item->width()))); + } +} + QTEST_QUICKCONTROLS_MAIN(tst_QQuickMenu) #include "tst_qquickmenu.moc" -- cgit v1.2.3