Merge pull request #6728 from wesnoth/hotkey-command-handling-cleanup

Hotkeys: refactored how hotkey_commands are stored and managed (WML hotkeys only)

src/game_events/menu_item.cpp

@@ -70,6 +70,7 @@ wml_menu_item::wml_menu_item(const std::string& id, const config& cfg)
 	: item_id_(id)
 	, event_name_(make_item_name(id))
 	, hotkey_id_(make_item_hotkey(id))
+	, hotkey_record_()
 	, image_(cfg["image"].str())
 	, description_(cfg["description"].t_str())
 	, needs_select_(cfg["needs_select"].to_bool(false))
@@ -92,6 +93,7 @@ wml_menu_item::wml_menu_item(const std::string& id, const vconfig& definition)
 	: item_id_(id)
 	, event_name_(make_item_name(id))
 	, hotkey_id_(make_item_hotkey(id))
+	, hotkey_record_()
 	, image_()
 	, description_()
 	, needs_select_(false)
@@ -106,7 +108,7 @@ wml_menu_item::wml_menu_item(const std::string& id, const vconfig& definition)
 {
 	// On the off-chance that update() doesn't do it, add the hotkey here.
 	// (Update can always modify it.)
-	hotkey::add_wml_hotkey(hotkey_id_, description_, default_hotkey_);
+	hotkey_record_.emplace(hotkey_id_, description_, default_hotkey_);
 
 	// Apply WML.
 	update(definition);
@@ -189,7 +191,7 @@ void wml_menu_item::fire_event(const map_location& event_hex, const game_data& d
 	}
 }
 
