Merge lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands

[Klaus Halfmann]

Refactor gameclient.h/.cc for better readability

* Refactor big switch statement in gameclient.cc
* Refactor ::run() to improve readability
* use send_player_command with Ptr instead of Reference to clarify ownership of PlayerCommand.
* Added more comments

Added some TODOs and empty comments where I did not know the details.

There should be no functional change at all.

[Klaus Halfmann (klaus-halfmann)]

Regressiontest are ok: Ran 44 tests in 1377.106s
Will try to do a network game, too.

[Klaus Halfmann (klaus-halfmann)]

Hello Gun:
* Adressed some comments
* Get the tribe and check whether it exists before you push it
  -> I have no idea (yet) what this code does :-)
* Playercommand is not for current player , not sure if this can normally happen
  lets wait If I ever see this, then we can make it an assert.
* The This / this confusion is because I moved some fucntion into Impl, but they need
  the main class. I could use "outer" or some other name to avoid it.
* send_player_command(Widelands::PlayerCommand* pc) making this a unique_ptr:
  * Will affect a lot of code
  * Some aspects of this code are not clear to me (e.g. Commnands a queued for networking)
  -> lets adress this in a seperate branch, I already regret I touched it :-)
* I would leave "GameClientImpl* d" as is:
  * it is used very often
  * it is used in the local scope only -> no risc to loose control
  * commonly used Pattern in Widelands
* d->settings.usernum == -2,-1, n seems to be used to define the "hello" handshake
  what about "kPreHello" and "kAfterHello" constants?

Maybe we should move the two remaining variables to GameClientImpl, too?

Please check the logic of the main switch statement, I changed some control
flow for better readbility.

I intend to do a similar refactoring with gamehost, too.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4845. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/526796650.
Appveyor build 4626. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4626.

[GunChleoc (gunchleoc)]

* Get the tribe and check whether it exists before you push it
  -> I have no idea (yet) what this code does :-)

It's for the tips in the game loading screen. We have tribe-specific lists of tips.

There is a function bool Widelands::tribe_exists(const std::string& tribename) that you can use.

* The This / this confusion is because I moved some fucntion into Impl, but they need
  the main class. I could use "outer" or some other name to avoid it.

I see now - it's just an ugly variable name, yes, please change the name to "game_client" or "parent" or some such.

* send_player_command(Widelands::PlayerCommand* pc) making this a unique_ptr:
  -> lets adress this in a seperate branch, I already regret I touched it :-)

LOL fair enough. Cans or worms are best dealt with separately.

* I would leave "GameClientImpl* d" as is:
  * it is used very often
  * it is used in the local scope only -> no risc to loose control
  * commonly used Pattern in Widelands

That's because it was written before we switched to C++11, so there was no unique_ptr available. You don't have to do it in this branch though.

* d->settings.usernum == -2,-1, n seems to be used to define the "hello" handshake
  what about "kPreHello" and "kAfterHello" constants?

I have found this comment:
  /**
   * Checks if client \ref name exists and \returns int32_t :
   * - the client number if found
   * - -1 if no client was found
   * - -2 if the host is the client (has no client number)
   */

So, kUnknownClient and kUnknownHost?

* Maybe we should move the two remaining variables to GameClientImpl, too?

Consistency is good :)

[Klaus Halfmann (klaus-halfmann)]

* Used parent instead of This.
* moved all Variables into Impl

About that -2 magic: I got lost in finding the sematics of user/player etc.
I added a TODO comment with our best guess by now.

I am at the End of my vacation (sigh) so lets get this in before it
starts lingering around for too long.

[Klaus Halfmann (klaus-halfmann)]

Found a first bug, still need some debugging

[Klaus Halfmann (klaus-halfmann)]

Still get a heap-use-after-free related to
    std::unique_ptr<UI::ProgressWindow> loader_ui(new UI::ProgressWindow());

But this is maybe just a problem as the game crashed with disconnect(CLIENT_CRASHED, )

A have a deja vue around GameClientImpl.modal which is released twice.

[GunChleoc (gunchleoc)]

You could also do the game tips like this:

    const std::string tribename = get_players_tribe();
    assert(Widelands::tribe_exists(tribename));
    std::unique_ptr<GameTips> tips(new GameTips(*loader, {"general_game", "multiplayer", tribename}));

As to the ASan crash, I suspect that there is a problem with the lifetime of an object caused by the pulling out of stuff into separate functions.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4871. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/527824464.
Appveyor build 4652. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4652.

[Klaus Halfmann (klaus-halfmann)]

Using unique_ptr<GameTips> is unneeded, this is only a helper class neede to as long as some progress dialog is open, so using normal scope is ok.
(We should actually cleanup that usage, some other time)

The Problem is the owenership of UI::ProgressWindow* loader

The disconnect code try to close the last 'modal' window.
In case of the loader this is incorrect.

Not sure how to cleanup tis messs by now

[Klaus Halfmann (klaus-halfmann)]

Found that I lost a '!' while refactoring.

Now a normal network game works as itendend.

I assume that disconnect/double freee has always been there,
I will create a followup bug for that one.
When debugging I triggered a racecondition, I doubt we ever will see that i the wild.

Gun: and all, please take anotheer look

review: Needs Resubmitting

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4891. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/528361519.
Appveyor build 4672. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4672.

[Klaus Halfmann (klaus-halfmann)]

Now why can we nort merge this one?

[GunChleoc (gunchleoc)]

Sorry, your recent changes slipped me by. Thanks for the reminder, I will do another review & testing.

[GunChleoc (gunchleoc)]

I have done some testing and it seems to work fine. I am not getting the double free you mentioned.

@bunnybot merge

review: Approve

=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc	2019-05-22 11:23:14 +0000
+++ src/ai/defaultai.cc	2019-05-25 17:10:52 +0000
@@ -6612,7 +6612,7 @@
 			const uint16_t new_target = std::max<uint16_t>(default_target * multiplier / 10, 3);
 			assert(new_target > 1);
 
-			game().send_player_command(*new Widelands::CmdSetWareTargetQuantity(
+			game().send_player_command(new Widelands::CmdSetWareTargetQuantity(
 			   gametime, player_number(), observer->economy.serial(), id, new_target));
 		}
 	}
 
=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2019-05-11 18:19:20 +0000
+++ src/logic/game.cc	2019-05-25 17:10:52 +0000
@@ -711,7 +711,7 @@
  * It takes the appropriate action, i.e. either add to the cmd_queue or send
  * across the network.
  */
