Clamp ally-team widget index in the map editor's teams editor

src/AllyTeamWidgetIndex.h

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+#pragma once
+
+#include <SDL_stdinc.h>
+
+/// Ally team numbers in GameHeader are 1-based (its constructor assigns
+/// team i the value i+1), while the ally-team widget rows are 0-based and
+/// only teamCount of them exist (one per team in the MapHeader).
+/// A malformed or uninitialized header can carry values outside
+/// [1, teamCount] -- notably 0, which would underflow to setIndex(-1),
+/// or a value above teamCount -- and either throws inside
+/// MultiTextButton::setIndex. Map any such value to a defined widget
+/// state: the first entry (index 0). For well-formed values this is
+/// exactly allyTeamNumber - 1; the inverse (widget index + 1) is applied
+/// when the widgets are written back to the header.
+inline int allyTeamNumberToWidgetIndex(Uint8 allyTeamNumber, int teamCount)
+{
+	const int index = static_cast<int>(allyTeamNumber) - 1;
+	if (index < 0 || index >= teamCount)
+		return 0;
+	return index;
+}

src/CustomGameOtherOptions.cpp

@@ -2,6 +2,7 @@
 // Copyright (C) 2008 Bradley Arsenault
 
 #include "CustomGameOtherOptions.h"
+#include "AllyTeamWidgetIndex.h"
 
 #include <Toolkit.h>
 #include <StringTable.h>
@@ -10,23 +11,6 @@
 #include <optional>
 #include <sstream>
 
