Skip to content

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

Merged
kylelutze merged 2 commits into
masterfrom
fix/teams-editor-ally-index
Sep 7, 2026
Merged

Clamp ally-team widget index in the map editor's teams editor#175
kylelutze merged 2 commits into
masterfrom
fix/teams-editor-ally-index

Conversation

@kylelutze

@kylelutze kylelutze commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The map editor's teams editor converted the header's 1-based ally-team number to a 0-based widget index by bare subtraction. A header with 0, or a value above the map's team count, gave an out-of-range index; MultiTextButton::setIndex uses vector::at, so the dialog threw and crashed on open.
  • Promotes the clamp helper already used by the custom-game options screen into a shared header-only AllyTeamWidgetIndex.h and uses it at both sites. Well-formed values still map to value - 1; only headers that previously threw now display ally team 1. Nothing written back changes.
  • Names the teams editor's widget return-code bases (PLAYER_ACTIVE_BASE, COLOR_BASE, AI_SELECTOR_BASE, ALLY_TEAM_BASE) instead of hand-typed 100/200/300/400, and documents generateGameHeader's inverse mapping.

Verification

  • scons -j16 clean.
  • tests/ally_team_widget_index_test.cpp (standalone, compile line in its header) passes all cases: 0, above-range, and 255 clamp to 0; 1..teamCount map to 0..teamCount-1.
  • Workflow 1 replay check: cpp-refactor.replay byte-identical (editor UI only, no sim path touched).

Manual check

Editor → open teams editor. Before: a map whose game header carries an ally-team number outside [1, numberOfTeams] crashes on dialog open. After: that row shows ally team 1. Well-formed maps look identical.

The teams editor converted the header's 1-based ally-team number to a
0-based widget index by bare subtraction. A header carrying 0, or a
value above the map's team count, produced an index outside the rows
the widget was populated with; MultiTextButton::setIndex stores the
index unsigned and looks it up with vector::at, so the dialog threw
std::out_of_range and crashed on open.

The custom-game options screen had already fixed the same defect with
a file-local clamp helper. Promote that helper to a shared header-only
AllyTeamWidgetIndex.h and use it at both sites. Well-formed values
still map to value - 1, so nothing written back changes; only headers
that previously threw now show ally team 1.

Also name the teams editor's widget return-code bases (player-active,
color, AI selector, ally team) instead of hand-typing 100/200/300/400
at nine sites, and document generateGameHeader's inverse mapping.

Verified: standalone driver tests/ally_team_widget_index_test.cpp
passes; cpp-refactor.replay byte-identical after the change.
@kylelutze
kylelutze requested a review from a team September 6, 2026 22:22

@Giszmo Giszmo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head 4214b84, tested merged onto master bfc8fd7. Approve, no blockers.

Verified

  • The crash is real: MultiTextButton::setIndex uses texts.at() and the ally widget only has numberOfTeams entries, so an ally number of 0 or above the team count threw on dialog open. The clamp is the same helper already used in CustomGameOtherOptions (4f13a60); well-formed values and the index + 1 write-back in generateGameHeader are unchanged.
  • The named return-code constants keep exactly the old 100/200/300/400 ranges; onAction logic is untouched.
  • scons -j16 on master + PR builds clean with no new warnings in the touched files. tests/ally_team_widget_index_test.cpp compiles and passes with the command in its header. The branch is a few commits behind master but merges clean.

Non-blocking notes

  1. Reachability: the editor's own add/remove team already rebuilds the header from scratch (MapEdit::regenerateGameHeader, src/map/edit/MapEditClicks.cpp:391), so this only fires for a map or game file whose stored header is already out of range. Fine as hardening; the PR text does not overclaim.
  2. Fallback choice: every out-of-range value collapses to widget 0 = ally team 1, so after OK those teams end up allied with team 0. Falling back to the team's own index (the reset() no-alliance layout, always < teamCount for a valid team) would be a more neutral default. Keeping 0 is defensible for consistency with the CustomGameOtherOptions site.
  3. Test placement: the repo convention is test/ (singular) with CppUnit fixtures built by test/SConstruct into TestsRunner, which CI runs. This file lives in tests/ and is hand-run only, so CI never executes it. Suggest moving it into test/ as a fixture (FilenameStripTest.cpp is a minimal template).
  4. Pre-existing, out of scope: the same malformed file also feeds color[i]->setSelectedColor(teamNumber), which ColorButton stores unchecked, so the dialog now opens but OK writes that team number back.

The test lived in tests/ as a hand-compiled main() that nothing built,
so CI never ran it. Rewrite it as a CppUnit fixture in test/ and add it
to TestsRunner, which the Linux CI runs. Same cases: well-formed values
map to value - 1; 0, above-range, and 255 clamp to the first row.
@kylelutze

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Note 3 addressed in 0b0f598: the test is now a CppUnit fixture in test/AllyTeamWidgetIndexTest.cpp, registered in TestsRunner (158 tests pass locally). Note 4 tracked as #188. Keeping the index-0 fallback (note 2) for consistency with CustomGameOtherOptions.

@kylelutze
kylelutze merged commit dfc361f into master Sep 7, 2026
3 checks passed
@kylelutze
kylelutze deleted the fix/teams-editor-ally-index branch September 7, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants