From 908faa13345797dd37102cf324f3aade3ba9f459 Mon Sep 17 00:00:00 2001 From: beholder Date: Fri, 10 Nov 2017 19:38:07 +0300 Subject: [PATCH 1/4] refactoring: simplify code & fix the issue with menu-es sometimes losing checked state --- interface/resources/qml/desktop/Desktop.qml | 24 +++++++++++-------- .../qml/hifi/tablet/TabletMenuStack.qml | 23 ------------------ 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/interface/resources/qml/desktop/Desktop.qml b/interface/resources/qml/desktop/Desktop.qml index 847111b8a0..e7c68b2a47 100644 --- a/interface/resources/qml/desktop/Desktop.qml +++ b/interface/resources/qml/desktop/Desktop.qml @@ -51,19 +51,23 @@ FocusScope { // The VR version of the primary menu property var rootMenu: Menu { + id: rootMenuId objectName: "rootMenu" - // for some reasons it is not possible to use just '({})' here as it gets empty when passed to TableRoot/DesktopRoot - property var exclusionGroupsByMenuItem : ListModel {} + property var exclusionGroups: ({}); + property Component exclusiveGroupMaker: Component { + ExclusiveGroup { + } + } - function addExclusionGroup(menuItem, exclusionGroup) - { - exclusionGroupsByMenuItem.append( - { - 'menuItem' : menuItem.toString(), - 'exclusionGroup' : exclusionGroup.toString() - } - ); + function addExclusionGroup(qmlAction, exclusionGroup) { + + var exclusionGroupId = exclusionGroup.toString(); + if(!exclusionGroups[exclusionGroupId]) { + exclusionGroups[exclusionGroupId] = exclusiveGroupMaker.createObject(rootMenuId); + } + + qmlAction.exclusiveGroup = exclusionGroups[exclusionGroupId] } } diff --git a/interface/resources/qml/hifi/tablet/TabletMenuStack.qml b/interface/resources/qml/hifi/tablet/TabletMenuStack.qml index ce4fac3bd5..8cd696a41b 100644 --- a/interface/resources/qml/hifi/tablet/TabletMenuStack.qml +++ b/interface/resources/qml/hifi/tablet/TabletMenuStack.qml @@ -66,7 +66,6 @@ Item { function toModel(items, newMenu) { var result = modelMaker.createObject(tabletMenu); - var exclusionGroups = {}; for (var i = 0; i < items.length; ++i) { var item = items[i]; @@ -78,28 +77,6 @@ Item { if (item.text !== "Users Online") { result.append({"name": item.text, "item": item}) } - - for(var j = 0; j < tabletMenu.rootMenu.exclusionGroupsByMenuItem.count; ++j) - { - var entry = tabletMenu.rootMenu.exclusionGroupsByMenuItem.get(j); - if(entry.menuItem == item.toString()) - { - var exclusionGroupId = entry.exclusionGroup; - console.debug('item exclusionGroupId: ', exclusionGroupId) - - if(!exclusionGroups[exclusionGroupId]) - { - exclusionGroups[exclusionGroupId] = exclusiveGroupMaker.createObject(newMenu); - console.debug('new exclusion group created: ', exclusionGroups[exclusionGroupId]) - } - - var exclusionGroup = exclusionGroups[exclusionGroupId]; - - item.exclusiveGroup = exclusionGroup - console.debug('item.exclusiveGroup: ', item.exclusiveGroup) - } - } - break; case MenuItemType.Separator: result.append({"name": "", "item": item}) From 87519cd26f0174ce273fc63ee40076bb02132a22 Mon Sep 17 00:00:00 2001 From: beholder Date: Fri, 10 Nov 2017 19:39:51 +0300 Subject: [PATCH 2/4] use conditional bindings to avoid intermediate binding states --- .../qml/hifi/tablet/TabletMenuItem.qml | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/interface/resources/qml/hifi/tablet/TabletMenuItem.qml b/interface/resources/qml/hifi/tablet/TabletMenuItem.qml index 11d3cab35e..520841b33f 100644 --- a/interface/resources/qml/hifi/tablet/TabletMenuItem.qml +++ b/interface/resources/qml/hifi/tablet/TabletMenuItem.qml @@ -40,37 +40,29 @@ Item { CheckBox { id: checkbox - // FIXME: Should use radio buttons if source.exclusiveGroup. + width: 20 visible: source !== null ? source.visible && source.type === 1 && source.checkable && !source.exclusiveGroup : false - checked: setChecked() - function setChecked() { - if (!source || source.type !== 1 || !source.checkable) { - return false; - } - // FIXME this works for native QML menus but I don't think it will - // for proxied QML menus - return source.checked; + + Binding on checked { + value: source.checked; + when: source && source.type === 1 && source.checkable && !source.exclusiveGroup; } } RadioButton { id: radiobutton - // FIXME: Should use radio buttons if source.exclusiveGroup. + width: 20 visible: source !== null ? source.visible && source.type === 1 && source.checkable && source.exclusiveGroup : false - checked: setChecked() - function setChecked() { - if (!source || source.type !== 1 || !source.checkable) { - return false; - } - // FIXME this works for native QML menus but I don't think it will - // for proxied QML menus - return source.checked; + + Binding on checked { + value: source.checked; + when: source && source.type === 1 && source.checkable && source.exclusiveGroup; } } } From da49ee8bee498b9d1d9280afbc6ccb325d566d40 Mon Sep 17 00:00:00 2001 From: beholder Date: Fri, 10 Nov 2017 19:42:10 +0300 Subject: [PATCH 3/4] 9032 Current display mode can be deselected in the toolbar/tablet menu --- interface/src/Application.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index b21588958e..e35407038b 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -7082,6 +7082,7 @@ DisplayPluginPointer Application::getActiveDisplayPlugin() const { return _displayPlugin; } +static const char* exclusionGroupKey = "exclusionGroup"; static void addDisplayPluginToMenu(DisplayPluginPointer displayPlugin, bool active = false) { auto menu = Menu::getInstance(); @@ -7117,6 +7118,8 @@ static void addDisplayPluginToMenu(DisplayPluginPointer displayPlugin, bool acti action->setCheckable(true); action->setChecked(active); displayPluginGroup->addAction(action); + + action->setProperty(exclusionGroupKey, QVariant::fromValue(displayPluginGroup)); Q_ASSERT(menu->menuItemExists(MenuOption::OutputMenu, name)); } From c4aa93710817fbc7a7179b96e79393b9c2f8dfdd Mon Sep 17 00:00:00 2001 From: Alexander Ivash Date: Sat, 18 Nov 2017 11:16:40 +0300 Subject: [PATCH 4/4] adjust to coding standards --- interface/src/Application.cpp | 4 ++-- interface/src/Menu.cpp | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index e35407038b..335a70a76c 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -7082,7 +7082,7 @@ DisplayPluginPointer Application::getActiveDisplayPlugin() const { return _displayPlugin; } -static const char* exclusionGroupKey = "exclusionGroup"; +static const char* EXCLUSION_GROUP_KEY = "exclusionGroup"; static void addDisplayPluginToMenu(DisplayPluginPointer displayPlugin, bool active = false) { auto menu = Menu::getInstance(); @@ -7119,7 +7119,7 @@ static void addDisplayPluginToMenu(DisplayPluginPointer displayPlugin, bool acti action->setChecked(active); displayPluginGroup->addAction(action); - action->setProperty(exclusionGroupKey, QVariant::fromValue(displayPluginGroup)); + action->setProperty(EXCLUSION_GROUP_KEY, QVariant::fromValue(displayPluginGroup)); Q_ASSERT(menu->menuItemExists(MenuOption::OutputMenu, name)); } diff --git a/interface/src/Menu.cpp b/interface/src/Menu.cpp index 9df22ab08e..9ec5cc6034 100644 --- a/interface/src/Menu.cpp +++ b/interface/src/Menu.cpp @@ -56,7 +56,7 @@ Menu* Menu::getInstance() { return dynamic_cast(qApp->getWindow()->menuBar()); } -const char* exclusionGroupKey = "exclusionGroup"; +const char* EXCLUSION_GROUP_KEY = "exclusionGroup"; Menu::Menu() { auto dialogsManager = DependencyManager::get(); @@ -228,21 +228,21 @@ Menu::Menu() { viewMenu, MenuOption::FirstPerson, Qt::CTRL | Qt::Key_F, true, qApp, SLOT(cameraMenuChanged()))); - firstPersonAction->setProperty(exclusionGroupKey, QVariant::fromValue(cameraModeGroup)); + firstPersonAction->setProperty(EXCLUSION_GROUP_KEY, QVariant::fromValue(cameraModeGroup)); // View > Third Person auto thirdPersonAction = cameraModeGroup->addAction(addCheckableActionToQMenuAndActionHash( viewMenu, MenuOption::ThirdPerson, Qt::CTRL | Qt::Key_G, false, qApp, SLOT(cameraMenuChanged()))); - thirdPersonAction->setProperty(exclusionGroupKey, QVariant::fromValue(cameraModeGroup)); + thirdPersonAction->setProperty(EXCLUSION_GROUP_KEY, QVariant::fromValue(cameraModeGroup)); // View > Mirror auto viewMirrorAction = cameraModeGroup->addAction(addCheckableActionToQMenuAndActionHash( viewMenu, MenuOption::FullscreenMirror, Qt::CTRL | Qt::Key_H, false, qApp, SLOT(cameraMenuChanged()))); - viewMirrorAction->setProperty(exclusionGroupKey, QVariant::fromValue(cameraModeGroup)); + viewMirrorAction->setProperty(EXCLUSION_GROUP_KEY, QVariant::fromValue(cameraModeGroup)); // View > Independent [advanced] auto viewIndependentAction = cameraModeGroup->addAction(addCheckableActionToQMenuAndActionHash(viewMenu, @@ -250,7 +250,7 @@ Menu::Menu() { false, qApp, SLOT(cameraMenuChanged()), UNSPECIFIED_POSITION, "Advanced")); - viewIndependentAction->setProperty(exclusionGroupKey, QVariant::fromValue(cameraModeGroup)); + viewIndependentAction->setProperty(EXCLUSION_GROUP_KEY, QVariant::fromValue(cameraModeGroup)); // View > Entity Camera [advanced] auto viewEntityCameraAction = cameraModeGroup->addAction(addCheckableActionToQMenuAndActionHash(viewMenu, @@ -258,7 +258,7 @@ Menu::Menu() { false, qApp, SLOT(cameraMenuChanged()), UNSPECIFIED_POSITION, "Advanced")); - viewEntityCameraAction->setProperty(exclusionGroupKey, QVariant::fromValue(cameraModeGroup)); + viewEntityCameraAction->setProperty(EXCLUSION_GROUP_KEY, QVariant::fromValue(cameraModeGroup)); viewMenu->addSeparator();