-void Game::send_player_command(PlayerCommand& pc) {
+void Game::send_player_command(PlayerCommand* pc) {
 	ctrl_->send_player_command(pc);
 }
 
@@ -734,55 +734,55 @@
 
 // we might want to make these inlines:
 void Game::send_player_bulldoze(PlayerImmovable& pi, bool const recurse) {
-	send_player_command(*new CmdBulldoze(get_gametime(), pi.owner().player_number(), pi, recurse));
+	send_player_command(new CmdBulldoze(get_gametime(), pi.owner().player_number(), pi, recurse));
 }
 
 void Game::send_player_dismantle(PlayerImmovable& pi) {
-	send_player_command(*new CmdDismantleBuilding(get_gametime(), pi.owner().player_number(), pi));
+	send_player_command(new CmdDismantleBuilding(get_gametime(), pi.owner().player_number(), pi));
 }
 
 void Game::send_player_build(int32_t const pid, const Coords& coords, DescriptionIndex const id) {
 	assert(tribes().building_exists(id));
-	send_player_command(*new CmdBuild(get_gametime(), pid, coords, id));
+	send_player_command(new CmdBuild(get_gametime(), pid, coords, id));
 }
 
 void Game::send_player_build_flag(int32_t const pid, const Coords& coords) {
-	send_player_command(*new CmdBuildFlag(get_gametime(), pid, coords));
+	send_player_command(new CmdBuildFlag(get_gametime(), pid, coords));
 }
 
 void Game::send_player_build_road(int32_t pid, Path& path) {
-	send_player_command(*new CmdBuildRoad(get_gametime(), pid, path));
+	send_player_command(new CmdBuildRoad(get_gametime(), pid, path));
 }
 
 void Game::send_player_flagaction(Flag& flag) {
-	send_player_command(*new CmdFlagAction(get_gametime(), flag.owner().player_number(), flag));
+	send_player_command(new CmdFlagAction(get_gametime(), flag.owner().player_number(), flag));
 }
 
 void Game::send_player_start_stop_building(Building& building) {
 	send_player_command(
-	   *new CmdStartStopBuilding(get_gametime(), building.owner().player_number(), building));
+	   new CmdStartStopBuilding(get_gametime(), building.owner().player_number(), building));
 }
 
 void Game::send_player_militarysite_set_soldier_preference(Building& building,
                                                            SoldierPreference my_preference) {
-	send_player_command(*new CmdMilitarySiteSetSoldierPreference(
+	send_player_command(new CmdMilitarySiteSetSoldierPreference(
 	   get_gametime(), building.owner().player_number(), building, my_preference));
 }
 
 void Game::send_player_start_or_cancel_expedition(Building& building) {
 	send_player_command(
-	   *new CmdStartOrCancelExpedition(get_gametime(), building.owner().player_number(), building));
+	   new CmdStartOrCancelExpedition(get_gametime(), building.owner().player_number(), building));
 }
 
 void Game::send_player_enhance_building(Building& building, DescriptionIndex const id) {
 	assert(building.owner().tribe().has_building(id));
 
 	send_player_command(
-	   *new CmdEnhanceBuilding(get_gametime(), building.owner().player_number(), building, id));
+	   new CmdEnhanceBuilding(get_gametime(), building.owner().player_number(), building, id));
 }
 
 void Game::send_player_evict_worker(Worker& worker) {
-	send_player_command(*new CmdEvictWorker(get_gametime(), worker.owner().player_number(), worker));
+	send_player_command(new CmdEvictWorker(get_gametime(), worker.owner().player_number(), worker));
 }
 
 void Game::send_player_set_ware_priority(PlayerImmovable& imm,
@@ -790,14 +790,14 @@
                                          DescriptionIndex const index,
                                          int32_t const prio) {
 	send_player_command(
-	   *new CmdSetWarePriority(get_gametime(), imm.owner().player_number(), imm, type, index, prio));
+	   new CmdSetWarePriority(get_gametime(), imm.owner().player_number(), imm, type, index, prio));
 }
 
 void Game::send_player_set_input_max_fill(PlayerImmovable& imm,
                                           DescriptionIndex const index,
                                           WareWorker type,
                                           uint32_t const max_fill) {
-	send_player_command(*new CmdSetInputMaxFill(
+	send_player_command(new CmdSetInputMaxFill(
 	   get_gametime(), imm.owner().player_number(), imm, index, type, max_fill));
 }
 
@@ -805,50 +805,49 @@
                                                TrainingAttribute attr,
                                                int32_t const val) {
 	send_player_command(
-	   *new CmdChangeTrainingOptions(get_gametime(), ts.owner().player_number(), ts, attr, val));
+	   new CmdChangeTrainingOptions(get_gametime(), ts.owner().player_number(), ts, attr, val));
 }
 
 void Game::send_player_drop_soldier(Building& b, int32_t const ser) {
 	assert(ser != -1);
-	send_player_command(*new CmdDropSoldier(get_gametime(), b.owner().player_number(), b, ser));
+	send_player_command(new CmdDropSoldier(get_gametime(), b.owner().player_number(), b, ser));
 }
 
 void Game::send_player_change_soldier_capacity(Building& b, int32_t const val) {
 	send_player_command(
-	   *new CmdChangeSoldierCapacity(get_gametime(), b.owner().player_number(), b, val));
+	   new CmdChangeSoldierCapacity(get_gametime(), b.owner().player_number(), b, val));
 }
 
 void Game::send_player_enemyflagaction(const Flag& flag,
                                        PlayerNumber const who_attacks,
                                        const std::vector<Serial>& soldiers) {
 	if (1 < player(who_attacks)
-	           .vision(Map::get_index(flag.get_building()->get_position(), map().get_width()))) {
-		send_player_command(*new CmdEnemyFlagAction(get_gametime(), who_attacks, flag, soldiers));
-	}
+	           .vision(Map::get_index(flag.get_building()->get_position(), map().get_width())))
+		send_player_command(new CmdEnemyFlagAction(get_gametime(), who_attacks, flag, soldiers));
 }
 
 void Game::send_player_ship_scouting_direction(Ship& ship, WalkingDir direction) {
-	send_player_command(*new CmdShipScoutDirection(
+	send_player_command(new CmdShipScoutDirection(
 	   get_gametime(), ship.get_owner()->player_number(), ship.serial(), direction));
 }
 
 void Game::send_player_ship_construct_port(Ship& ship, Coords coords) {
-	send_player_command(*new CmdShipConstructPort(
+	send_player_command(new CmdShipConstructPort(
 	   get_gametime(), ship.get_owner()->player_number(), ship.serial(), coords));
 }
 
 void Game::send_player_ship_explore_island(Ship& ship, IslandExploreDirection direction) {
-	send_player_command(*new CmdShipExploreIsland(
+	send_player_command(new CmdShipExploreIsland(
 	   get_gametime(), ship.get_owner()->player_number(), ship.serial(), direction));
 }
 
 void Game::send_player_sink_ship(Ship& ship) {
 	send_player_command(
-	   *new CmdShipSink(get_gametime(), ship.get_owner()->player_number(), ship.serial()));
+	   new CmdShipSink(get_gametime(), ship.get_owner()->player_number(), ship.serial()));
 }
 
 void Game::send_player_cancel_expedition_ship(Ship& ship) {
-	send_player_command(*new CmdShipCancelExpedition(
+	send_player_command(new CmdShipCancelExpedition(
 	   get_gametime(), ship.get_owner()->player_number(), ship.serial()));
 }
 
@@ -856,7 +855,7 @@
 	auto* object = objects().get_object(trade.initiator);
 	assert(object != nullptr);
 	send_player_command(
-	   *new CmdProposeTrade(get_gametime(), object->get_owner()->player_number(), trade));
+	   new CmdProposeTrade(get_gametime(), object->get_owner()->player_number(), trade));
 }
 
 int Game::propose_trade(const Trade& trade) {
 
=== modified file 'src/logic/game.h'
--- src/logic/game.h	2019-05-07 12:14:02 +0000
+++ src/logic/game.h	2019-05-25 17:10:52 +0000
@@ -248,7 +248,7 @@
 
 	void enqueue_command(Command* const);
 
-	void send_player_command(Widelands::PlayerCommand&);
+	void send_player_command(Widelands::PlayerCommand*);
 
 	void send_player_bulldoze(PlayerImmovable&, bool recurse = false);
 	void send_player_dismantle(PlayerImmovable&);
 
=== modified file 'src/logic/game_controller.h'
--- src/logic/game_controller.h	2019-02-27 19:00:36 +0000
+++ src/logic/game_controller.h	2019-05-25 17:10:52 +0000
@@ -46,7 +46,9 @@
 	}
 
 	virtual void think() = 0;
-	virtual void send_player_command(Widelands::PlayerCommand&) = 0;
+
+	// TODO(Klaus Halfmann): Command must be deleted once it was handled.
+	virtual void send_player_command(Widelands::PlayerCommand*) = 0;
 	virtual int32_t get_frametime() = 0;
 	virtual GameType get_game_type() = 0;
 
 
=== modified file 'src/logic/replay_game_controller.cc'
--- src/logic/replay_game_controller.cc	2019-02-23 11:00:49 +0000
+++ src/logic/replay_game_controller.cc	2019-05-25 17:10:52 +0000
@@ -60,7 +60,7 @@
 	}
 }
 
-void ReplayGameController::send_player_command(Widelands::PlayerCommand&) {
+void ReplayGameController::send_player_command(Widelands::PlayerCommand*) {
 	throw wexception("Trying to send a player command during replay");
 }
 
 
=== modified file 'src/logic/replay_game_controller.h'
--- src/logic/replay_game_controller.h	2019-02-23 11:00:49 +0000
+++ src/logic/replay_game_controller.h	2019-05-25 17:10:52 +0000
@@ -35,7 +35,7 @@
 
 	void think() override;
 
-	void send_player_command(Widelands::PlayerCommand&) override;
+	void send_player_command(Widelands::PlayerCommand*) override;
 	int32_t get_frametime() override;
 	GameController::GameType get_game_type() override;
 	uint32_t real_speed() override;
 
=== modified file 'src/logic/single_player_game_controller.cc'
--- src/logic/single_player_game_controller.cc	2019-02-23 11:00:49 +0000
+++ src/logic/single_player_game_controller.cc	2019-05-25 17:10:52 +0000
@@ -74,9 +74,9 @@
 	}
 }
 
-void SinglePlayerGameController::send_player_command(Widelands::PlayerCommand& pc) {
-	pc.set_cmdserial(++player_cmdserial_);
-	game_.enqueue_command(&pc);
+void SinglePlayerGameController::send_player_command(Widelands::PlayerCommand* pc) {
+	pc->set_cmdserial(++player_cmdserial_);
+	game_.enqueue_command(pc);
 }
 
 int32_t SinglePlayerGameController::get_frametime() {
 
=== modified file 'src/logic/single_player_game_controller.h'
--- src/logic/single_player_game_controller.h	2019-02-27 19:00:36 +0000
+++ src/logic/single_player_game_controller.h	2019-05-25 17:10:52 +0000
@@ -30,7 +30,7 @@
 	~SinglePlayerGameController() override;
 
 	void think() override;
-	void send_player_command(Widelands::PlayerCommand&) override;
+	void send_player_command(Widelands::PlayerCommand*) override;
 	int32_t get_frametime() override;
 	GameController::GameType get_game_type() override;
 	uint32_t real_speed() override;
 
=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc	2019-04-29 16:22:08 +0000
+++ src/network/gameclient.cc	2019-05-25 17:10:52 +0000
@@ -58,6 +58,8 @@
 #include "wui/interactive_spectator.h"
 
 struct GameClientImpl {
+	bool internet_;
+
 	GameSettings settings;
 
 	std::string localplayername;
@@ -91,13 +93,121 @@
 
 	/// Backlog of chat messages
 	std::vector<ChatMessage> chatmessages;
+
+	/** File that is eventually transferred via the network if not found at the other side */
+	std::unique_ptr<NetTransferFile> file_;
+
+	void send_hello();
+	void send_player_command(Widelands::PlayerCommand*);
+
+	bool run_map_menu(GameClient* parent);
+	void run_game(InteractiveGameBase* igb, UI::ProgressWindow*);
+
+	InteractiveGameBase* init_game(GameClient* parent, UI::ProgressWindow*);
+
 };
 
+void GameClientImpl::send_hello() {
+	SendPacket s;
+	s.unsigned_8(NETCMD_HELLO);
+	s.unsigned_8(NETWORK_PROTOCOL_VERSION);
+	s.string(localplayername);
+	s.string(build_id());
+	net->send(s);
+}
+
+void GameClientImpl::send_player_command(Widelands::PlayerCommand* pc) {
+	SendPacket s;
+	s.unsigned_8(NETCMD_PLAYERCOMMAND);
+	s.signed_32(game->get_gametime());
+	pc->serialize(s);
+	net->send(s);
+}
+
+/**
+ * Show and run() the fullscreen menu for setting map and mapsettings.
+ *
+ *  @return true to indicate that run is done.
+ */
+bool GameClientImpl::run_map_menu(GameClient* parent) {
+	FullscreenMenuLaunchMPG lgm(parent, parent);
+	lgm.set_chat_provider(*parent);
+	modal = &lgm;
+	FullscreenMenuBase::MenuTarget code = lgm.run<FullscreenMenuBase::MenuTarget>();
+	modal = nullptr;
+	if (code == FullscreenMenuBase::MenuTarget::kBack) {
+		// if this is an internet game, tell the metaserver that client is back in the lobby.
+		if (internet_) {
+			InternetGaming::ref().set_game_done();
+		}
+		return true;
+	}
+	return false;
+}
+
+/**
+ * Show progress dialog and load map or saved game.
+ */
+InteractiveGameBase* GameClientImpl::init_game(GameClient* parent, UI::ProgressWindow* loader) {
+
+	const std::string& tribename = parent->get_players_tribe();
+	assert(Widelands::tribe_exists(tribename));
+	GameTips tips(*loader, {"general_game", "multiplayer", tribename});
+
+	modal = loader;
+
+	loader->step(_("Preparing game"));
+
+	game->set_game_controller(parent);
+	uint8_t const pn = settings.playernum + 1;
+	game->save_handler().set_autosave_filename(
+					(boost::format("%s_netclient%u") % kAutosavePrefix % static_cast<unsigned int>(pn)).str());
+	InteractiveGameBase* igb;
+	if (pn > 0) {
+		igb = new InteractivePlayer(*game, g_options.pull_section("global"), pn, true);
+	} else {
+		igb = new InteractiveSpectator(*game, g_options.pull_section("global"), true);
+	}
+	game -> set_ibase(igb);
+	igb->set_chat_provider(*parent);
+	if (settings.savegame) {  // savegame
+		game->init_savegame(loader, settings);
+	} else {                  //  new map
+		game->init_newgame(loader, settings);
+	}
+	return igb;
+}
+
+
+/**
+ * Run the actual game and cleanup when done.
+ */
+void GameClientImpl::run_game(InteractiveGameBase* igb, UI::ProgressWindow* loader) {
+	time.reset(game->get_gametime());
+	lasttimestamp = game->get_gametime();
+	lasttimestamp_realtime = SDL_GetTicks();
+
+	modal = igb;
+	game->run(loader, settings.savegame ?
+			 Widelands::Game::Loaded :
+			 settings.scenario ? Widelands::Game::NewMPScenario : Widelands::Game::NewNonScenario,
+			 "", false, (boost::format("netclient_%d") % static_cast<int>(settings.usernum)).str());
+
+	// if this is an internet game, tell the metaserver that the game is done.
+	if (internet_) {
+		InternetGaming::ref().set_game_done();
+	}
+	modal = nullptr;
+	game = nullptr;
+}
+
 GameClient::GameClient(const std::pair<NetAddress, NetAddress>& host,
                        const std::string& playername,
                        bool internet,
                        const std::string& gamename)
-   : d(new GameClientImpl), internet_(internet) {
+   : d(new GameClientImpl) {
+
+	d->internet_ = internet;
 
 	if (internet) {
 		assert(!gamename.empty());
@@ -127,7 +237,7 @@
 	d->game = nullptr;
 	d->realspeed = 0;
 	d->desiredspeed = 1000;
-	file_ = nullptr;
+	d->file_ = nullptr;
 
 	// Get the default win condition script
 	d->settings.win_condition_script = d->settings.win_condition_scripts.front();
@@ -141,94 +251,41 @@
 	delete d;
 }
 
+
+
 void GameClient::run() {
-	SendPacket s;
-	s.unsigned_8(NETCMD_HELLO);
-	s.unsigned_8(NETWORK_PROTOCOL_VERSION);
-	s.string(d->localplayername);
-	s.string(build_id());
-	d->net->send(s);
 
+	d->send_hello();
 	d->settings.multiplayer = true;
 
 	// Fill the list of possible system messages
 	NetworkGamingMessages::fill_map();
-	{
-		FullscreenMenuLaunchMPG lgm(this, this);
-		lgm.set_chat_provider(*this);
-		d->modal = &lgm;
-		FullscreenMenuBase::MenuTarget code = lgm.run<FullscreenMenuBase::MenuTarget>();
-		d->modal = nullptr;
-		if (code == FullscreenMenuBase::MenuTarget::kBack) {
-			// if this is an internet game, tell the metaserver that client is back in the lobby.
-			if (internet_)
-				InternetGaming::ref().set_game_done();
-			return;
-		}
+
+	if (d->run_map_menu(this)) {
+	    return; // did not select a Map ...
 	}
 
 	d->server_is_waiting = true;
 
+	bool write_sync_streams = g_options.pull_section("global").get_bool("write_syncstreams", true);
 	Widelands::Game game;
-	game.set_write_syncstream(g_options.pull_section("global").get_bool("write_syncstreams", true));
+	game.set_write_syncstream(write_sync_streams);
 
 	try {
 		std::unique_ptr<UI::ProgressWindow> loader_ui(new UI::ProgressWindow());
-		d->modal = loader_ui.get();
-		std::vector<std::string> tipstext;
-		tipstext.push_back("general_game");
-		tipstext.push_back("multiplayer");
-		try {
-			tipstext.push_back(get_players_tribe());
-		} catch (NoTribe) {
-		}
-		GameTips tips(*loader_ui.get(), tipstext);
-
-		loader_ui->step(_("Preparing game"));
 
 		d->game = &game;
-		game.set_game_controller(this);
-		uint8_t const pn = d->settings.playernum + 1;
-		game.save_handler().set_autosave_filename(
-		   (boost::format("%s_netclient%u") % kAutosavePrefix % static_cast<unsigned int>(pn)).str());
-		InteractiveGameBase* igb;
-		if (pn > 0)
-			igb = new InteractivePlayer(game, g_options.pull_section("global"), pn, true);
-		else
-			igb = new InteractiveSpectator(game, g_options.pull_section("global"), true);
-		game.set_ibase(igb);
-		igb->set_chat_provider(*this);
-		if (!d->settings.savegame) {  //  new map
-			game.init_newgame(loader_ui.get(), d->settings);
-		} else {  // savegame
-			game.init_savegame(loader_ui.get(), d->settings);
-		}
-		d->time.reset(game.get_gametime());
-		d->lasttimestamp = game.get_gametime();
-		d->lasttimestamp_realtime = SDL_GetTicks();
-
-		d->modal = igb;
-		game.run(
-		   loader_ui.get(),
-		   d->settings.savegame ?
-		      Widelands::Game::Loaded :
-		      d->settings.scenario ? Widelands::Game::NewMPScenario : Widelands::Game::NewNonScenario,
-		   "", false, (boost::format("netclient_%d") % static_cast<int>(d->settings.usernum)).str());
-
-		// if this is an internet game, tell the metaserver that the game is done.
-		if (internet_)
-			InternetGaming::ref().set_game_done();
-		d->modal = nullptr;
-		d->game = nullptr;
+		InteractiveGameBase* igb = d->init_game(this, loader_ui.get());
+		d->run_game(igb, loader_ui.get());
+
 	} catch (...) {
 		WLApplication::emergency_save(game);
 		d->game = nullptr;
 		disconnect("CLIENT_CRASHED");
 		// We will bounce back to the main menu, so we better log out
-		if (internet_) {
+		if (d->internet_) {
 			InternetGaming::ref().logout("CLIENT_CRASHED");
 		}
-		d->modal = nullptr;
 		throw;
 	}
 }
@@ -237,6 +294,7 @@
 	handle_network();
 
 	if (d->game) {
+		// TODO(Klaus Halfmann): what kind of time tricks are done here?
 		if (d->realspeed == 0 || d->server_is_waiting)
 			d->time.fastforward();
 		else
@@ -254,25 +312,28 @@
 	}
 }
 
-void GameClient::send_player_command(Widelands::PlayerCommand& pc) {
+/**
+ * Send PlayerCommand to server.
+ *
+ * @param pc will always be deleted in the end.
+ */
+void GameClient::send_player_command(Widelands::PlayerCommand* pc) {
 	assert(d->game);
-	if (pc.sender() != d->settings.playernum + 1) {
-		delete &pc;
-		return;
+
+	// TODDO(Klaus Halfmann)should this be an assert?
+	if (pc->sender() == d->settings.playernum + 1) //  allow command for current player only
+	{
+		log("[Client]: send playercommand at time %i\n", d->game->get_gametime());
+
+		d->send_player_command(pc);
+
+		d->lasttimestamp = d->game->get_gametime();
+		d->lasttimestamp_realtime = SDL_GetTicks();
+	} else {
+		log("[Client]: Playercommand is not for current player? %i\n", pc -> sender());
 	}
 
-	log("[Client]: send playercommand at time %i\n", d->game->get_gametime());
-
-	SendPacket s;
-	s.unsigned_8(NETCMD_PLAYERCOMMAND);
-	s.signed_32(d->game->get_gametime());
-	pc.serialize(s);
-	d->net->send(s);
-
-	d->lasttimestamp = d->game->get_gametime();
-	d->lasttimestamp_realtime = SDL_GetTicks();
-
-	delete &pc;
+	delete pc;
 }
 
 int32_t GameClient::get_frametime() {
@@ -544,6 +605,333 @@
 	}
 }
 
+void GameClient::handle_disconnect(RecvPacket& packet) {
+	uint8_t number = packet.unsigned_8();
+	std::string reason = packet.string();
+	if (number == 1)
+		disconnect(reason, "", false);
+	else {
+		std::string arg = packet.string();
+		disconnect(reason, arg, false);
+	}
+}
+
+/**
+ * Hello from the other side
+ */
+void GameClient::handle_hello(RecvPacket& packet) {
+	if (d->settings.usernum != -2) // TODO(Klaus Halfmann): if the host is the client ?.
+		throw ProtocolException(NETCMD_HELLO); // I am talkimg with myself? Bad idea
+	uint8_t const version = packet.unsigned_8();
+	if (version != NETWORK_PROTOCOL_VERSION)
+		throw DisconnectException("DIFFERENT_PROTOCOL_VERS");
+	d->settings.usernum = packet.unsigned_32(); // TODO(Klaus Halfmann): usernum is int8_t.
+	d->settings.playernum = -1;
+}
+
+/**
+ * Give a pong for a ping
+ */
+void GameClient::handle_ping(RecvPacket&) {
+	SendPacket s;
+	s.unsigned_8(NETCMD_PONG);
+	d->net->send(s);
+
+	log("[Client] Pong!\n");
+}
+
+/**
+ * New Map name was sent.
+ */
+void GameClient::handle_setting_map(RecvPacket& packet) {
+	d->settings.mapname = packet.string();
+	d->settings.mapfilename = g_fs->FileSystem::fix_cross_file(packet.string());
+	d->settings.savegame = packet.unsigned_8() == 1;
+	d->settings.scenario = packet.unsigned_8() == 1;
+	log("[Client] SETTING_MAP '%s' '%s'\n", d->settings.mapname.c_str(),
+		d->settings.mapfilename.c_str());
+
+	// New map was set, so we clean up the buffer of a previously requested file
+	d->file_.reset(nullptr);
+}
+
+/**
+ *
+ */
+// TODO(Klaus Halfmann): refactor this until it can be understood, move into impl.
+void GameClient::handle_new_file(RecvPacket& packet) {
+	std::string path = g_fs->FileSystem::fix_cross_file(packet.string());
+	uint32_t bytes = packet.unsigned_32();
+	std::string md5 = packet.string();
+
+	// Check whether the file or a file with that name already exists
+	if (g_fs->file_exists(path)) {
+		// If the file is a directory, we have to rename the file and replace it with the version
+		// of the host. If it is a zipped file, we can check, whether the host and the client have
+		// got the same file.
+		if (!g_fs->is_directory(path)) {
+			FileRead fr;
+			fr.open(*g_fs, path);
+			if (bytes == fr.get_size()) {
+				std::unique_ptr<char[]> complete(new char[bytes]);
+				if (!complete) {
+					throw wexception("Out of memory");
+				}
+				fr.data_complete(complete.get(), bytes);
+				// TODO(Klaus Halfmann): compute MD5 on the fly in FileRead...
+				SimpleMD5Checksum md5sum;
+				md5sum.data(complete.get(), bytes);
+				md5sum.finish_checksum();
+				std::string localmd5 = md5sum.get_checksum().str();
+				if (localmd5 == md5)
+					// everything is alright we now have the file.
+					return;
+			}
+		}
+		// Don't overwrite the file, better rename the original one
+		try {
+			g_fs->fs_rename(path, backup_file_name(path));
+		} catch (const FileError& e) {
+			log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
+				"%s\n",
+				e.what());
+			// TODO(Arty): What now? It just means the next step will fail
+			// or possibly result in some corrupt file
+		}
+	}
+
+	// Yes we need the file!
+	SendPacket s;
+	s.unsigned_8(NETCMD_NEW_FILE_AVAILABLE);
+	d->net->send(s);
+
+	d->file_.reset(new NetTransferFile());
+	d->file_->bytes = bytes;
+	d->file_->filename = path;
+	d->file_->md5sum = md5;
+	size_t position = path.rfind(g_fs->file_separator(), path.size() - 2);
+	if (position != std::string::npos) {
+		path.resize(position);
+		g_fs->ensure_directory_exists(path);
+	}
+}
+
+/**
+ *
+ */
+// TODO(Klaus Halfmann): refactor this until it can be understood, move into impl.
+void GameClient::handle_file_part(RecvPacket& packet) {
+	// Only go on, if we are waiting for a file part at the moment. It can happen, that an
+	// "unrequested" part is send by the server if the map was changed just a moment ago
+	// and there was an outstanding request from the client.
+	if (!d->file_)
+		return;  // silently ignore
+
+	uint32_t part = packet.unsigned_32();
+	uint32_t size = packet.unsigned_32();
+
+	// Send an answer
+	SendPacket s;
+	s.unsigned_8(NETCMD_FILE_PART);
+	s.unsigned_32(part);
+	s.string(d->file_->md5sum);
+	d->net->send(s);
+
+	FilePart fp;
+
+	char buf[NETFILEPARTSIZE];
+	assert(size <= NETFILEPARTSIZE);
+
+	// TODO(Klaus Halfmann): read directcly into FilePart?
+	if (packet.data(buf, size) != size)
+		log("Readproblem. Will try to go on anyways\n");
+	memcpy(fp.part, &buf[0], size);
+	d->file_->parts.push_back(fp);
+
+	// Write file to disk as soon as all parts arrived
+	uint32_t left = (d->file_->bytes - NETFILEPARTSIZE * part);
+	if (left <= NETFILEPARTSIZE) {
+		FileWrite fw;
+		left = d->file_->bytes;
+		uint32_t i = 0;
+		// Put all data together
+		while (left > 0) {
+			uint32_t writeout = (left > NETFILEPARTSIZE) ? NETFILEPARTSIZE : left;
+			fw.data(d->file_->parts[i].part, writeout, FileWrite::Pos::null());
+			left -= writeout;
+			++i;
+		}
+		// Now really write the file
+		fw.write(*g_fs, d->file_->filename.c_str());
+
+		// Check for consistence
+		FileRead fr;
+		fr.open(*g_fs, d->file_->filename);
+
+		std::unique_ptr<char[]> complete(new char[d->file_->bytes]);
+
+		fr.data_complete(complete.get(), d->file_->bytes);
+		SimpleMD5Checksum md5sum;
+		md5sum.data(complete.get(), d->file_->bytes);
+		md5sum.finish_checksum();
+		std::string localmd5 = md5sum.get_checksum().str();
+		if (localmd5 != d->file_->md5sum) {
+			// Something went wrong! We have to rerequest the file.
+			s.reset();
+			s.unsigned_8(NETCMD_NEW_FILE_AVAILABLE);
+			d->net->send(s);
+			// Notify the players
+			s.reset();
+			s.unsigned_8(NETCMD_CHAT);
+			s.string(_("/me 's file failed md5 checksumming."));
+			d->net->send(s);
+			try {
+				g_fs->fs_unlink(d->file_->filename);
+			} catch (const FileError& e) {
+				log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
+					"%s\n",
+					e.what());
+			}
+		}
+		// Check file for validity
+		bool invalid = false;
+		if (d->settings.savegame) {
+			// Saved game check - does Widelands recognize the file as saved game?
+			Widelands::Game game;
+			try {
+				Widelands::GameLoader gl(d->file_->filename, game);
+			} catch (...) {
+				invalid = true;
+			}
+		} else {
+			// Map check - does Widelands recognize the file as map?
+			Widelands::Map map;
+			std::unique_ptr<Widelands::MapLoader> ml = map.get_correct_loader(d->file_->filename);
+			if (!ml) {
+				invalid = true;
+			}
+		}
+		if (invalid) {
+			try {
+				g_fs->fs_unlink(d->file_->filename);
+				// Restore original file, if there was one before
+				if (g_fs->file_exists(backup_file_name(d->file_->filename)))
+					g_fs->fs_rename(backup_file_name(d->file_->filename), d->file_->filename);
+			} catch (const FileError& e) {
+				log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
+					"%s\n", e.what());
+			}
+			s.reset();
+			s.unsigned_8(NETCMD_CHAT);
+			s.string(_("/me checked the received file. Although md5 check summing succeeded, "
+					   "I can not handle the file."));
+			d->net->send(s);
+		}
+	}
+}
+
+/**
+ *
+ */
+void GameClient::handle_setting_tribes(RecvPacket& packet) {
+	d->settings.tribes.clear();
+	for (uint8_t i = packet.unsigned_8(); i; --i) {
+		Widelands::TribeBasicInfo info = Widelands::get_tribeinfo(packet.string());
+
+		// Get initializations (we have to do this locally, for translations)
+		LuaInterface lua;
+		info.initializations.clear();
+		for (uint8_t j = packet.unsigned_8(); j > 0; --j) {
+			std::string const initialization_script = packet.string();
+			std::unique_ptr<LuaTable> t = lua.run_script(initialization_script);
+			t->do_not_warn_about_unaccessed_keys();
+			info.initializations.push_back(Widelands::TribeBasicInfo::Initialization(
+				initialization_script, t->get_string("descname"), t->get_string("tooltip")));
+		}
+		d->settings.tribes.push_back(info);
+	}
+}
+
+/**
+ *
+ */
+void GameClient::handle_setting_allplayers(RecvPacket& packet) {
+	d->settings.players.resize(packet.unsigned_8());
+	for (uint8_t i = 0; i < d->settings.players.size(); ++i) {
+		receive_one_player(i, packet);
+	}
+	// Map changes are finished here
+	Notifications::publish(NoteGameSettings(NoteGameSettings::Action::kMap));
+}
+
+/**
+ *
+ */
+void GameClient::handle_playercommand(RecvPacket& packet) {
+	if (!d->game)
+		throw DisconnectException("PLAYERCMD_WO_GAME");
+
+	int32_t const time = packet.signed_32();
+	Widelands::PlayerCommand& plcmd = *Widelands::PlayerCommand::deserialize(packet);
+	plcmd.set_duetime(time);
+	d->game->enqueue_command(&plcmd);
+	d->time.receive(time);
+}
+
+/**
+ *
+ */
+void GameClient::handle_syncrequest(RecvPacket& packet) {
+	if (!d->game)
+		throw DisconnectException("SYNCREQUEST_WO_GAME");
+	int32_t const time = packet.signed_32();
+	d->time.receive(time);
+	d->game->enqueue_command(new CmdNetCheckSync(time, [this] { sync_report_callback(); }));
+	d->game->report_sync_request();
+}
+
+/**
+ *
+ */
+void GameClient::handle_chat(RecvPacket& packet) {
+	ChatMessage c("");
+	c.playern = packet.signed_16();
+	c.sender  = packet.string();
+	c.msg     = packet.string();
+	if (packet.unsigned_8())
+		c.recipient = packet.string();
+	d->chatmessages.push_back(c);
+	Notifications::publish(c);
+}
+
+/**
+ *
+ */
+void GameClient::handle_system_message(RecvPacket& packet) {
+	const std::string code = packet.string();
+	const std::string arg1 = packet.string();
+	const std::string arg2 = packet.string();
+	const std::string arg3 = packet.string();
+	ChatMessage c(NetworkGamingMessages::get_message(code, arg1, arg2, arg3));
+	c.playern = UserSettings::none();  //  == System message
+									   // c.sender remains empty to indicate a system message
+	d->chatmessages.push_back(c);
+	Notifications::publish(c);
+}
+
+/**
+ *
+ */
+void GameClient::handle_desync(RecvPacket&) {
+	log("[Client] received NETCMD_INFO_DESYNC. Trying to salvage some "
+		"information for debugging.\n");
+	if (d->game) {
+		d->game->save_syncstream(true);
+		// We don't know our playernumber, so report as -1
+		d->game->report_desync(-1);
+	}
+}
+
 /**
  * Handle one packet received from the host.
  *
@@ -552,363 +940,80 @@
 void GameClient::handle_packet(RecvPacket& packet) {
 	uint8_t cmd = packet.unsigned_8();
 
-	if (cmd == NETCMD_DISCONNECT) {
-		uint8_t number = packet.unsigned_8();
-		std::string reason = packet.string();
-		if (number == 1)
-			disconnect(reason, "", false);
-		else {
-			std::string arg = packet.string();
-			disconnect(reason, arg, false);
-		}
-		return;
-	}
-
-	if (d->settings.usernum == -2) {
-		if (cmd != NETCMD_HELLO)
+	switch (cmd) {
+		case NETCMD_DISCONNECT:
+			return handle_disconnect(packet);
+		case NETCMD_HELLO:
+			return handle_hello(packet);
+		case NETCMD_PING:
+			return handle_ping(packet);
+		case NETCMD_SETTING_MAP:
+			return handle_setting_map(packet);
+		case NETCMD_NEW_FILE_AVAILABLE:
+			return handle_new_file(packet);
+		case NETCMD_FILE_PART:
+			return handle_file_part(packet);
+		case NETCMD_SETTING_TRIBES:
+			return handle_setting_tribes(packet);
+		case NETCMD_SETTING_ALLPLAYERS:
+			return handle_setting_allplayers(packet);
+		case NETCMD_SETTING_PLAYER: {
+			uint8_t player = packet.unsigned_8();
+			receive_one_player(player, packet);
+		}
+			break;
+		case NETCMD_SETTING_ALLUSERS: {
+			d->settings.users.resize(packet.unsigned_8());
+			for (uint32_t i = 0; i < d->settings.users.size(); ++i)
+				receive_one_user(i, packet);
+		}
+			break;
+		case NETCMD_SETTING_USER: {
+			uint32_t user = packet.unsigned_32();
+			receive_one_user(user, packet);
+		}
+			break;
+		case NETCMD_SET_PLAYERNUMBER: {
+			int32_t number = packet.signed_32();
+			d->settings.playernum = number;
+			d->settings.users.at(d->settings.usernum).position = number;
+		}
+			break;
+		case NETCMD_WIN_CONDITION:
+			d->settings.win_condition_script = g_fs->FileSystem::fix_cross_file(packet.string());
+			break;
+		case NETCMD_PEACEFUL_MODE:
+			d->settings.peaceful = packet.unsigned_8();
+			break;
+		case NETCMD_LAUNCH:
+			if (!d->modal || d->game) {
+				throw DisconnectException("UNEXPECTED_LAUNCH");
+			}
+			d->modal->end_modal<FullscreenMenuBase::MenuTarget>(FullscreenMenuBase::MenuTarget::kOk);
+			break;
+		case NETCMD_SETSPEED:
+			d->realspeed = packet.unsigned_16();
+			log("[Client] speed: %u.%03u\n", d->realspeed / 1000, d->realspeed % 1000);
+			break;
+		case NETCMD_TIME:
+			d->time.receive(packet.signed_32());
+			break;
+		case NETCMD_WAIT:
+			log("[Client]: server is waiting.\n");
+			d->server_is_waiting = true;
+			break;
+		case NETCMD_PLAYERCOMMAND:
+			return handle_playercommand(packet);
+		case NETCMD_SYNCREQUEST:
+			return handle_syncrequest(packet);
+		case NETCMD_CHAT:
+			return handle_chat(packet);
+		case NETCMD_SYSTEM_MESSAGE_CODE:
+			return handle_system_message(packet);
+		case NETCMD_INFO_DESYNC:
+			return handle_desync(packet);
+		default:
 			throw ProtocolException(cmd);
-		uint8_t const version = packet.unsigned_8();
-		if (version != NETWORK_PROTOCOL_VERSION)
-			throw DisconnectException("DIFFERENT_PROTOCOL_VERS");
-		d->settings.usernum = packet.unsigned_32();
-		d->settings.playernum = -1;
-		return;
-	}
-
-	switch (cmd) {
-	case NETCMD_PING: {
-		SendPacket s;
-		s.unsigned_8(NETCMD_PONG);
-		d->net->send(s);
-
-		log("[Client] Pong!\n");
-		break;
-	}
-
-	case NETCMD_SETTING_MAP: {
-		d->settings.mapname = packet.string();
-		d->settings.mapfilename = g_fs->FileSystem::fix_cross_file(packet.string());
-		d->settings.savegame = packet.unsigned_8() == 1;
-		d->settings.scenario = packet.unsigned_8() == 1;
-		log("[Client] SETTING_MAP '%s' '%s'\n", d->settings.mapname.c_str(),
-		    d->settings.mapfilename.c_str());
-
-		// New map was set, so we clean up the buffer of a previously requested file
-		file_.reset(nullptr);
-		break;
-	}
-
-	case NETCMD_NEW_FILE_AVAILABLE: {
-		std::string path = g_fs->FileSystem::fix_cross_file(packet.string());
-		uint32_t bytes = packet.unsigned_32();
-		std::string md5 = packet.string();
-
-		// Check whether the file or a file with that name already exists
-		if (g_fs->file_exists(path)) {
-			// If the file is a directory, we have to rename the file and replace it with the version
-			// of the
-			// host. If it is a ziped file, we can check, whether the host and the client have got the
-			// same file.
-			if (!g_fs->is_directory(path)) {
-				FileRead fr;
-				fr.open(*g_fs, path);
-				if (bytes == fr.get_size()) {
-					std::unique_ptr<char[]> complete(new char[bytes]);
-					if (!complete)
-						throw wexception("Out of memory");
-
-					fr.data_complete(complete.get(), bytes);
-					SimpleMD5Checksum md5sum;
-					md5sum.data(complete.get(), bytes);
-					md5sum.finish_checksum();
-					std::string localmd5 = md5sum.get_checksum().str();
-					if (localmd5 == md5)
-						// everything is alright we already have the file.
-						return;
-				}
-			}
-			// Don't overwrite the file, better rename the original one
-			try {
-				g_fs->fs_rename(path, backup_file_name(path));
-			} catch (const FileError& e) {
-				log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
-				    "%s\n",
-				    e.what());
-				// TODO(Arty): What now? It just means the next step will fail
-				// or possibly result in some corrupt file
-			}
-		}
-
-		// Yes we need the file!
-		SendPacket s;
-		s.unsigned_8(NETCMD_NEW_FILE_AVAILABLE);
-		d->net->send(s);
-
-		file_.reset(new NetTransferFile());
-		file_->bytes = bytes;
-		file_->filename = path;
-		file_->md5sum = md5;
-		size_t position = path.rfind(g_fs->file_separator(), path.size() - 2);
-		if (position != std::string::npos) {
-			path.resize(position);
-			g_fs->ensure_directory_exists(path);
-		}
-		break;
-	}
-
-	case NETCMD_FILE_PART: {
-		// Only go on, if we are waiting for a file part at the moment. It can happen, that an
-		// "unrequested"
-		// part is send by the server if the map was changed just a moment ago and there was an
-		// outstanding
-		// request from the client.
-		if (!file_)
-			return;  // silently ignore
-
-		uint32_t part = packet.unsigned_32();
-		uint32_t size = packet.unsigned_32();
-
-		// Send an answer
-		SendPacket s;
-		s.unsigned_8(NETCMD_FILE_PART);
-		s.unsigned_32(part);
-		s.string(file_->md5sum);
-		d->net->send(s);
-
-		FilePart fp;
-
-		char buf[NETFILEPARTSIZE];
-		assert(size <= NETFILEPARTSIZE);
-
-		if (packet.data(buf, size) != size)
-			log("Readproblem. Will try to go on anyways\n");
-		memcpy(fp.part, &buf[0], size);
-		file_->parts.push_back(fp);
-
-		// Write file to disk as soon as all parts arrived
-		uint32_t left = (file_->bytes - NETFILEPARTSIZE * part);
-		if (left <= NETFILEPARTSIZE) {
-			FileWrite fw;
-			left = file_->bytes;
-			uint32_t i = 0;
-			// Put all data together
-			while (left > 0) {
-				uint32_t writeout = (left > NETFILEPARTSIZE) ? NETFILEPARTSIZE : left;
-				fw.data(file_->parts[i].part, writeout, FileWrite::Pos::null());
-				left -= writeout;
-				++i;
-			}
-			// Now really write the file
-			fw.write(*g_fs, file_->filename.c_str());
-
-			// Check for consistence
-			FileRead fr;
-			fr.open(*g_fs, file_->filename);
-
-			std::unique_ptr<char[]> complete(new char[file_->bytes]);
-
-			fr.data_complete(complete.get(), file_->bytes);
-			SimpleMD5Checksum md5sum;
-			md5sum.data(complete.get(), file_->bytes);
-			md5sum.finish_checksum();
-			std::string localmd5 = md5sum.get_checksum().str();
-			if (localmd5 != file_->md5sum) {
-				// Something went wrong! We have to rerequest the file.
-				s.reset();
-				s.unsigned_8(NETCMD_NEW_FILE_AVAILABLE);
-				d->net->send(s);
-				// Notify the players
-				s.reset();
-				s.unsigned_8(NETCMD_CHAT);
-				s.string(_("/me 's file failed md5 checksumming."));
-				d->net->send(s);
-				try {
-					g_fs->fs_unlink(file_->filename);
-				} catch (const FileError& e) {
-					log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
-					    "%s\n",
-					    e.what());
-				}
-			}
-			// Check file for validity
-			bool invalid = false;
-			if (d->settings.savegame) {
-				// Saved game check - does Widelands recognize the file as saved game?
-				Widelands::Game game;
-				try {
-					Widelands::GameLoader gl(file_->filename, game);
-				} catch (...) {
-					invalid = true;
-				}
-			} else {
-				// Map check - does Widelands recognize the file as map?
-				Widelands::Map map;
-				std::unique_ptr<Widelands::MapLoader> ml = map.get_correct_loader(file_->filename);
-				if (!ml)
-					invalid = true;
-			}
-			if (invalid) {
-				try {
-					g_fs->fs_unlink(file_->filename);
-					// Restore original file, if there was one before
-					if (g_fs->file_exists(backup_file_name(file_->filename)))
-						g_fs->fs_rename(backup_file_name(file_->filename), file_->filename);
-				} catch (const FileError& e) {
-					log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
-					    "%s\n",
-					    e.what());
-				}
-				s.reset();
-				s.unsigned_8(NETCMD_CHAT);
-				s.string(_("/me checked the received file. Although md5 check summing succeeded, "
-				           "I can not handle the file."));
-				d->net->send(s);
-			}
-		}
-		break;
-	}
-
-	case NETCMD_SETTING_TRIBES: {
-		d->settings.tribes.clear();
-		for (uint8_t i = packet.unsigned_8(); i; --i) {
-			Widelands::TribeBasicInfo info = Widelands::get_tribeinfo(packet.string());
-
-			// Get initializations (we have to do this locally, for translations)
-			LuaInterface lua;
-			info.initializations.clear();
-			for (uint8_t j = packet.unsigned_8(); j; --j) {
-				std::string const initialization_script = packet.string();
-				std::unique_ptr<LuaTable> t = lua.run_script(initialization_script);
-				t->do_not_warn_about_unaccessed_keys();
-				info.initializations.push_back(Widelands::TribeBasicInfo::Initialization(
-				   initialization_script, t->get_string("descname"), t->get_string("tooltip")));
-			}
-			d->settings.tribes.push_back(info);
-		}
-		break;
-	}
-
-	case NETCMD_SETTING_ALLPLAYERS: {
-		d->settings.players.resize(packet.unsigned_8());
-		for (uint8_t i = 0; i < d->settings.players.size(); ++i) {
-			receive_one_player(i, packet);
-		}
-		// Map changes are finished here
-		Notifications::publish(NoteGameSettings(NoteGameSettings::Action::kMap));
-		break;
-	}
-	case NETCMD_SETTING_PLAYER: {
-		uint8_t player = packet.unsigned_8();
-		receive_one_player(player, packet);
-		break;
-	}
-	case NETCMD_SETTING_ALLUSERS: {
-		d->settings.users.resize(packet.unsigned_8());
-		for (uint32_t i = 0; i < d->settings.users.size(); ++i)
-			receive_one_user(i, packet);
-		break;
-	}
-	case NETCMD_SETTING_USER: {
-		uint32_t user = packet.unsigned_32();
-		receive_one_user(user, packet);
-		break;
-	}
-	case NETCMD_SET_PLAYERNUMBER: {
-		int32_t number = packet.signed_32();
-		d->settings.playernum = number;
-		d->settings.users.at(d->settings.usernum).position = number;
-		break;
-	}
-	case NETCMD_WIN_CONDITION: {
-		d->settings.win_condition_script = g_fs->FileSystem::fix_cross_file(packet.string());
-		break;
-	}
-	case NETCMD_PEACEFUL_MODE: {
-		d->settings.peaceful = packet.unsigned_8();
-		break;
-	}
-
-	case NETCMD_LAUNCH: {
-		if (!d->modal || d->game) {
-			throw DisconnectException("UNEXPECTED_LAUNCH");
-		}
-		d->modal->end_modal<FullscreenMenuBase::MenuTarget>(FullscreenMenuBase::MenuTarget::kOk);
-		break;
-	}
-
-	case NETCMD_SETSPEED:
-		d->realspeed = packet.unsigned_16();
-		log("[Client] speed: %u.%03u\n", d->realspeed / 1000, d->realspeed % 1000);
-		break;
-
-	case NETCMD_TIME:
-		d->time.receive(packet.signed_32());
-		break;
-
-	case NETCMD_WAIT:
-		log("[Client]: server is waiting.\n");
-		d->server_is_waiting = true;
-		break;
-
-	case NETCMD_PLAYERCOMMAND: {
-		if (!d->game)
-			throw DisconnectException("PLAYERCMD_WO_GAME");
-
-		int32_t const time = packet.signed_32();
-		Widelands::PlayerCommand& plcmd = *Widelands::PlayerCommand::deserialize(packet);
-		plcmd.set_duetime(time);
-		d->game->enqueue_command(&plcmd);
-		d->time.receive(time);
-		break;
-	}
-
-	case NETCMD_SYNCREQUEST: {
-		if (!d->game)
-			throw DisconnectException("SYNCREQUEST_WO_GAME");
-		int32_t const time = packet.signed_32();
-		d->time.receive(time);
-		d->game->enqueue_command(new CmdNetCheckSync(time, [this] { sync_report_callback(); }));
-		d->game->report_sync_request();
-		break;
-	}
-
-	case NETCMD_CHAT: {
-		ChatMessage c("");
-		c.playern = packet.signed_16();
-		c.sender = packet.string();
-		c.msg = packet.string();
-		if (packet.unsigned_8())
-			c.recipient = packet.string();
-		d->chatmessages.push_back(c);
-		Notifications::publish(c);
-		break;
-	}
-
-	case NETCMD_SYSTEM_MESSAGE_CODE: {
-		const std::string code = packet.string();
-		const std::string arg1 = packet.string();
-		const std::string arg2 = packet.string();
-		const std::string arg3 = packet.string();
-		ChatMessage c(NetworkGamingMessages::get_message(code, arg1, arg2, arg3));
-		c.playern = UserSettings::none();  //  == System message
-		// c.sender remains empty to indicate a system message
-		d->chatmessages.push_back(c);
-		Notifications::publish(c);
-		break;
-	}
-
-	case NETCMD_INFO_DESYNC:
-		log("[Client] received NETCMD_INFO_DESYNC. Trying to salvage some "
-		    "information for debugging.\n");
-		if (d->game) {
-			d->game->save_syncstream(true);
-			// We don't know our playernumber, so report as -1
-			d->game->report_desync(-1);
-		}
-		break;
-
-	default:
-		throw ProtocolException(cmd);
 	}
 }
 
@@ -917,7 +1022,7 @@
  */
 void GameClient::handle_network() {
 	// if this is an internet game, handle the metaserver network
-	if (internet_)
+	if (d->internet_)
 		InternetGaming::ref().handle_metaserver_communication();
 	try {
 		assert(d->net != nullptr);
@@ -965,7 +1070,7 @@
 
 	bool const trysave = showmsg && d->game;
 
-	if (showmsg) {
+	if (showmsg && d->modal) { // can only show a message with a valid modal parent window
 		std::string msg;
 		if (arg.empty())
 			msg = NetworkGamingMessages::get_message(reason);
@@ -985,8 +1090,9 @@
 	if (trysave)
 		WLApplication::emergency_save(*d->game);
 
+	// TODO(Klaus Halfmann): Some of the modal windows are now handled by unique_ptr resulting in a double free.
 	if (d->modal) {
 		d->modal->end_modal<FullscreenMenuBase::MenuTarget>(FullscreenMenuBase::MenuTarget::kBack);
-		d->modal = nullptr;
 	}
+	d->modal = nullptr;
 }
 
=== modified file 'src/network/gameclient.h'
--- src/network/gameclient.h	2019-04-29 16:22:08 +0000
+++ src/network/gameclient.h	2019-05-25 17:10:52 +0000
@@ -29,14 +29,16 @@
 #include "network/netclient_interface.h"
 
 struct GameClientImpl;
+class InteractiveGameBase;
 
-// TODO(unknown): Use composition instead of inheritance
 /**
  * GameClient manages the lifetime of a network game in which this computer
  * participates as a client.
  *
  * This includes running the game setup screen and the actual game after
  * launch, as well as dealing with the actual network protocol.
+ *
+ * @param internet TODO(Klaus Halfmann): true: coonnect into the open internet via proxy, false connect locally / via IP.
  */
 struct GameClient : public GameController, public GameSettingsProvider, public ChatProvider {
 	GameClient(const std::pair<NetAddress, NetAddress>& host,
@@ -50,7 +52,7 @@
 
 	// GameController interface
 	void think() override;
-	void send_player_command(Widelands::PlayerCommand&) override;
+	void send_player_command(Widelands::PlayerCommand*) override;
 	int32_t get_frametime() override;
 	GameController::GameType get_game_type() override;
 
@@ -115,7 +117,21 @@
 
 	void sync_report_callback();
 
-	void handle_packet(RecvPacket&);
+	void handle_hello(RecvPacket& packet);
+	void handle_disconnect(RecvPacket& packet);
+	void handle_ping(RecvPacket& packet);
+	void handle_new_file(RecvPacket& packet);
+	void handle_syncrequest(RecvPacket& packet);
+	void handle_setting_map(RecvPacket& packet);
+	void handle_file_part(RecvPacket& packet);
+	void handle_setting_tribes(RecvPacket& packet);
+	void handle_setting_allplayers(RecvPacket& packet);
+	void handle_playercommand(RecvPacket& packet);
+	void handle_chat(RecvPacket& packet);
+	void handle_system_message(RecvPacket& packet);
+	void handle_desync(RecvPacket& packet);
+	void handle_packet(RecvPacket& packet);
+
 	void handle_network();
 	void send_time();
 	void receive_one_player(uint8_t number, StreamRead&);
@@ -125,9 +141,9 @@
 	                bool sendreason = true,
 	                bool showmsg = true);
 
-	std::unique_ptr<NetTransferFile> file_;
+
 	GameClientImpl* d;
-	bool internet_;
+
 };
 
 #endif  // end of include guard: WL_NETWORK_GAMECLIENT_H
 
=== modified file 'src/network/gamehost.cc'
--- src/network/gamehost.cc	2019-05-11 22:55:40 +0000
+++ src/network/gamehost.cc	2019-05-25 17:10:52 +0000
@@ -761,15 +761,15 @@
 	}
 }
 
-void GameHost::send_player_command(Widelands::PlayerCommand& pc) {
-	pc.set_duetime(d->committed_networktime + 1);
+void GameHost::send_player_command(Widelands::PlayerCommand* pc) {
+	pc->set_duetime(d->committed_networktime + 1);
 
 	SendPacket packet;
 	packet.unsigned_8(NETCMD_PLAYERCOMMAND);
-	packet.signed_32(pc.duetime());
-	pc.serialize(packet);
+	packet.signed_32(pc->duetime());
+	pc-> serialize(packet);
 	broadcast(packet);
-	d->game->enqueue_command(&pc);
+	d->game->enqueue_command(pc);
 
 	committed_network_time(d->committed_networktime + 1);
 }
@@ -2181,11 +2181,11 @@
 		if (!d->game)
 			throw DisconnectException("PLAYERCMD_WO_GAME");
 		int32_t time = r.signed_32();
-		Widelands::PlayerCommand& plcmd = *Widelands::PlayerCommand::deserialize(r);
+		Widelands::PlayerCommand* plcmd = Widelands::PlayerCommand::deserialize(r);
 		log("[Host]: Client %u (%u) sent player command %u for %u, time = %i\n", i, client.playernum,
-		    static_cast<unsigned int>(plcmd.id()), plcmd.sender(), time);
+		    static_cast<unsigned int>(plcmd->id()), plcmd->sender(), time);
 		receive_client_time(i, time);
-		if (plcmd.sender() != client.playernum + 1)
+		if (plcmd->sender() != client.playernum + 1)
 			throw DisconnectException("PLAYERCMD_FOR_OTHER");
 		send_player_command(plcmd);
 	} break;
 
=== modified file 'src/network/gamehost.h'
--- src/network/gamehost.h	2019-05-04 10:47:44 +0000
+++ src/network/gamehost.h	2019-05-25 17:10:52 +0000
@@ -50,7 +50,7 @@
 
 	// GameController interface
 	void think() override;
-	void send_player_command(Widelands::PlayerCommand&) override;
+	void send_player_command(Widelands::PlayerCommand*) override;
 	int32_t get_frametime() override;
 	GameController::GameType get_game_type() override;
 
 
=== modified file 'src/network/netclient_interface.h'
--- src/network/netclient_interface.h	2019-02-23 11:00:49 +0000
+++ src/network/netclient_interface.h	2019-05-25 17:10:52 +0000
@@ -27,6 +27,7 @@
 /**
  * NetClient manages the network connection for a network game in which this computer
  * participates as a client.
+ *
  * This class provides the interface all NetClient implementation have to follow.
  * Currently two implementations exists: A "real" NetClient for local games and a
  * NetClientProxy which relays commands over a relay server.
@@ -42,18 +43,21 @@
 
 	/**
 	 * Returns whether the client is connected.
+	 *
 	 * \return \c true if the connection is open, \c false otherwise.
 	 */
 	virtual bool is_connected() const = 0;
 
 	/**
 	 * Closes the connection.
+	 *
 	 * If you want to send a goodbye-message to the host, do so before calling this.
 	 */
 	virtual void close() = 0;
 
 	/**
 	 * Tries to receive a packet.
+	 *
 	 * \return A pointer to a packet if one packet is available, an invalid pointer otherwise.
 	 *   Calling this on a closed connection will return an invalid pointer.
 	 */
@@ -61,6 +65,7 @@
 
 	/**
 	 * Sends a packet.
+	 *
 	 * Calling this on a closed connection will silently fail.
 	 * \param packet The packet to send.
 	 */
 
=== modified file 'src/network/nethostproxy.cc'
--- src/network/nethostproxy.cc	2018-03-03 10:48:14 +0000
+++ src/network/nethostproxy.cc	2019-05-25 17:10:52 +0000
@@ -256,7 +256,7 @@
 			conn_->receive(&cmd);
 			uint8_t id;
 			conn_->receive(&id);
-			assert(clients_.count(id));
+			assert(clients_.count(id)); // TODO(Klaus Halfmann): As of a race condition this may not always hold.
 			clients_.at(id).state_ = Client::State::kDisconnected;
 		}
 		break;
 
=== modified file 'src/wui/economy_options_window.cc'
--- src/wui/economy_options_window.cc	2019-02-23 11:00:49 +0000
+++ src/wui/economy_options_window.cc	2019-05-25 17:10:52 +0000
@@ -189,11 +189,11 @@
 			// Don't allow negative new amount.
 			if (amount >= 0 || -amount <= static_cast<int>(tq.permanent)) {
 				if (is_wares) {
-					game.send_player_command(*new Widelands::CmdSetWareTargetQuantity(
+					game.send_player_command(new Widelands::CmdSetWareTargetQuantity(
 					   game.get_gametime(), player_->player_number(), serial_, index,
 					   tq.permanent + amount));
 				} else {
-					game.send_player_command(*new Widelands::CmdSetWorkerTargetQuantity(
+					game.send_player_command(new Widelands::CmdSetWorkerTargetQuantity(
 					   game.get_gametime(), player_->player_number(), serial_, index,
 					   tq.permanent + amount));
 				}
@@ -209,10 +209,10 @@
 	for (const Widelands::DescriptionIndex& index : items) {
 		if (display_.ware_selected(index)) {
 			if (is_wares) {
-				game.send_player_command(*new Widelands::CmdResetWareTargetQuantity(
+				game.send_player_command(new Widelands::CmdResetWareTargetQuantity(
 				   game.get_gametime(), player_->player_number(), serial_, index));
 			} else {
-				game.send_player_command(*new Widelands::CmdResetWorkerTargetQuantity(
+				game.send_player_command(new Widelands::CmdResetWorkerTargetQuantity(
 				   game.get_gametime(), player_->player_number(), serial_, index));
 			}
 		}
 
=== modified file 'src/wui/game_message_menu.cc'
--- src/wui/game_message_menu.cc	2019-04-18 16:50:35 +0000
+++ src/wui/game_message_menu.cc	2019-05-25 17:10:52 +0000
@@ -336,7 +336,7 @@
 			//  Maybe the message was removed since think?
 			if (message->status() == Message::Status::kNew) {
 				Widelands::Game& game = iplayer().game();
-				game.send_player_command(*new Widelands::CmdMessageSetStatusRead(
+				game.send_player_command(new Widelands::CmdMessageSetStatusRead(
 				   game.get_gametime(), player.player_number(), id));
 			}
 			centerviewbtn_->set_enabled(message->position());
@@ -439,12 +439,12 @@
 		switch (mode) {
 		case Inbox:
 			// Archive highlighted message
-			game.send_player_command(*new Widelands::CmdMessageSetStatusArchived(
+			game.send_player_command(new Widelands::CmdMessageSetStatusArchived(
 			   game.get_gametime(), plnum, MessageId(selected_record)));
 			break;
 		case Archive:
 			// Restore highlighted message
-			game.send_player_command(*new Widelands::CmdMessageSetStatusRead(
+			game.send_player_command(new Widelands::CmdMessageSetStatusRead(
 			   game.get_gametime(), plnum, MessageId(selected_record)));
 			break;
 		}
 
=== modified file 'src/wui/warehousewindow.cc'
--- src/wui/warehousewindow.cc	2019-02-23 11:00:49 +0000
+++ src/wui/warehousewindow.cc	2019-05-25 17:10:52 +0000
@@ -160,7 +160,7 @@
 
 		for (const Widelands::DescriptionIndex& index : indices) {
 			if (display_.ware_selected(index)) {
-				gb_.game().send_player_command(*new Widelands::CmdSetStockPolicy(
+				gb_.game().send_player_command(new Widelands::CmdSetStockPolicy(
 				   gb_.game().get_gametime(), wh_.owner().player_number(), wh_, is_workers, index,
 				   newpolicy));
 			}