EventManager maintains a map of menu ID -> command strings

Use a "reverse map" alongside the forward map of command strings to wxMenuItem pointers, avoiding the need for a slow search for a specific menu ID in onMenuItemClicked().
codereader · Jun 22, 2022 · dfbe8e4 · dfbe8e4
1 parent bfebe76
commit dfbe8e4
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 39 deletions.
diff --git a/radiant/eventmanager/EventManager.cpp b/radiant/eventmanager/EventManager.cpp
@@ -6,6 +6,7 @@
 #include <iostream>
 #include <typeinfo>
 
+#include <wx/debug.h>
 #include <wx/wxprec.h>
 #include <wx/toolbar.h>
 #include <wx/menu.h>
@@ -260,48 +261,47 @@ void EventManager::setToggled(const std::string& name, const bool toggled)
 
 void EventManager::registerMenuItem(const std::string& eventName, wxMenuItem* item)
 {
-	_menuItems.emplace(eventName, item);
+    // Add to both forward and reverse maps
+    _menuItems.emplace(eventName, item);
+    _commandsByMenuID.emplace(item->GetId(), eventName);
 
-	// Set the accelerator of this menu item
-	auto& accelerator = findAccelerator(eventName);
+    // Set the accelerator of this menu item
+    auto& accelerator = findAccelerator(eventName);
 
-	Event::setMenuItemAccelerator(item, accelerator);
+    Event::setMenuItemAccelerator(item, accelerator);
 
-	// Check if we have an event object corresponding to this event name
-	auto evt = findEvent(eventName);
+    // Check if we have an event object corresponding to this event name
+    auto evt = findEvent(eventName);
 
-	if (!evt->empty())
-	{
-		evt->connectMenuItem(item);
-	}
-	else
-	{
-		item->GetMenu()->Bind(wxEVT_MENU, &EventManager::onMenuItemClicked, this, item->GetId());
-	}
+    if (!evt->empty())
+        evt->connectMenuItem(item);
+    else
+        item->GetMenu()->Bind(wxEVT_MENU, &EventManager::onMenuItemClicked, this, item->GetId());
 }
 
 void EventManager::unregisterMenuItem(const std::string& eventName, wxMenuItem* item)
 {
-	for (auto it = _menuItems.lower_bound(eventName);
-		 it != _menuItems.end() && it != _menuItems.upper_bound(eventName); ++it)
-	{
-		if (it->second != item) continue;
+    for (auto it = _menuItems.lower_bound(eventName);
+         it != _menuItems.end() && it != _menuItems.upper_bound(eventName); ++it)
+    {
+        if (it->second != item)
+            continue;
 
-		// Check if we have an event object corresponding to this event name
-		auto evt = findEvent(eventName);
+        // Check if we have an event object corresponding to this event name
+        auto evt = findEvent(eventName);
 
-		if (!evt->empty())
-		{
-			evt->disconnectMenuItem(item);
-		}
-		else
-		{
-			item->GetMenu()->Unbind(wxEVT_MENU, &EventManager::onMenuItemClicked, this, item->GetId());
-		}
+        if (!evt->empty())
+            evt->disconnectMenuItem(item);
+        else
+            item->GetMenu()->Unbind(wxEVT_MENU, &EventManager::onMenuItemClicked, this,
+                                    item->GetId());
 
-		_menuItems.erase(it);
-		break;
-	}
+        // Erase from both forward and reverse maps
+        _menuItems.erase(it);
+        _commandsByMenuID.erase(item->GetId());
+
+        break;
+    }
 }
 
 void EventManager::registerToolItem(const std::string& eventName, const wxToolBarToolBase* item)
@@ -370,14 +370,10 @@ void EventManager::onToolItemClicked(wxCommandEvent& ev)
 
 void EventManager::onMenuItemClicked(wxCommandEvent& ev)
 {
-	for (const auto& pair : _menuItems)
-	{
-		if (pair.second->GetId() == ev.GetId())
-		{
-			GlobalCommandSystem().execute(pair.first);
-			break;
-		}
-	}
+    if (const auto i = _commandsByMenuID.find(ev.GetId()); i != _commandsByMenuID.end())
+        GlobalCommandSystem().execute(i->second);
+    else
+        wxFAIL_MSG("onMenuItemClicked(): no command for menu ID");
 }
 
 Accelerator& EventManager::connectAccelerator(int keyCode, unsigned int modifierFlags, const std::string& command)

diff --git a/radiant/eventmanager/EventManager.h b/radiant/eventmanager/EventManager.h
@@ -26,6 +26,9 @@ class EventManager :
 	std::multimap<std::string, wxMenuItem*> _menuItems;
 	std::multimap<std::string, const wxToolBarToolBase*> _toolItems;
 
+	// Reverse mapping of menu IDs back to command strings
+	std::map<int, std::string> _commandsByMenuID;
+
 	// The command-to-accelerator map containing all registered shortcuts
 	typedef std::map<std::string, Accelerator::Ptr> AcceleratorMap;
 	AcceleratorMap _accelerators;