Introduce circular dependency detection and unit tests.

Signed-off-by: Danilo Aimini <[email protected]>

Code/Framework/AzToolsFramework/AzToolsFramework/ActionManager/Menu/MenuManager.cpp

@@ -279,6 +279,13 @@ namespace AzToolsFramework
                 subMenuIdentifier.c_str(), menuIdentifier.c_str()));
         }
 
+        if (WouldGenerateCircularDependency(menuIdentifier, subMenuIdentifier))
+        {
+            return AZ::Failure(AZStd::string::format(
+                "Menu Manager - Could not add sub-menu \"%s\" to menu \"%s\" as this would generate a circular dependency.",
+                subMenuIdentifier.c_str(), menuIdentifier.c_str()));
+        }
+
         menuIterator->second.AddSubMenu(sortIndex, subMenuIdentifier);
         m_subMenusToMenusMap[subMenuIdentifier].insert(menuIdentifier);
         m_menusToRefresh.insert(menuIdentifier);
@@ -614,6 +621,26 @@ namespace AzToolsFramework
                 menuIterator->second.RefreshMenu();
             }
         }
+
+        if (!m_menusToRefresh.empty())
+        {
+            RefreshMenus();
+        }
+    }
+
+    bool MenuManager::WouldGenerateCircularDependency(const AZStd::string& menuIdentifier, const AZStd::string& subMenuIdentifier)
+    {
+        // Iterate through all menus that have menuIdentifier as their submenu.
+        for (auto identifier : m_subMenusToMenusMap[menuIdentifier])
+        {
+            // Return true if the menu is the submenu we're trying to add, or keep checking with the ancestors.
+            if (identifier == subMenuIdentifier || WouldGenerateCircularDependency(identifier, subMenuIdentifier))
+            {
+                return true;
+            }
+        }
+
+        return false;
     }
 
     void MenuManager::RefreshMenuBars()

Code/Framework/AzToolsFramework/AzToolsFramework/ActionManager/Menu/MenuManager.h

@@ -84,6 +84,9 @@ namespace AzToolsFramework
         // ActionManagerNotificationBus overrides ...
         void OnActionStateChanged(AZStd::string actionIdentifier) override;
 
+        // Identifies whether adding a submenu to a menu would generate any circular dependencies.
+        bool WouldGenerateCircularDependency(const AZStd::string& menuIdentifier, const AZStd::string& subMenuIdentifier);
+
         AZStd::unordered_map<AZStd::string, EditorMenu> m_menus;
         AZStd::unordered_map<AZStd::string, EditorMenuBar> m_menuBars;
 

Code/Framework/AzToolsFramework/Tests/ActionManager/MenuManagerTests.cpp

@@ -262,6 +262,11 @@ namespace UnitTest
         // Add the sub-menu to the menu.
         m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testMenu", "o3de.menu.testSubMenu", 42);
 
+        // Add an action to the sub-menu, else it will be empty and not be displayed.
+        m_actionManagerInterface->RegisterActionContext("", "o3de.context.test", {}, m_widget);
+        m_actionManagerInterface->RegisterAction("o3de.context.test", "o3de.action.test", {}, []{});
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu", "o3de.action.test", 42);
+
         // Manually trigger Menu refresh - Editor will call this once per tick.
         m_menuManagerInternalInterface->RefreshMenus();
 
@@ -293,6 +298,12 @@ namespace UnitTest
         m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu1", {});
         m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu2", {});
 
+        // Add an action to the sub-menus, else they will be empty and not be displayed.
+        m_actionManagerInterface->RegisterActionContext("", "o3de.context.test", {}, m_widget);
+        m_actionManagerInterface->RegisterAction("o3de.context.test", "o3de.action.test", {}, []{});
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu1", "o3de.action.test", 42);
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu2", "o3de.action.test", 42);
+
         // Add the sub-menus to the menu.
         AZStd::vector<AZStd::pair<AZStd::string, int>> testMenus;
         testMenus.emplace_back("o3de.menu.testSubMenu1", 100);
@@ -369,6 +380,13 @@ namespace UnitTest
         m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu2", {});
         m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu3", {});
 
+        // Add an action to the sub-menus, else they will be empty and not be displayed.
+        m_actionManagerInterface->RegisterActionContext("", "o3de.context.test", {}, m_widget);
+        m_actionManagerInterface->RegisterAction("o3de.context.test", "o3de.action.test", {}, []{});
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu1", "o3de.action.test", 42);
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu2", "o3de.action.test", 42);
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu3", "o3de.action.test", 42);
+
         // Add the sub-menus to the menu.
         AZStd::vector<AZStd::pair<AZStd::string, int>> testMenuAdds;
         testMenuAdds.emplace_back("o3de.menu.testSubMenu1", 100);