-namespace
-{
-	/// Ally team numbers in GameHeader are 1-based (its constructor assigns
-	/// team i the value i+1), while the ally-team widget rows are 0-based.
-	/// A malformed or uninitialized header can carry values outside
-	/// [1, teamCount] -- notably 0, which would underflow to setIndex(-1)
-	/// and throw inside MultiTextButton::setIndex. Map any such value to a
-	/// defined widget state: the first entry (index 0).
-	int allyTeamNumberToWidgetIndex(Uint8 allyTeamNumber, int teamCount)
-	{
-		const int index = static_cast<int>(allyTeamNumber) - 1;
-		if (index < 0 || index >= teamCount)
-			return 0;
-		return index;
-	}
-}
-
 CustomGameOtherOptions::CustomGameOtherOptions(GameHeader& gameHeader, MapHeader& mapHeader, bool readOnly)
 	:	gameHeader(gameHeader), oldGameHeader(gameHeader)
 {

src/map/edit/MapEditDialog.cpp

@@ -5,6 +5,7 @@
 #include "FormatableString.h"
 #include "Game.h"
 #include "AINames.h"
+#include "AllyTeamWidgetIndex.h"
 #include "GameHeader.h"
 #include "GlobalContainer.h"
 #include "GUIButton.h"
@@ -87,14 +88,14 @@ TeamsEditor::TeamsEditor(Game* game)
 
 	for(int i=0; i<Team::MAX_COUNT; ++i)
 	{
-		isPlayerActive[i] = new OnOffButton(10, 60+i*25, 21, 21, ALIGN_LEFT, ALIGN_TOP, gameHeader.getBasePlayer(i).type != BasePlayer::P_NONE, 100+i);
+		isPlayerActive[i] = new OnOffButton(10, 60+i*25, 21, 21, ALIGN_LEFT, ALIGN_TOP, gameHeader.getBasePlayer(i).type != BasePlayer::P_NONE, PLAYER_ACTIVE_BASE+i);
 		addWidget(isPlayerActive[i]);
 		if(i==0)
 		{
 			isPlayerActive[i]->visible=false;
 		}
 
-		color[i] = new ColorButton(35, 60+25*i, 21, 21, ALIGN_LEFT, ALIGN_TOP, 200+i);
+		color[i] = new ColorButton(35, 60+25*i, 21, 21, ALIGN_LEFT, ALIGN_TOP, COLOR_BASE+i);
 		for (int j = 0; j<mapHeader.getNumberOfTeams(); j++)
 			color[i]->addColor(mapHeader.getBaseTeam(j).color);
 		color[i]->setSelectedColor(gameHeader.getBasePlayer(i).teamNumber);
@@ -109,7 +110,7 @@ TeamsEditor::TeamsEditor(Game* game)
 		else
 		{
 			playerName[i]=NULL;
-			aiSelector[i]=new MultiTextButton(60, 60+i*25, 100, 21, ALIGN_LEFT, ALIGN_TOP, "standard", Toolkit::getStringTable()->getString("[AI]"), 300+i);
+			aiSelector[i]=new MultiTextButton(60, 60+i*25, 100, 21, ALIGN_LEFT, ALIGN_TOP, "standard", Toolkit::getStringTable()->getString("[AI]"), AI_SELECTOR_BASE+i);
 			for (int aii=0; aii<AI::SIZE; aii++)
 				aiSelector[i]->addText(AINames::getAIText(aii));
 			if(gameHeader.getBasePlayer(i).type >= BasePlayer::P_AI)
@@ -119,15 +120,17 @@ TeamsEditor::TeamsEditor(Game* game)
 			addWidget(aiSelector[i]);
 		}
 
-		allyTeamNumbers[i] = new MultiTextButton(185, 60+25*i, 21, 21, ALIGN_LEFT, ALIGN_TOP, "standard", "", 400+i);
+		allyTeamNumbers[i] = new MultiTextButton(185, 60+25*i, 21, 21, ALIGN_LEFT, ALIGN_TOP, "standard", "", ALLY_TEAM_BASE+i);
 		allyTeamNumbers[i]->clearTexts();
 		for(int j=0; j<mapHeader.getNumberOfTeams(); ++j)
 		{
 			std::stringstream s;
 			s<<j+1;
 			allyTeamNumbers[i]->addText(s.str());
 		}
-		allyTeamNumbers[i]->setIndex(gameHeader.getAllyTeamNumber(gameHeader.getBasePlayer(i).teamNumber)-1);
+		allyTeamNumbers[i]->setIndex(allyTeamNumberToWidgetIndex(
+			gameHeader.getAllyTeamNumber(gameHeader.getBasePlayer(i).teamNumber),
+			mapHeader.getNumberOfTeams()));
 		addWidget(allyTeamNumbers[i]);
 
 
@@ -159,19 +162,19 @@ void TeamsEditor::onAction(Widget *source, Action action, int par1, int par2)
 		{
 			endValue=CANCEL;
 		}
-		else if(par1>=100 && par1<200)
+		else if(par1>=PLAYER_ACTIVE_BASE && par1<COLOR_BASE)
 		{
-			int n = par1-100;
+			int n = par1-PLAYER_ACTIVE_BASE;
 			color[n]->visible=isPlayerActive[n]->getState();
 			aiSelector[n]->visible=isPlayerActive[n]->getState();
 			allyTeamNumbers[n]->visible=isPlayerActive[n]->getState();
 		}
 	}
 	if(action==BUTTON_PRESSED || action==BUTTON_SHORTCUT)
 	{
-		if(par1>=200 && par1<300)
+		if(par1>=COLOR_BASE && par1<AI_SELECTOR_BASE)
 		{
-			int n = par1-200;
+			int n = par1-COLOR_BASE;
 			for(int i=0; i<Team::MAX_COUNT; ++i)
 			{
 				if(color[i]->getSelectedColor() == color[n]->getSelectedColor() && i!=n)
@@ -181,7 +184,7 @@ void TeamsEditor::onAction(Widget *source, Action action, int par1, int par2)
 			}
 		}
 
-		if(par1>=400)
+		if(par1>=ALLY_TEAM_BASE)
 		{
 			GameHeader& gameHeader = game->gameHeader;
 			int team = -1;

src/map/edit/MapEditDialog.h

@@ -79,13 +79,25 @@ class TeamsEditor : public OverlayScreen
 	TeamsEditor(Game* game);
 	virtual ~TeamsEditor() { }
 	void onAction(Widget *source, Action action, int par1, int par2);
+	///Rebuilds the game's GameHeader from scratch out of the widget state
+	///(it does not mutate the existing header). Active slots are packed
+	///into consecutive player numbers; each slot's ally-team widget index
+	///(0-based) is written back as ally team number widgetIndex + 1, the
+	///inverse of allyTeamNumberToWidgetIndex().
 	void generateGameHeader();
-	
+
 	enum
 	{
 		OK,
 		CANCEL
 	};
+
+	///Widget return codes are base + slot index, one base per widget row
+	///kind, so onAction can recover the slot from the code.
+	static constexpr int PLAYER_ACTIVE_BASE = 100;
+	static constexpr int COLOR_BASE = 200;
+	static constexpr int AI_SELECTOR_BASE = 300;
+	static constexpr int ALLY_TEAM_BASE = 400;
 private:
 	Game* game;
 

tests/ally_team_widget_index_test.cpp

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+//
+// Standalone check for allyTeamNumberToWidgetIndex (src/AllyTeamWidgetIndex.h).
+// Not part of the SCons build. Compile and run by hand from glob2/:
+//
+//   g++ -std=c++17 $(sdl2-config --cflags) -Isrc tests/ally_team_widget_index_test.cpp -o .tmp/ally_test && .tmp/ally_test
+
+#include "AllyTeamWidgetIndex.h"
+
+#include <cstdio>
+#include <cstdlib>
+
+namespace
+{
+	int failures = 0;
+
+	void expect(Uint8 allyTeamNumber, int teamCount, int want)
+	{
+		const int got = allyTeamNumberToWidgetIndex(allyTeamNumber, teamCount);
+		if (got != want)
+		{
+			std::printf("FAIL: (%d, %d) -> %d, want %d\n", allyTeamNumber, teamCount, got, want);
+			++failures;
+		}
+	}
+}
+
+int main()
+{
+	// Well-formed 1-based values map to value - 1.
+	expect(1, 4, 0);
+	expect(2, 4, 1);
+	expect(3, 4, 2);
+	expect(4, 4, 3);
+	expect(1, 1, 0);
+	// 0 (uninitialized header) clamps to the first row instead of underflowing.
+	expect(0, 4, 0);
+	// Values above the populated row count clamp instead of throwing.
+	expect(5, 4, 0);
+	expect(255, 4, 0);
+	expect(2, 1, 0);
+
+	if (failures)
+	{
+		std::printf("%d failure(s)\n", failures);
+		return EXIT_FAILURE;
+	}
+	std::printf("all ally-team widget index cases passed\n");
+	return EXIT_SUCCESS;
+}