-void wml_menu_item::finish_handler() const
+void wml_menu_item::finish_handler()
 {
 	if(!command_.empty()) {
 		assert(resources::game_events);
@@ -198,11 +200,11 @@ void wml_menu_item::finish_handler() const
 
 	// Hotkey support
 	if(use_hotkey_) {
-		hotkey::remove_wml_hotkey(hotkey_id_);
+		hotkey_record_.reset();
 	}
 }
 
-void wml_menu_item::init_handler() const
+void wml_menu_item::init_handler()
 {
 	// If this menu item has a [command], add a handler for it.
 	if(!command_.empty()) {
@@ -212,7 +214,7 @@ void wml_menu_item::init_handler() const
 
 	// Hotkey support
 	if(use_hotkey_) {
-		hotkey::add_wml_hotkey(hotkey_id_, description_, default_hotkey_);
+		hotkey_record_.emplace(hotkey_id_, description_, default_hotkey_);
 	}
 }
 
@@ -321,15 +323,15 @@ void wml_menu_item::update(const vconfig& vcfg)
 
 	if(use_hotkey_ && !old_use_hotkey) {
 		// The hotkey needs to be enabled.
-		hotkey::add_wml_hotkey(hotkey_id_, description_, default_hotkey_);
+		hotkey_record_.emplace(hotkey_id_, description_, default_hotkey_);
 
 	} else if(use_hotkey_ && hotkey_updated) {
 		// The hotkey needs to be updated.
-		hotkey::add_wml_hotkey(hotkey_id_, description_, default_hotkey_);
+		hotkey_record_.emplace(hotkey_id_, description_, default_hotkey_);
 
 	} else if(!use_hotkey_ && old_use_hotkey) {
 		// The hotkey needs to be disabled.
-		hotkey::remove_wml_hotkey(hotkey_id_);
+		hotkey_record_.reset();
 	}
 }
 

src/game_events/menu_item.hpp

@@ -21,8 +21,11 @@
 #pragma once
 
 #include "config.hpp"
+#include "hotkey/hotkey_command.hpp"
 #include "variable.hpp"
 
+#include <optional>
+
 class filter_context;
 class game_data;
 struct map_location;
@@ -103,10 +106,10 @@ class wml_menu_item
 	void fire_event(const map_location& event_hex, const game_data& data) const;
 
 	/** Removes the implicit event handler for an inlined [command]. */
-	void finish_handler() const;
+	void finish_handler();
 
 	/** Initializes the implicit event handler for an inlined [command]. */
-	void init_handler() const;
+	void init_handler();
 
 	/**
 	 * The text to put in a menu for this item.
@@ -150,6 +153,9 @@ class wml_menu_item
 	/** The id for this item's hotkey; based on the item's id. */
 	const std::string hotkey_id_;
 
+	/** Controls the lifetime of the associate hotkey's hotkey_command. */
+	std::optional<hotkey::wml_hotkey_record> hotkey_record_;
+
 	/** The image to display in the menu next to this item's description. */
 	std::string image_;
 

src/gui/dialogs/preferences_dialog.cpp

@@ -795,7 +795,7 @@ listbox& preferences_dialog::setup_hotkey_list()
 	const std::string eh = "<span color='#0f0'>" + _("editor_hotkeys^E") + "</span>";
 	const std::string mh = "<span color='#0f0'>" + _("mainmenu_hotkeys^M") + "</span>";
 
-	for(const auto& hotkey_item : hotkey::get_hotkey_commands()) {
+	for(const auto& [id, hotkey_item] : hotkey::get_hotkey_commands()) {
 		if(hotkey_item.hidden) {
 			continue;
 		}

src/hotkey/hotkey_command.cpp

@@ -324,11 +324,7 @@ const std::set<HOTKEY_COMMAND> toggle_commands {
 };
 
 // Contains copies of master_hotkey_list and all current active wml menu hotkeys
-// TODO: Maybe known_hotkeys is not a fitting name anymore.
-std::vector<hotkey::hotkey_command> known_hotkeys;
-
-// Index map for known_hotkeys. Since known_hotkeys begins with master_hotkey_list, they are also indexes for master_hotkey_list.
-std::map<std::string, std::size_t> command_map;
+std::map<std::string_view, hotkey::hotkey_command> registered_hotkeys;
 
 hk_scopes scope_active(0);
 } // end anon namespace
@@ -373,75 +369,40 @@ bool is_scope_active(hk_scopes s)
 
 const hotkey_command& get_hotkey_command(const std::string& command)
 {
-	if(command_map.find(command) == command_map.end()) {
+	try {
+		return registered_hotkeys.at(command);
+	} catch(const std::out_of_range&) {
 		return get_hotkey_null();
 	}
-
-	return known_hotkeys[command_map[command]];
 }
 
-const std::vector<hotkey_command>& get_hotkey_commands()
+const std::map<std::string_view, hotkey::hotkey_command>& get_hotkey_commands()
 {
-	return known_hotkeys;
-}
-
-// Returns whether a hotkey was deleted.
-bool remove_wml_hotkey(const std::string& id)
-{
-	const hotkey::hotkey_command& command = get_hotkey_command(id);
-
-	if(command.command == hotkey::HOTKEY_NULL) {
-		LOG_G << "remove_wml_hotkey: command with id=" + id + " doesn't exist\n";
-		return false;
-	} else if(command.command != hotkey::HOTKEY_WML) {
-		LOG_G << "remove_wml_hotkey: command with id=" + id + " cannot be removed because it is no wml menu hotkey\n";
-		return false;
-	} else {
-		LOG_G << "removing wml hotkey with id=" + id + "\n";
-
-		// Iterate in reverse since it's almost certain the appropriate hotkey will be near
-		// the end, since this function is used to removed WML hotkeys, which are added after
-		// the built-ins.
-		for(auto itor = known_hotkeys.rbegin(); itor != known_hotkeys.rend(); ++itor) {
-			if(itor->id == id) {
-				known_hotkeys.erase(std::next(itor).base());
-				break;
-			}
-		}
-
-		// command_map might be all wrong now, so we need to rebuild.
-		command_map.clear();
-
-		for(std::size_t index = 0; index < known_hotkeys.size(); ++index) {
-			command_map[known_hotkeys[index].id] = index;
-		}
-
-		return true;
-	}
+	return registered_hotkeys;
 }
 
 bool has_hotkey_command(const std::string& id)
 {
 	return get_hotkey_command(id).command != hotkey::HOTKEY_NULL;
 }
 
-void add_wml_hotkey(const std::string& id, const t_string& description, const config& default_hotkey)
+wml_hotkey_record::wml_hotkey_record(const std::string& id, const t_string& description, const config& default_hotkey)
+	: cleanup_()
 {
 	if(id == "null") {
 		LOG_G << "Couldn't add wml hotkey with null id and description = '" << description << "'.\n";
 		return;
 	}
 
-	if(has_hotkey_command(id)) {
-		LOG_G << "Hotkey with id '" << id << "' already exists. Deleting the old hotkey_command.\n";
-		remove_wml_hotkey(id);
-	}
-
-	DBG_G << "Added wml hotkey with id = '" << id << "' and description = '" << description << "'.\n";
-
-	known_hotkeys.emplace_back(hotkey::HOTKEY_WML, id, description, false, false, scope_game, HKCAT_CUSTOM, t_string(""));
+	const auto& [iter, inserted] = registered_hotkeys.try_emplace(
+		id, hotkey::HOTKEY_WML, id, description, false, false, scope_game, HKCAT_CUSTOM, t_string(""));
 
-	command_map[id] = known_hotkeys.size() - 1;
+	if(inserted) {
+		DBG_G << "Added wml hotkey with id = '" << id << "' and description = '" << description << "'.\n";
+	} else {
+		LOG_G << "Hotkey with id '" << id << "' already exists.\n";
+		return;
+	}
 
 	if(!default_hotkey.empty() && !has_hotkey_item(id)) {
 		hotkey::hotkey_ptr new_item = hotkey::load_from_config(default_hotkey);
@@ -454,6 +415,16 @@ void add_wml_hotkey(const std::string& id, const t_string& description, const co
 			ERR_CF << "failed to add default hotkey with id=" + id;
 		}
 	}
+
+	// Record the cleanup handler
+	cleanup_ = [i = iter] { registered_hotkeys.erase(i); };
+}
+
+wml_hotkey_record::~wml_hotkey_record()
+{
+	if(cleanup_) {
+		cleanup_();
+	}
 }
 
 hotkey_command::hotkey_command(const hotkey_command_temp& temp_command)
@@ -511,7 +482,7 @@ bool hotkey_command::null() const
 
 const hotkey_command& hotkey_command::get_command_by_command(hotkey::HOTKEY_COMMAND command)
 {
-	for(hotkey_command& cmd : known_hotkeys) {
+	for(auto& [id, cmd] : registered_hotkeys) {
 		if(cmd.command == command) {
 			return cmd;
 		}
@@ -526,17 +497,7 @@ const hotkey_command& hotkey_command::get_command_by_command(hotkey::HOTKEY_COMM
 
 const hotkey_command& get_hotkey_null()
 {
-	// It is the last entry in that array, and the indexes in master_hotkey_list and known_hotkeys are the same.
-	return known_hotkeys[master_hotkey_list.size() - 1];
-}
-
-void delete_all_wml_hotkeys()
-{
-	while(known_hotkeys.back().command == hotkey::HOTKEY_WML) {
-		command_map.erase(known_hotkeys.back().id);
-
-		known_hotkeys.pop_back();
-	}
+	return registered_hotkeys.at("null");
 }
 
 const std::string& get_description(const std::string& command)
@@ -552,29 +513,14 @@ const std::string& get_tooltip(const std::string& command)
 
 void init_hotkey_commands()
 {
-	known_hotkeys.clear();
-
-	// Reserve enough space for the built-in hotkeys and 20 extra spaces for WML hotkeys. This is
-	// to avoid reallocation of this huge vector when any of the latter are added. 20 is honestly
-	// overkill, since there's really no reason to ever have near that many hotkey-enabled WML menu
-	// items, but it doesn't cost us anything to have extra.
-	known_hotkeys.reserve(master_hotkey_list.size() + 20);
+	registered_hotkeys.clear();
 
-	std::size_t i = 0;
 	for(const hotkey_command_temp& cmd : master_hotkey_list) {
 		// Initialize the full hotkey from the temp data.
-		auto& h = known_hotkeys.emplace_back(cmd);
-
-		// Note the known_hotkeys index associated with this command.
-		command_map[h.id] = i++;
+		registered_hotkeys.try_emplace(cmd.id, cmd);
 	}
 }
 
-void clear_hotkey_commands()
-{
-	command_map.clear();
-}
-
 HOTKEY_COMMAND get_id(const std::string& command)
 {
 	return get_hotkey_command(command).command;

src/hotkey/hotkey_command.hpp

@@ -19,6 +19,7 @@
 #include "tstring.hpp"
 
 #include <bitset>
+#include <functional>
 #include <list>
 #include <map>
 #include <vector>
@@ -235,7 +236,7 @@ struct hotkey_command
 	/** Constructs a new command from a temporary static hotkey object. */
 	hotkey_command(const hotkey_command_temp& temp_command);
 
-	/** @todo: remove this with c++20. We can use aggregate initialization with emplace_back.*/
+	/** @todo: see if we can remove this with c++20. Aggregate initialization with try_emplace?*/
 	hotkey_command(HOTKEY_COMMAND cmd, const std::string& id, const t_string& desc, bool hidden, bool toggle, hk_scopes scope, HOTKEY_CATEGORY category, const t_string& tooltip);
 
 	hotkey_command(const hotkey_command&) = default;
@@ -292,7 +293,7 @@ class scope_changer {
  * returns a container that contains all currently active hotkey_commands.
  * everything that wants a hotkey, must be in this container
  */
-const std::vector<hotkey_command>& get_hotkey_commands();
+const std::map<std::string_view, hotkey::hotkey_command>& get_hotkey_commands();
 
 /** returns the hotkey_command with the given name */
 const hotkey_command& get_hotkey_command(const std::string& command);
@@ -309,23 +310,32 @@ bool is_scope_active(hk_scopes s);
 bool has_hotkey_command(const std::string& id);
 
 /**
- * adds a new wml hotkey to the list, but only if there is no hotkey with that id yet on the list.
- * the object that is created here will be deleted in "delete_all_wml_hotkeys()"
+ * RAII helper class to control the lifetime of a WML hotkey_command.
  */
-void add_wml_hotkey(const std::string& id, const t_string& description, const config& default_hotkey);
+class wml_hotkey_record
+{
+public:
+	wml_hotkey_record() = default;
+
+	/** Don't allow copying so objects don't get erased early. */
+	wml_hotkey_record(const wml_hotkey_record&) = delete;
+	const wml_hotkey_record& operator=(const wml_hotkey_record&) = delete;
+
+	/** Registers a hotkey_command for a WML hotkey with the given ID if one does not already exist. */
+	wml_hotkey_record(const std::string& id, const t_string& description, const config& default_hotkey);
 
-/** deletes all wml hotkeys, should be called after a game has ended */
-void delete_all_wml_hotkeys();
-/** removes a wml hotkey with the given id, returns true if the deletion was successful */
-bool remove_wml_hotkey(const std::string& id);
+	~wml_hotkey_record();
+
+private:
+	/** Handles removing the associated hotkey_command on this object's destruction. */
+	std::function<void()> cleanup_{};
+};
 
 const std::string& get_description(const std::string& command);
 const std::string& get_tooltip(const std::string& command);
 
 void init_hotkey_commands();
 
-void clear_hotkey_commands();
-
 /** returns get_hotkey_command(command).id */
 HOTKEY_COMMAND get_id(const std::string& command);
 }

src/hotkey/hotkey_manager.cpp

@@ -33,9 +33,7 @@ void manager::init()
 
 void manager::wipe()
 {
-	clear_hotkey_commands();
 	clear_hotkeys();
-	delete_all_wml_hotkeys();
 }
 
 manager::~manager()

src/play_controller.cpp

@@ -201,7 +201,6 @@ play_controller::play_controller(const config& level, saved_game& state_of_game,
 play_controller::~play_controller()
 {
 	unit_types.remove_scenario_fixes();
-	hotkey::delete_all_wml_hotkeys();
 	clear_resources();
 }