@@ -793,4 +811,98 @@ namespace UnitTest
         EXPECT_EQ(menu->actions().size(), 1);
     }
 
+    TEST_F(ActionManagerFixture, VerifySubMenuIsHiddenWhenEmpty)
+    {
+        // Register menus
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testMenu", {});
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu", {});
+        m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testMenu", "o3de.menu.testSubMenu", 42);
+
+        // Register a new action and add it to the sub-menu.
+        m_actionManagerInterface->RegisterActionContext("", "o3de.context.test", {}, m_widget);
+        m_actionManagerInterface->RegisterAction("o3de.context.test", "o3de.action.test", {}, []{});
+        m_menuManagerInterface->AddActionToMenu("o3de.menu.testSubMenu", "o3de.action.test", 42);
+
+        // Add enabled state callback.
+        bool enabledState = true;
+        m_actionManagerInterface->InstallEnabledStateCallback(
+            "o3de.action.test",
+            [&]()
+            {
+                return enabledState;
+            }
+        );
+
+        // Manually trigger Menu refresh - Editor will call this once per tick.
+        m_menuManagerInternalInterface->RefreshMenus();
+
+        // Verify the sub-menu is now in the menu.
+        {
+            QMenu* menu = m_menuManagerInternalInterface->GetMenu("o3de.menu.testMenu");
+            QMenu* submenu = m_menuManagerInternalInterface->GetMenu("o3de.menu.testSubMenu");
+            const auto& actions = menu->actions();
+
+            EXPECT_EQ(actions.size(), 1);
+            EXPECT_EQ(actions[0]->menu(), submenu);
+        }
+
+        // Set the action as disabled.
+        enabledState = false;
+        m_actionManagerInterface->UpdateAction("o3de.action.test");
+
+        // Manually trigger Menu refresh - Editor will call this once per tick.
+        m_menuManagerInternalInterface->RefreshMenus();
+
+        // Verify the sub-menu is no longer part of the menu since it is empty.
+        {
+            QMenu* menu = m_menuManagerInternalInterface->GetMenu("o3de.menu.testMenu");
+            EXPECT_EQ(menu->actions().size(), 0);
+        }
+
+        // Set the action as enabled.
+        enabledState = true;
+        m_actionManagerInterface->UpdateAction("o3de.action.test");
+
+        // Manually trigger Menu refresh - Editor will call this once per tick.
+        m_menuManagerInternalInterface->RefreshMenus();
+
+        // Verify the sub-menu is in the menu again.
+        {
+            QMenu* menu = m_menuManagerInternalInterface->GetMenu("o3de.menu.testMenu");
+            QMenu* submenu = m_menuManagerInternalInterface->GetMenu("o3de.menu.testSubMenu");
+            const auto& actions = menu->actions();
+
+            EXPECT_EQ(actions.size(), 1);
+            EXPECT_EQ(actions[0]->menu(), submenu);
+        }
+    }
+
+    TEST_F(ActionManagerFixture, VerifySimpleAddSubMenuCircularDependency)
+    {
+        // Register menus
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testMenu", {});
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu", {});
+        m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testMenu", "o3de.menu.testSubMenu", 42);
+
+        // Verify I can't add "o3de.menu.testMenu" as a sub-menu for "o3de.menu.testSubMenu"
+        // as it would cause a circular dependency.
+        auto outcome = m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testSubMenu", "o3de.menu.testMenu", 42);
+        EXPECT_FALSE(outcome.IsSuccess());
+    }
+
+    TEST_F(ActionManagerFixture, VerifyNestedAddSubMenuCircularDependency)
+    {
+        // Register menus
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testMenu", {});
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testSubMenu", {});
+        m_menuManagerInterface->RegisterMenu("o3de.menu.testSubSubMenu", {});
+        m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testMenu", "o3de.menu.testSubMenu", 42);
+        m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testSubMenu", "o3de.menu.testSubSubMenu", 42);
+
+        // Verify I can't add "o3de.menu.testMenu" as a sub-menu for "o3de.menu.testSubSubMenu"
+        // as it would cause a circular dependency.
+        auto outcome = m_menuManagerInterface->AddSubMenuToMenu("o3de.menu.testSubSubMenu", "o3de.menu.testMenu", 42);
+        EXPECT_FALSE(outcome.IsSuccess());
+    }
+
 } // namespace UnitTest