widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1691335-network-empty-map-wincondition into lp:widelands.

Commit message:
Fixed crash in multiplayer client when the host hasn't selected a map yet.
- Catch exception when server sends a garbage win condition and display an error message in the win condition dropdown.
- Enable loading of win conditions for host when no map is selected. The above exception no longer occurs.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1691335 in widelands: "Multiplayer clients crashes if no map has been selected"
  https://bugs.launchpad.net/widelands/+bug/1691335

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1691335-network-empty-map-wincondition/+merge/324160

This fixes the loading of win conditions in multiplayer.

I'd still like to know though why we got a crash instead of an exception - the culprit here is r8353:

https://code.launchpad.net/~widelands-dev/widelands/refac-netcode/+merge/323798

-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1691335-network-empty-map-wincondition into lp:widelands.

=== modified file 'src/scripting/run_script.cc'
--- src/scripting/run_script.cc	2017-01-25 18:55:59 +0000
+++ src/scripting/run_script.cc	2017-05-17 09:02:03 +0000
@@ -21,6 +21,8 @@
 
 #include <memory>
 
+#include <boost/format.hpp>
+
 #include "io/filesystem/filesystem.h"
 #include "scripting/lua_table.h"
 
@@ -31,6 +33,9 @@
 	if (!fs || !fs->file_exists(filename)) {
 		throw LuaScriptNotExistingError(filename);
 	}
+	if (fs->is_directory(filename)) {
+		throw LuaScriptNotExistingError((boost::format("%s is a directory") % filename.c_str()).str());
+	}
 	size_t length;
 	void* input_data = fs->load(filename, length);
 	const std::string data(static_cast<char*>(input_data));

=== modified file 'src/ui_fsmenu/launch_game.cc'
--- src/ui_fsmenu/launch_game.cc	2017-05-07 17:10:56 +0000
+++ src/ui_fsmenu/launch_game.cc	2017-05-17 09:02:03 +0000
@@ -124,29 +124,23 @@
  */
 void FullscreenMenuLaunchGame::update_win_conditions() {
 	if (!init_win_condition_label()) {
-		Widelands::Map map;
-		std::unique_ptr<Widelands::MapLoader> ml =
-		   map.get_correct_loader(settings_->settings().mapfilename);
-		if (ml != nullptr) {
-			ml->preload_map(true);
-			load_win_conditions(map);
-		} else {
-			const std::string error_message =
-			   (boost::format(_("Unable to determine valid win conditions because the map '%s' could "
-			                    "not be loaded.")) %
-			    settings_->settings().mapfilename)
-			      .str();
-			win_condition_dropdown_.set_label(_("Error"));
-			win_condition_dropdown_.set_tooltip(error_message);
-			log("Launch Game: No map loader: %s\n", error_message.c_str());
+		std::set<std::string> tags;
+		if (!settings_->settings().mapfilename.empty()) {
+		 	Widelands::Map map;
+			std::unique_ptr<Widelands::MapLoader> ml =
+				map.get_correct_loader(settings_->settings().mapfilename);
+			if (ml != nullptr) {
+				ml->preload_map(true);
+				tags = map.get_tags();
+			}
 		}
+		load_win_conditions(tags);
 	}
 }
 
-void FullscreenMenuLaunchGame::load_win_conditions(const Widelands::Map& map) {
+void FullscreenMenuLaunchGame::load_win_conditions(const std::set<std::string>& tags) {
 	win_condition_dropdown_.clear();
 	try {
-		const std::set<std::string> tags = map.get_tags();
 		// Make sure that the last win condition is still valid. If not, pick the first one
 		// available.
 		if (last_win_condition_.empty()) {

=== modified file 'src/ui_fsmenu/launch_game.h'
--- src/ui_fsmenu/launch_game.h	2017-02-10 17:59:49 +0000
+++ src/ui_fsmenu/launch_game.h	2017-05-17 09:02:03 +0000
@@ -64,9 +64,9 @@
 	/// Loads all win conditions that can be played with the map into the selection dropdown.
 	/// Disables the dropdown if the map is a scenario.
 	void update_win_conditions();
-	/// Reads the win conditions that are available for the given map and adds the entries to the
+	/// Reads the win conditions that are available for the given map tags and adds the entries to the
 	/// dropdown.
-	void load_win_conditions(const Widelands::Map& map);
+	void load_win_conditions(const std::set<std::string>& tags);
 	/// Remembers the win condition that is currently selected in the dropdown.
 	virtual void win_condition_selected() = 0;
 	/// If the win condition in 'win_condition_script' can be played with the map tags,

=== modified file 'src/ui_fsmenu/launch_mpg.cc'
--- src/ui_fsmenu/launch_mpg.cc	2017-04-21 09:56:42 +0000
+++ src/ui_fsmenu/launch_mpg.cc	2017-05-17 09:02:03 +0000
@@ -412,6 +412,12 @@
 				win_condition_dropdown_.set_label(_(t->get_string("name")));
 				win_condition_dropdown_.set_tooltip(_(t->get_string("description")));
 			}
+		} catch (LuaScriptNotExistingError&) {
+			win_condition_dropdown_.set_label(_("Error"));
+			win_condition_dropdown_.set_tooltip(
+						(boost::format(_("Unable to load the win condition script file '%s'."))
+							% settings_->get_win_condition_script()).str());
+
 		} catch (LuaTableKeyError& e) {
 			log("LaunchMPG: Error loading win condition: %s %s\n",
 			    settings_->get_win_condition_script().c_str(), e.what());