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

[Jens Beyer]

Fix the crash, and by the way, replace the 99 as loop boundary by the MAX_PLAYERS define used everywhere else.

For me it works... but as usual, I'm not a C++ programmer yet ^^ so please check well.

[Jens Beyer (qcumber-some)]

On a second look, maybe I was a bit too eager on the MAX_PLAYERS thing, and I could take that part back, but on the other hand, it still looks like it would never happen to be something over MAX_PLAYERS... well, you decide :-)

I tested again and don't see any issues.

I found a new issue (when 8 players are selected, the "up" button is disabled, but when you press "down", the "up" button is not enabled again). In current trunk this is not testable, as it crashes after selecting "8". If you want, I can open a new bug for that.

[GunChleoc (gunchleoc)]

The assert(nr_players <= 99) isn't a loop boundary, it is there to ensure that a condition is met. In debug mode, if the condition isn't met, Widelands will terminate with a message. The reason for assertions is to define certain wanted behaviour and thus help guard against regression bugs.

In this case, the assertion is there because there is some string assembly going on directly below it that relies on the number not being larger than 2 digits. Your code change is not wrong at the moment, because MAX_PLAYERS is smaller than 99, but the 99 is semantically more clear. E.g. imagine we would allow more than 99 players some day, then the string assembly code would need changing - this is is signalled by the assertion.

Good catch about the button, I have fixed it.

review: Approve

=== modified file 'src/editor/ui_menus/editor_player_menu.cc'
--- src/editor/ui_menus/editor_player_menu.cc	2015-09-08 07:16:17 +0000
+++ src/editor/ui_menus/editor_player_menu.cc	2015-10-11 07:56:12 +0000
@@ -176,12 +176,12 @@
 
 		// Get/Set (localized) tribe names
 		if (map.get_scenario_player_tribe(p) != UNDEFINED_TRIBE_NAME) {
-			m_selected_tribes[p] = map.get_scenario_player_tribe(p);
+			m_selected_tribes[p - 1] = map.get_scenario_player_tribe(p);
 		} else {
-			m_selected_tribes[p] = m_tribenames[0];
-			map.set_scenario_player_tribe(p, m_selected_tribes[p]);
+			m_selected_tribes[p - 1] = m_tribenames[0];
+			map.set_scenario_player_tribe(p, m_selected_tribes[p - 1]);
 		}
-		m_plr_set_tribes_buts[p - 1]->set_title(m_tribe_descnames.find(m_selected_tribes[p])->second);
+		m_plr_set_tribes_buts[p - 1]->set_title(m_tribe_descnames.find(m_selected_tribes[p - 1])->second);
 
 		// Set default AI and closeable to false (always default - should be changed by hand)
 		map.set_scenario_player_ai(p, "");
@@ -248,6 +248,7 @@
 			set_starting_pos_clicked(nr_players);
 	}
 	map.set_nrplayers(nr_players);
+	m_add_player        .set_enabled(nr_players < MAX_PLAYERS);
 	m_remove_last_player.set_enabled(1 < nr_players);
 
 	update();