Reallocate static ranges for wxMenuItem

Previously different menus may use conflict ids in range 10000~. Fix TypesettingTools/Aegisub#53 See also Aegisub/Aegisub#131
Ristellise · Jun 4, 2020 · 8af0fc5 · 8af0fc5
1 parent bd7bc1c
commit 8af0fc5
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 26 deletions.
diff --git a/src/base_grid.cpp b/src/base_grid.cpp
@@ -55,9 +55,10 @@
 #include <wx/scrolbar.h>
 #include <wx/sizer.h>
 
+// Check menu.h for id range allocation before editing this enum
 enum {
 	GRID_SCROLLBAR = 1730,
-	MENU_SHOW_COL = 1250 // Needs 15 IDs after this
+	MENU_SHOW_COL = (wxID_HIGHEST + 1) + 2000 // Needs 15 IDs after this
 };
 
 BaseGrid::BaseGrid(wxWindow* parent, agi::Context *context)
@@ -541,7 +542,7 @@ void BaseGrid::OnMouseEvent(wxMouseEvent &event) {
 void BaseGrid::OnContextMenu(wxContextMenuEvent &evt) {
 	wxPoint pos = evt.GetPosition();
 	if (pos == wxDefaultPosition || ScreenToClient(pos).y > lineHeight) {
-		if (!context_menu) context_menu = menu::GetMenu("grid_context", context);
+		if (!context_menu) context_menu = menu::GetMenu("grid_context", (wxID_HIGHEST + 1) + 8000, context);
 		menu::OpenPopupMenu(context_menu.get(), this);
 	}
 	else {

diff --git a/src/frame_main.cpp b/src/frame_main.cpp
@@ -128,7 +128,7 @@ FrameMain::FrameMain()
 	EnableToolBar(*OPT_GET("App/Show Toolbar"));
 
 	StartupLog("Initialize menu bar");
-	menu::GetMenuBar("main", this, context.get());
+	menu::GetMenuBar("main", this, (wxID_HIGHEST + 1) + 10000, context.get());
 
 	StartupLog("Create status bar");
 	CreateStatusBar(2);

diff --git a/src/include/aegisub/menu.h b/src/include/aegisub/menu.h
@@ -28,6 +28,19 @@ class wxMenu;
 class wxMenuBar;
 class wxWindow;
 
+/*
+ID allocation for menu items:
+
+... - wxID_ANY(-1), wxID_LOWEST(4999) - wxID_HIGHEST(5999)    Reserved by wxWidgets, see documentation for wxID_HIGHEST
+
+(wxID_HIGHEST + 1) + 2000 ~ (wxID_HIGHEST + 1) + 2014    Grid column list, see base_grid.cpp
+(wxID_HIGHEST + 1) + 3000 ~ (wxID_HIGHEST + 1) + 3001    Context menu, see timeedit_ctrl.cpp
+(wxID_HIGHEST + 1) + 4000 ~ (wxID_HIGHEST + 1) + 7999    Context menu, see subs_edit_ctrl.cpp
+(wxID_HIGHEST + 1) + 8000 ~ (wxID_HIGHEST + 1) + 8019    Grid context menu, see base_grid.cpp
+(wxID_HIGHEST + 1) + 9000 ~ (wxID_HIGHEST + 1) + 9004    Video context menu, see video_display.cpp
+(wxID_HIGHEST + 1) + 10000 ~ (wxID_HIGHEST + 1) + 10999  Main menu
+*/
+
 namespace menu {
 	DEFINE_EXCEPTION(Error, agi::Exception);
 	DEFINE_EXCEPTION(UnknownMenu, Error);
@@ -39,15 +52,15 @@ namespace menu {
 	/// Throws:
 	///     UnknownMenu if no menu with the given name was found
 	///     BadMenu if there is a menu with the given name, but it is invalid
-	void GetMenuBar(std::string const& name, wxFrame *window, agi::Context *c);
+	void GetMenuBar(std::string const& name, wxFrame *window, int id_base, agi::Context *c);
 
 	/// @brief Get the menu with the specified name as a wxMenu
 	/// @param name Name of the menu
 	///
 	/// Throws:
 	///     UnknownMenu if no menu with the given name was found
 	///     BadMenu if there is a menu with the given name, but it is invalid
-	std::unique_ptr<wxMenu> GetMenu(std::string const& name, agi::Context *c);
+	std::unique_ptr<wxMenu> GetMenu(std::string const& name, int id_base, agi::Context *c);
 
 	/// @brief Open a popup menu at the mouse
 	/// @param menu Menu to open

diff --git a/src/menu.cpp b/src/menu.cpp
@@ -55,10 +55,11 @@
 #endif
 
 namespace {
-/// Window ID of first menu item
-static const int MENU_ID_BASE = 10000;
 
 class MruMenu final : public wxMenu {
+	/// Window ID of first menu item
+	const int id_base;
+
 	std::string type;
 	std::vector<wxMenuItem *> items;
 	std::vector<std::string> *cmds;
@@ -70,16 +71,16 @@ class MruMenu final : public wxMenu {
 
 		for (size_t i = GetMenuItemCount(); i < new_size; ++i) {
 			if (i >= items.size()) {
-				items.push_back(new wxMenuItem(this, MENU_ID_BASE + cmds->size(), "_"));
+				items.push_back(new wxMenuItem(this, id_base + cmds->size(), "_"));
 				cmds->push_back(agi::format("recent/%s/%d", boost::to_lower_copy(type), i));
 			}
 			Append(items[i]);
 		}
 	}
 
 public:
-	MruMenu(std::string type, std::vector<std::string> *cmds)
-	: type(std::move(type))
+	MruMenu(int id_base, std::string type, std::vector<std::string> *cmds)
+	: id_base(id_base), type(std::move(type))
 	, cmds(cmds)
 	{
 	}
@@ -122,6 +123,8 @@ class MruMenu final : public wxMenu {
 /// on submenus in many cases, and registering large numbers of wxEVT_UPDATE_UI
 /// handlers makes everything involves events unusably slow.
 class CommandManager {
+	/// Window ID of first menu item
+	const int id_base;
 	/// Menu items which need to do something on menu open
 	std::vector<std::pair<std::string, wxMenuItem*>> dynamic_items;
 	/// Menu items which need to be updated only when hotkeys change
@@ -169,8 +172,8 @@ class CommandManager {
 	}
 
 public:
-	CommandManager(agi::Context *context)
-	: context(context)
+	CommandManager(int id_base, agi::Context *context)
+	: id_base(id_base), context(context)
 	, hotkeys_changed(hotkey::inst->AddHotkeyChangeListener(&CommandManager::OnHotkeysChanged, this))
 	{
 	}
@@ -198,7 +201,7 @@ class CommandManager {
 
 		menu_text += to_wx("\t" + hotkey::get_hotkey_str_first("Default", co->name()));
 
-		wxMenuItem *item = new wxMenuItem(parent, MENU_ID_BASE + items.size(), menu_text, co->StrHelp(), kind);
+		wxMenuItem *item = new wxMenuItem(parent, id_base + items.size(), menu_text, co->StrHelp(), kind);
 #if defined(__WXMSW__)
 #if wxCHECK_VERSION(3, 1, 3)
 		if (kind == wxITEM_NORMAL) {
@@ -243,7 +246,7 @@ class CommandManager {
 	/// @param name MRU type
 	/// @param parent Menu to append the new MRU menu to
 	void AddRecent(std::string const& name, wxMenu *parent) {
-		mru.push_back(new MruMenu(name, &items));
+		mru.push_back(new MruMenu(id_base, name, &items));
 		parent->AppendSubMenu(mru.back(), _("&Recent"));
 	}
 
@@ -257,7 +260,7 @@ class CommandManager {
 	void OnMenuClick(wxCommandEvent &evt) {
 		// This also gets clicks on unrelated things such as the toolbar, so
 		// the window ID ranges really need to be unique
-		size_t id = static_cast<size_t>(evt.GetId() - MENU_ID_BASE);
+		size_t id = static_cast<size_t>(evt.GetId() - id_base);
 		if (id < items.size() && context)
 			cmd::call(items[id], context);
 
@@ -290,13 +293,13 @@ class CommandManager {
 /// Wrapper for wxMenu to add a command manager
 struct CommandMenu final : public wxMenu {
 	CommandManager cm;
-	CommandMenu(agi::Context *c) : cm(c) { }
+	CommandMenu(int id_base, agi::Context *c) : cm(id_base, c) { }
 };
 
 /// Wrapper for wxMenuBar to add a command manager
 struct CommandMenuBar final : public wxMenuBar {
 	CommandManager cm;
-	CommandMenuBar(agi::Context *c) : cm(c) { }
+	CommandMenuBar(int id_base, agi::Context *c) : cm(id_base, c) { }
 };
 
 /// Read a string from a json object
@@ -512,7 +515,7 @@ class AutomationMenu final : public wxMenu {
 }
 
 namespace menu {
-	void GetMenuBar(std::string const& name, wxFrame *window, agi::Context *c) {
+	void GetMenuBar(std::string const& name, wxFrame *window, int id_base, agi::Context *c) {
 #ifdef __WXMAC__
 		auto bind_events = [&](CommandMenuBar *menu) {
 			window->Bind(wxEVT_ACTIVATE, [=](wxActivateEvent&) { menu->cm.SetContext(c); });
@@ -528,7 +531,7 @@ namespace menu {
 		}
 #endif
 
-		auto menu = agi::make_unique<CommandMenuBar>(c);
+		auto menu = agi::make_unique<CommandMenuBar>(id_base, c);
 		for (auto const& item : get_menu(name)) {
 			std::string submenu, disp;
 			read_entry(item, "submenu", &submenu);
@@ -561,8 +564,8 @@ namespace menu {
 		menu.release();
 	}
 
-	std::unique_ptr<wxMenu> GetMenu(std::string const& name, agi::Context *c) {
-		auto menu = agi::make_unique<CommandMenu>(c);
+	std::unique_ptr<wxMenu> GetMenu(std::string const& name, int id_base, agi::Context *c) {
+		auto menu = agi::make_unique<CommandMenu>(id_base, c);
 		build_menu(name, c, &menu->cm, menu.get());
 		menu->Bind(wxEVT_MENU_OPEN, &CommandManager::OnMenuOpen, &menu->cm);
 		menu->Bind(wxEVT_MENU, &CommandManager::OnMenuClick, &menu->cm);

diff --git a/src/subs_edit_ctrl.cpp b/src/subs_edit_ctrl.cpp
@@ -61,8 +61,9 @@
 #define LANGS_MAX 1000
 
 /// Event ids
+// Check menu.h for id range allocation before editing this enum
 enum {
-	EDIT_MENU_SPLIT_PRESERVE = 1400,
+	EDIT_MENU_SPLIT_PRESERVE = (wxID_HIGHEST + 1) + 4000,
 	EDIT_MENU_SPLIT_ESTIMATE,
 	EDIT_MENU_SPLIT_VIDEO,
 	EDIT_MENU_CUT,
@@ -73,9 +74,9 @@ enum {
 	EDIT_MENU_REMOVE_FROM_DICT,
 	EDIT_MENU_SUGGESTION,
 	EDIT_MENU_SUGGESTIONS,
-	EDIT_MENU_THESAURUS = 1450,
+	EDIT_MENU_THESAURUS = (wxID_HIGHEST + 1) + 5000,
 	EDIT_MENU_THESAURUS_SUGS,
-	EDIT_MENU_DIC_LANGUAGE = 1600,
+	EDIT_MENU_DIC_LANGUAGE = (wxID_HIGHEST + 1) + 6000,
 	EDIT_MENU_DIC_LANGS,
 	EDIT_MENU_THES_LANGUAGE = EDIT_MENU_DIC_LANGUAGE + LANGS_MAX,
 	EDIT_MENU_THES_LANGS

diff --git a/src/timeedit_ctrl.cpp b/src/timeedit_ctrl.cpp
@@ -48,8 +48,9 @@
 
 #define TimeEditWindowStyle
 
+// Check menu.h for id range allocation before editing this enum
 enum {
-	Time_Edit_Copy = 1320,
+	Time_Edit_Copy = (wxID_HIGHEST + 1) + 3000,
 	Time_Edit_Paste
 };
 

diff --git a/src/video_display.cpp b/src/video_display.cpp
@@ -383,7 +383,7 @@ void VideoDisplay::OnMouseWheel(wxMouseEvent& event) {
 }
 
 void VideoDisplay::OnContextMenu(wxContextMenuEvent&) {
-	if (!context_menu) context_menu = menu::GetMenu("video_context", con);
+	if (!context_menu) context_menu = menu::GetMenu("video_context", (wxID_HIGHEST + 1) + 9000, con);
 	SetCursor(wxNullCursor);
 	menu::OpenPopupMenu(context_menu.get(), this);
 }