From 7327a56ad2b4b1ca7afe67eaa33682777aada8c6 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Mon, 29 Nov 2021 22:44:05 +0100 Subject: [PATCH 1/2] Remove deprecated usage of setUserData This removes the usage of QObjectUserData, setUserData and userData. Fixes #1305 --- libraries/ui/src/OffscreenUi.cpp | 52 ++++---- libraries/ui/src/OffscreenUi.h | 5 +- libraries/ui/src/VrMenu.cpp | 220 ++++++++++++++----------------- libraries/ui/src/VrMenu.h | 37 +++++- 4 files changed, 161 insertions(+), 153 deletions(-) diff --git a/libraries/ui/src/OffscreenUi.cpp b/libraries/ui/src/OffscreenUi.cpp index d5fa6609d4..07376cf761 100644 --- a/libraries/ui/src/OffscreenUi.cpp +++ b/libraries/ui/src/OffscreenUi.cpp @@ -38,14 +38,14 @@ *

This API currently has no effect and is not used.

* * @namespace OffscreenFlags - * + * * @hifi-interface * @hifi-client-entity * @hifi-avatar * - * @property {boolean} navigationFocused - true if UI has joystick navigation focus, false if it + * @property {boolean} navigationFocused - true if UI has joystick navigation focus, false if it * doesn't. - * @property {boolean} navigationFocusDisabled - true if UI joystick navigation focus is disabled, + * @property {boolean} navigationFocusDisabled - true if UI joystick navigation focus is disabled, * false if it isn't. */ @@ -75,7 +75,7 @@ public: emit navigationFocusDisabledChanged(); } } - + signals: /*@jsdoc @@ -99,9 +99,9 @@ private: static OffscreenFlags* offscreenFlags { nullptr }; -// This hack allows the QML UI to work with keys that are also bound as -// shortcuts at the application level. However, it seems as though the -// bound actions are still getting triggered. At least for backspace. +// This hack allows the QML UI to work with keys that are also bound as +// shortcuts at the application level. However, it seems as though the +// bound actions are still getting triggered. At least for backspace. // Not sure why. // // However, the problem may go away once we switch to the new menu system, @@ -168,7 +168,7 @@ void OffscreenUi::onRootContextCreated(QQmlContext* qmlContext) { qmlContext->setContextProperty("fileDialogHelper", new FileDialogHelper()); #ifdef DEBUG qmlContext->setContextProperty("DebugQML", QVariant(true)); -#else +#else qmlContext->setContextProperty("DebugQML", QVariant(false)); #endif @@ -284,7 +284,7 @@ QQuickItem* OffscreenUi::createMessageBox(Icon icon, const QString& title, const map.insert("buttons", buttons.operator int()); map.insert("defaultButton", defaultButton); QVariant result; - bool invokeResult; + bool invokeResult = false; auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); if (tablet->getToolbarMode() && _desktop) { @@ -309,7 +309,7 @@ int OffscreenUi::waitForMessageBoxResult(QQuickItem* messageBox) { if (!messageBox) { return QMessageBox::NoButton; } - + return MessageBoxListener(messageBox).waitForButtonResult(); } @@ -424,8 +424,8 @@ QString OffscreenUi::getText(const Icon icon, const QString& title, const QStrin QString OffscreenUi::getItem(const Icon icon, const QString& title, const QString& label, const QStringList& items, int current, bool editable, bool* ok) { - if (ok) { - *ok = false; + if (ok) { + *ok = false; } auto offscreenUi = DependencyManager::get(); @@ -580,7 +580,7 @@ QQuickItem* OffscreenUi::createInputDialog(const Icon icon, const QString& title auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); - bool invokeResult; + bool invokeResult = false; if (tablet->getToolbarMode() && _desktop) { invokeResult = QMetaObject::invokeMethod(_desktop, "inputDialog", Q_RETURN_ARG(QVariant, result), @@ -608,7 +608,7 @@ QQuickItem* OffscreenUi::createCustomInputDialog(const Icon icon, const QString& auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); - bool invokeResult; + bool invokeResult = false; if (tablet->getToolbarMode() && _desktop) { invokeResult = QMetaObject::invokeMethod(_desktop, "inputDialog", Q_RETURN_ARG(QVariant, result), @@ -619,7 +619,7 @@ QQuickItem* OffscreenUi::createCustomInputDialog(const Icon icon, const QString& Q_ARG(QVariant, QVariant::fromValue(map))); emit tabletScriptingInterface->tabletNotification(); } - + if (!invokeResult) { qWarning() << "Failed to create custom message box"; return nullptr; @@ -648,13 +648,13 @@ void OffscreenUi::setNavigationFocused(bool focused) { // FIXME HACK.... // This hack is an attempt to work around the 'offscreen UI can't gain keyboard focus' bug // https://app.asana.com/0/27650181942747/83176475832393 -// The problem seems related to https://bugreports.qt.io/browse/QTBUG-50309 +// The problem seems related to https://bugreports.qt.io/browse/QTBUG-50309 // // The workaround seems to be to give some other window (same process or another process doesn't seem to matter) -// focus and then put focus back on the interface main window. +// focus and then put focus back on the interface main window. // -// If I could reliably reproduce this bug I could eventually track down what state change is occuring -// during the process of the main window losing and then gaining focus, but failing that, here's a +// If I could reliably reproduce this bug I could eventually track down what state change is occuring +// during the process of the main window losing and then gaining focus, but failing that, here's a // brute force way of triggering that state change at application start in a way that should be nearly // imperceptible to the user. class KeyboardFocusHack : public QObject { @@ -754,7 +754,7 @@ private slots: QString OffscreenUi::fileDialog(const QVariantMap& properties) { QVariant buildDialogResult; - bool invokeResult; + bool invokeResult = false; auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); if (tablet->getToolbarMode() && _desktop) { @@ -783,7 +783,7 @@ QString OffscreenUi::fileDialog(const QVariantMap& properties) { ModalDialogListener* OffscreenUi::fileDialogAsync(const QVariantMap& properties) { QVariant buildDialogResult; - bool invokeResult; + bool invokeResult = false; auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); if (tablet->getToolbarMode() && _desktop) { @@ -1003,7 +1003,7 @@ class AssetDialogListener : public ModalDialogListener { QString OffscreenUi::assetDialog(const QVariantMap& properties) { // ATP equivalent of fileDialog(). QVariant buildDialogResult; - bool invokeResult; + bool invokeResult = false; auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); if (tablet->getToolbarMode() && _desktop) { @@ -1033,7 +1033,7 @@ QString OffscreenUi::assetDialog(const QVariantMap& properties) { ModalDialogListener *OffscreenUi::assetDialogAsync(const QVariantMap& properties) { // ATP equivalent of fileDialog(). QVariant buildDialogResult; - bool invokeResult; + bool invokeResult = false; auto tabletScriptingInterface = DependencyManager::get(); TabletProxy* tablet = dynamic_cast(tabletScriptingInterface->getTablet("com.highfidelity.interface.tablet.system")); if (tablet->getToolbarMode() && _desktop) { @@ -1165,7 +1165,7 @@ bool OffscreenUi::eventFilter(QObject* originalDestination, QEvent* event) { } // QML input elements absorb key press, but apparently not key release. - // therefore we want to ensure that key release events for key presses that were + // therefore we want to ensure that key release events for key presses that were // accepted by the QML layer are suppressed if (type == QEvent::KeyRelease && pressed) { pressed = false; @@ -1175,10 +1175,6 @@ bool OffscreenUi::eventFilter(QObject* originalDestination, QEvent* event) { return result; } -unsigned int OffscreenUi::getMenuUserDataId() const { - return _vrMenu->_userDataId; -} - ModalDialogListener::ModalDialogListener(QQuickItem *dialog) : _dialog(dialog) { if (!dialog) { _finished = true; diff --git a/libraries/ui/src/OffscreenUi.h b/libraries/ui/src/OffscreenUi.h index 7e20a0b09d..994a579f44 100644 --- a/libraries/ui/src/OffscreenUi.h +++ b/libraries/ui/src/OffscreenUi.h @@ -74,7 +74,7 @@ public: // Setting pinned to true will hide all overlay elements on the desktop that don't have a pinned flag void setPinned(bool pinned = true); - + void togglePinned(); void setConstrainToolbarToCenterX(bool constrained); @@ -237,7 +237,6 @@ public: static ModalDialogListener* getTextAsync(const Icon icon, const QString & title, const QString & label, const QString & text = QString()); static ModalDialogListener* getItemAsync(const Icon icon, const QString & title, const QString & label, const QStringList & items, int current = 0, bool editable = true); - unsigned int getMenuUserDataId() const; QList &getModalDialogListeners(); signals: @@ -270,7 +269,7 @@ private: QList _modalDialogListeners; std::unordered_map _pressedKeys; VrMenu* _vrMenu { nullptr }; - QQueue> _queuedMenuInitializers; + QQueue> _queuedMenuInitializers; }; #endif diff --git a/libraries/ui/src/VrMenu.cpp b/libraries/ui/src/VrMenu.cpp index 8003337eac..0fce96ab71 100644 --- a/libraries/ui/src/VrMenu.cpp +++ b/libraries/ui/src/VrMenu.cpp @@ -17,139 +17,117 @@ #include "OffscreenUi.h" #include "ui/Logging.h" -static unsigned int USER_DATA_ID = 0; -// Binds together a Qt Action or Menu with the QML Menu or MenuItem -// -// TODO On reflection, it may be pointless to use the UUID. Perhaps -// simply creating the bidirectional link pointing to both the widget -// and qml object and inject the pointer into both objects -class MenuUserData : public QObjectUserData { -public: - MenuUserData(QAction* action, QObject* qmlObject, QObject* qmlParent) { - if (!USER_DATA_ID) { - USER_DATA_ID = DependencyManager::get()->getMenuUserDataId(); - } - _action = action; - _qml = qmlObject; - _qmlParent = qmlParent; - action->setUserData(USER_DATA_ID, this); - qmlObject->setUserData(USER_DATA_ID, this); - qmlObject->setObjectName(uuid.toString()); - // Make sure we can find it again in the future +MenuUserData::MenuUserData(QAction* action, QObject* qmlObject, QObject* qmlParent) { + _action = action; + _qml = qmlObject; + _qmlParent = qmlParent; + + action->setProperty(USER_DATA, QVariant::fromValue(this)); + qmlObject->setProperty(USER_DATA, QVariant::fromValue(this)); + qmlObject->setObjectName(uuid.toString()); + // Make sure we can find it again in the future + updateQmlItemFromAction(); + _changedConnection = QObject::connect(action, &QAction::changed, [=] { updateQmlItemFromAction(); - _changedConnection = QObject::connect(action, &QAction::changed, [=] { - updateQmlItemFromAction(); - }); - _shutdownConnection = QObject::connect(qApp, &QCoreApplication::aboutToQuit, [=] { - QObject::disconnect(_changedConnection); - }); - - class ExclusionGroupSetter : public QObject { - public: - ExclusionGroupSetter(QObject* from, QObject* to, QObject* qmlParent) : QObject(from), _from(from), _to(to), _qmlParent(qmlParent) { - _from->installEventFilter(this); - } - - ~ExclusionGroupSetter() { - _from->removeEventFilter(this); - } - protected: - virtual bool eventFilter(QObject* o, QEvent* e) override { - if (e->type() == QEvent::DynamicPropertyChange) { - QDynamicPropertyChangeEvent* dpc = static_cast(e); - if (dpc->propertyName() == "exclusionGroup") - { - // unfortunately Qt doesn't support passing dynamic properties between C++ / QML, so we have to use this ugly helper function - QMetaObject::invokeMethod(_qmlParent, - "addExclusionGroup", - Qt::DirectConnection, - Q_ARG(QVariant, QVariant::fromValue(_to)), - Q_ARG(QVariant, _from->property(dpc->propertyName()))); - } - } - - return QObject::eventFilter(o, e); - } - - private: - QObject* _from; - QObject* _to; - QObject* _qmlParent; - }; - - new ExclusionGroupSetter(action, qmlObject, qmlParent); - } - - ~MenuUserData() { + }); + _shutdownConnection = QObject::connect(qApp, &QCoreApplication::aboutToQuit, [=] { QObject::disconnect(_changedConnection); - QObject::disconnect(_shutdownConnection); - _action->setUserData(USER_DATA_ID, nullptr); - _qml->setUserData(USER_DATA_ID, nullptr); - } + }); - void updateQmlItemFromAction() { - _qml->setProperty("checkable", _action->isCheckable()); - _qml->setProperty("enabled", _action->isEnabled()); - QString text = _action->text(); - _qml->setProperty("text", text); - _qml->setProperty("shortcut", _action->shortcut().toString()); - _qml->setProperty("checked", _action->isChecked()); - _qml->setProperty("visible", _action->isVisible()); - } - - void clear() { - _qml->setProperty("checkable", 0); - _qml->setProperty("enabled", 0); - _qml->setProperty("text", 0); - _qml->setProperty("shortcut", 0); - _qml->setProperty("checked", 0); - _qml->setProperty("visible", 0); - - _action->setUserData(USER_DATA_ID, nullptr); - _qml->setUserData(USER_DATA_ID, nullptr); - } - - - const QUuid uuid{ QUuid::createUuid() }; - - static bool hasData(QAction* object) { - if (!object) { - qWarning() << "Attempted to fetch MenuUserData for null object"; - return false; + class ExclusionGroupSetter : public QObject { + public: + ExclusionGroupSetter(QObject* from, QObject* to, QObject* qmlParent) : QObject(from), _from(from), _to(to), _qmlParent(qmlParent) { + _from->installEventFilter(this); } - return (nullptr != static_cast(object->userData(USER_DATA_ID))); - } - static MenuUserData* forObject(QAction* object) { - if (!object) { - qWarning() << "Attempted to fetch MenuUserData for null object"; - return nullptr; + ~ExclusionGroupSetter() { + _from->removeEventFilter(this); } - auto result = static_cast(object->userData(USER_DATA_ID)); - if (!result) { - qWarning() << "Unable to find MenuUserData for object " << object; - if (auto action = dynamic_cast(object)) { - qWarning() << action->text(); - } else if (auto menu = dynamic_cast(object)) { - qWarning() << menu->title(); + protected: + virtual bool eventFilter(QObject* o, QEvent* e) override { + if (e->type() == QEvent::DynamicPropertyChange) { + QDynamicPropertyChangeEvent* dpc = static_cast(e); + if (dpc->propertyName() == "exclusionGroup") + { + // unfortunately Qt doesn't support passing dynamic properties between C++ / QML, so we have to use this ugly helper function + QMetaObject::invokeMethod(_qmlParent, + "addExclusionGroup", + Qt::DirectConnection, + Q_ARG(QVariant, QVariant::fromValue(_to)), + Q_ARG(QVariant, _from->property(dpc->propertyName()))); + } } - return nullptr; + + return QObject::eventFilter(o, e); } - return result; + + private: + QObject* _from; + QObject* _to; + QObject* _qmlParent; + }; + + new ExclusionGroupSetter(action, qmlObject, qmlParent); +} + +MenuUserData::~MenuUserData() { + QObject::disconnect(_changedConnection); + QObject::disconnect(_shutdownConnection); + _action->setProperty(USER_DATA, QVariant()); + _qml->setProperty(USER_DATA, QVariant()); +} + +void MenuUserData::updateQmlItemFromAction() { + _qml->setProperty("checkable", _action->isCheckable()); + _qml->setProperty("enabled", _action->isEnabled()); + QString text = _action->text(); + _qml->setProperty("text", text); + _qml->setProperty("shortcut", _action->shortcut().toString()); + _qml->setProperty("checked", _action->isChecked()); + _qml->setProperty("visible", _action->isVisible()); +} + +void MenuUserData::clear() { + _qml->setProperty("checkable", 0); + _qml->setProperty("enabled", 0); + _qml->setProperty("text", 0); + _qml->setProperty("shortcut", 0); + _qml->setProperty("checked", 0); + _qml->setProperty("visible", 0); + + _action->setProperty(USER_DATA, QVariant()); + _qml->setProperty(USER_DATA, QVariant()); +} + + + +bool MenuUserData::hasData(QAction* object) { + if (!object) { + qWarning() << "Attempted to fetch MenuUserData for null object"; + return false; } + return (nullptr != object->property(USER_DATA).value()); +} -private: - Q_DISABLE_COPY(MenuUserData); - - QMetaObject::Connection _shutdownConnection; - QMetaObject::Connection _changedConnection; - QAction* _action { nullptr }; - QObject* _qml { nullptr }; - QObject* _qmlParent{ nullptr }; -}; - +MenuUserData* MenuUserData::forObject(QAction* object) { + if (!object) { + qWarning() << "Attempted to fetch MenuUserData for null object"; + return nullptr; + } + auto result = object->property(USER_DATA).value(); + if (!result) { + qWarning() << "Unable to find MenuUserData for object " << object; + if (auto action = dynamic_cast(object)) { + qWarning() << action->text(); + } else if (auto menu = dynamic_cast(object)) { + qWarning() << menu->title(); + } + return nullptr; + } + return result; +} VrMenu::VrMenu(OffscreenUi* parent) : QObject(parent) { _rootMenu = parent->getRootItem()->findChild("rootMenu"); diff --git a/libraries/ui/src/VrMenu.h b/libraries/ui/src/VrMenu.h index c2f9fd830b..4e7031f650 100644 --- a/libraries/ui/src/VrMenu.h +++ b/libraries/ui/src/VrMenu.h @@ -18,8 +18,44 @@ #include #include #include +#include #include "OffscreenUi.h" + + +// Binds together a Qt Action or Menu with the QML Menu or MenuItem +// +// TODO On reflection, it may be pointless to use the UUID. Perhaps +// simply creating the bidirectional link pointing to both the widget +// and qml object and inject the pointer into both objects +class MenuUserData : public QObject { + Q_OBJECT +public: + MenuUserData(QAction* action, QObject* qmlObject, QObject* qmlParent); + ~MenuUserData(); + void updateQmlItemFromAction(); + void clear(); + + const QUuid uuid{ QUuid::createUuid() }; + + static bool hasData(QAction* object); + + static MenuUserData* forObject(QAction* object); + +private: + Q_DISABLE_COPY(MenuUserData); + + static constexpr const char *USER_DATA{"user_data"}; + + QMetaObject::Connection _shutdownConnection; + QMetaObject::Connection _changedConnection; + QAction* _action { nullptr }; + QObject* _qml { nullptr }; + QObject* _qmlParent{ nullptr }; +}; + + + // FIXME break up the rendering code (VrMenu) and the code for mirroring a Widget based menu in QML class VrMenu : public QObject { Q_OBJECT @@ -37,7 +73,6 @@ protected: friend class MenuUserData; friend class OffscreenUi; - const unsigned int _userDataId { QObject::registerUserData() }; }; #endif // hifi_VrMenu_h From 27ddd1d61d0583d6f3e3a9aaa27d7e6d8a1eb1e5 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Thu, 20 Jan 2022 23:07:13 +0100 Subject: [PATCH 2/2] Review fix --- libraries/ui/src/VrMenu.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libraries/ui/src/VrMenu.cpp b/libraries/ui/src/VrMenu.cpp index 0fce96ab71..6ba01165d5 100644 --- a/libraries/ui/src/VrMenu.cpp +++ b/libraries/ui/src/VrMenu.cpp @@ -49,8 +49,7 @@ MenuUserData::MenuUserData(QAction* action, QObject* qmlObject, QObject* qmlPare virtual bool eventFilter(QObject* o, QEvent* e) override { if (e->type() == QEvent::DynamicPropertyChange) { QDynamicPropertyChangeEvent* dpc = static_cast(e); - if (dpc->propertyName() == "exclusionGroup") - { + if (dpc->propertyName() == "exclusionGroup") { // unfortunately Qt doesn't support passing dynamic properties between C++ / QML, so we have to use this ugly helper function QMetaObject::invokeMethod(_qmlParent, "addExclusionGroup",