Attempt refactoring 1

src/game_events/handlers.cpp

@@ -47,11 +47,11 @@ namespace game_events
 {
 /* ** event_handler ** */
 
-event_handler::event_handler(const config& cfg, bool imi, handler_vec::size_type index, manager& man)
+event_handler::event_handler(const config& cfg, bool imi, handler_vec::size_type index)
 	: first_time_only_(cfg["first_time_only"].to_bool(true))
 	, is_menu_item_(imi)
+	, disabled_(false)
 	, index_(index)
-	, man_(&man)
 	, cfg_(cfg)
 {
 }
@@ -66,31 +66,29 @@ event_handler::event_handler(const config& cfg, bool imi, handler_vec::size_type
  */
 void event_handler::disable()
 {
-	assert(man_);
-	assert(man_->event_handlers_);
+	assert(!disabled_);;
+	disabled_ = true;
+
+	//assert(man_);
+	//assert(man_->event_handlers_);
 
 	// Handlers must have an index after they're created.
-	assert(index_ < man_->event_handlers_->size());
+	//assert(index_ < man_->event_handlers_->size());
 
 	// Disable this handler.
-	(*man_->event_handlers_)[index_].reset();
+	//(*man_->event_handlers_)[index_].reset();
 }
 
 /**
  * Handles the queued event, according to our WML instructions.
- * WARNING: *this may be destroyed at the end of this call, unless
- *          the caller maintains a handler_ptr to this.
  *
  * @param[in]     event_info  Information about the event that needs handling.
- * @param[in,out] handler_p   The caller's smart pointer to *this. It may be
- *                            reset() during processing.
  */
-void event_handler::handle_event(const queued_event& event_info, handler_ptr& handler_p, game_lua_kernel& lk)
+void event_handler::handle_event(const queued_event& event_info, game_lua_kernel& lk)
 {
-	// We will need our config after possibly self-destructing. Make a copy now.
-	// TODO: instead of copying possibly huge config objects we should use shared things and only increase a refcount
-	// here.
-	vconfig vcfg(cfg_, true);
+	if(disabled_) {
+		return;
+	}
 
 	if(is_menu_item_) {
 		DBG_NG << cfg_["name"] << " will now invoke the following command(s):\n" << cfg_;
@@ -101,11 +99,10 @@ void event_handler::handle_event(const queued_event& event_info, handler_ptr& ha
 		disable();
 
 		// Also remove our caller's hold on us.
-		handler_p.reset();
+	//	handler_p.reset();
 	}
-	// *WARNING*: At this point, dereferencing this could be a memory violation!
 
-	lk.run_wml_action("command", vcfg, event_info);
+	lk.run_wml_action("command", vconfig(cfg_, false), event_info);
 	sound::commit_music_changes();
 }
 

src/game_events/handlers.hpp

@@ -48,7 +48,12 @@ typedef std::vector<handler_ptr> handler_vec;
 class event_handler
 {
 public:
-	event_handler(const config& cfg, bool is_menu_item, handler_vec::size_type index, manager&);
+	event_handler(const config& cfg, bool is_menu_item, handler_vec::size_type index);
+
+	bool disabled() const
+	{
+		return disabled_;
+	}
 
 	/** The index of *this should only be of interest when controlling iterations. */
 	handler_vec::size_type index() const
@@ -63,9 +68,10 @@ class event_handler
 		return is_menu_item_;
 	}
 
-	/** Disables *this, removing it from the game. */
+	/** Flag this as disabled. */
 	void disable();
-	void handle_event(const queued_event& event_info, handler_ptr& handler_p, game_lua_kernel&);
+
+	void handle_event(const queued_event& event_info, game_lua_kernel&);
 
 	const config& get_config() const
 	{
@@ -75,8 +81,8 @@ class event_handler
 private:
 	bool first_time_only_;
 	bool is_menu_item_;
+	bool disabled_;
 	handler_vec::size_type index_;
-	manager* man_;
 	config cfg_;
 };
 

src/game_events/manager.cpp

@@ -44,7 +44,7 @@ namespace game_events
 /** Create an event handler. */
 void manager::add_event_handler(const config& handler, bool is_menu_item)
 {
-	event_handlers_->add_event_handler(handler, *this, is_menu_item);
+	event_handlers_->add_event_handler(handler, is_menu_item);
 }
 
 /** Removes an event handler. */
@@ -108,15 +108,6 @@ manager::iteration::iteration(const std::string& event_name, manager& man)
 	, var_it_(event_name.empty() ? var_list_.end() : var_list_.begin())
 	, gamedata_(resources::gamedata)
 {
-	// Clean up expired ptrs. This saves us effort later since it ensures every ptr is valid.
-
-	//main_list_.remove_if(
-	//	[](weak_handler_ptr ptr) { return ptr.expired(); }
-	//);
-
-	//var_list_.remove_if(
-	//	[](weak_handler_ptr ptr) { return ptr.expired(); }
-	//);
 }
 
 /**
@@ -182,6 +173,7 @@ handler_ptr manager::iteration::operator*()
 
 	// Which list? (Index ties go to the main list.)
 	current_is_known_ = main_index < end_ || var_index < end_;
+	//current_is_known_ = main_ptr != nullptr || var_ptr != nullptr;
 	main_is_current_ = main_index <= var_index;
 
 	if(!current_is_known_) {
@@ -233,6 +225,9 @@ void manager::execute_on_events(const std::string& event_id, manager::event_func
 		func(*this, hand);
 		++iter;
 	}
+
+	// Clean up expired ptrs. This saves us effort later since it ensures every ptr is valid.
+	event_handlers_->drop_expired_handlers(event_id);
 }
 
 game_events::wml_event_pump& manager::pump()

src/game_events/manager_impl.cpp

@@ -95,7 +95,7 @@ handler_list& event_handlers::get(const std::string& name)
  * An event with a nonempty ID will not be added if an event with that
  * ID already exists.
  */
-void event_handlers::add_event_handler(const config& cfg, manager& man, bool is_menu_item)
+void event_handlers::add_event_handler(const config& cfg, bool is_menu_item)
 {
 	const std::string name = cfg["name"];
 	std::string id = cfg["id"];
@@ -111,7 +111,7 @@ void event_handlers::add_event_handler(const config& cfg, manager& man, bool is_
 
 	// Create a new handler.
 	DBG_EH << "inserting event handler for name=" << name << " with id=" << id << "\n";
-	handler_ptr new_handler(new event_handler(cfg, is_menu_item, active_.size(), man));
+	handler_ptr new_handler(new event_handler(cfg, is_menu_item, active_.size()));
 
 	active_.push_back(new_handler);
 
@@ -162,6 +162,44 @@ void event_handlers::remove_event_handler(const std::string& id)
 	log_handlers();
 }
 
+namespace
+{
+bool is_expired(weak_handler_ptr ptr)
+{
+	return ptr.expired();
+}
+
+} // end anon namespace
+
+void event_handlers::drop_expired_handlers(const std::string& event_name)
+{
+#if 0
+	std::vector<handler_vec::iterator> to_remove;
+
+	// First, clean up the 'owning' list.
+	for(auto iter = active_.begin(); iter != active_.end(); ++iter) {
+		// if a handler is disabled...
+		if((*iter)->disabled()) {
+			// ...reset the shared_ptr that owns it...
+			iter->reset();
+
+			// ...and note this entry for removal.
+			to_remove.push_back(iter);
+		}
+	}
+#endif
+
+	auto to_remove = std::remove_if(active_.begin(), active_.end(), [](handler_ptr p) { return p->disabled(); });
+	active_.erase(to_remove, active_.end());
+
+	get(event_name).remove_if(is_expired);
+	dynamic_.remove_if(is_expired);
+
+	//for(auto& i : to_remove) {
+	//	active_.erase(i);
+	//}
+}
+
 const handler_ptr event_handlers::get_event_handler_by_id(const std::string& id)
 {
 	auto find_it = id_map_.find(id);

src/game_events/manager_impl.hpp

@@ -77,11 +77,14 @@ class event_handlers
 	handler_list& get(const std::string& name);
 
 	/** Adds an event handler. */
-	void add_event_handler(const config& cfg, manager& man, bool is_menu_item = false);
+	void add_event_handler(const config& cfg, bool is_menu_item = false);
 
 	/** Removes an event handler, identified by its ID. */
 	void remove_event_handler(const std::string& id);
 
+	/** Removes all expired event handlers and any weak_ptrs to them. */
+	void drop_expired_handlers(const std::string& event_name);
+
 	/** Gets an event handler, identified by its ID. */
 	const handler_ptr get_event_handler_by_id(const std::string& id);
 

src/game_events/pump.cpp

@@ -303,7 +303,7 @@ void wml_event_pump::process_event(handler_ptr& handler_p, const queued_event& e
 	++impl_->internal_wml_tracking;
 	context::scoped evc(impl_->contexts_);
 	assert(resources::lua_kernel != nullptr);
-	handler_p->handle_event(ev, handler_p, *resources::lua_kernel);
+	handler_p->handle_event(ev, *resources::lua_kernel);
 	// NOTE: handler_p may be null at this point!
 
 	if(ev.name == "select") {