From 18f9f6f4a6008f929c2d07ad22e4cfc8710f46cf Mon Sep 17 00:00:00 2001 From: drdkad Date: Tue, 18 Aug 2026 11:32:23 +0100 Subject: [PATCH 01/10] Add SetStrategies to declare the strategy list of a player --- src/games/game.h | 5 +++ src/games/gametable.cc | 98 ++++++++++++++++++++++++++++++++++++------ src/games/gametable.h | 8 +++- 3 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index 8461513fc5..ab65be3aa1 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -1383,6 +1383,11 @@ class GameRep : public std::enable_shared_from_this { { throw UndefinedException(); } + /// Declare the ordered list of strategies of a player. + virtual void SetStrategies(const GamePlayer &, const std::vector &) + { + throw UndefinedException(); + } /// Returns the total number of actions in the game virtual int BehavProfileLength() const = 0; //@} diff --git a/src/games/gametable.cc b/src/games/gametable.cc index dbb2afe273..90141268b4 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -563,8 +563,15 @@ void GameTableRep::DeleteStrategy(const GameStrategy &p_strategy) const auto deletedIt = std::find(player->m_strategies.begin(), player->m_strategies.end(), std::shared_ptr(p_strategy)); const long deletedDigit = deletedIt - player->m_strategies.begin(); + std::vector old_to_new(player->m_strategies.size(), -1); + long newDigit = 0; + for (size_t i = 0; i < old_to_new.size(); ++i) { + if (static_cast(i) != deletedDigit) { + old_to_new[i] = newDigit++; + } + } player->m_strategies.erase(deletedIt); - RebuildTable(old_radices, player->GetNumber() - 1, deletedDigit); + RebuildTable(old_radices, player->GetNumber() - 1, old_to_new); p_strategy->Invalidate(); } @@ -605,6 +612,71 @@ void GameTableRep::RelabelStrategies(const GamePlayer &p_player, } } +void GameTableRep::SetStrategies(const GamePlayer &p_player, + const std::vector &p_labels) +{ + if (p_player->GetGame().get() != this) { + throw MismatchException(); + } + if (p_labels.empty()) { + throw ValueException("At least one strategy must be specified"); + } + std::set declared; + for (const auto &label : p_labels) { + if (label.empty()) { + throw ValueException("Strategy label must not be empty"); + } + CheckLabel(label); + if (!declared.insert(label).second) { + throw ValueException("Strategy label '" + label + "' appears more than once"); + } + } + // Match declared labels against current strategies. + std::map current; + for (const auto &strategy : p_player->m_strategies) { + if (!current.emplace(strategy->GetLabel(), strategy->GetNumber() - 1).second) { + throw ValueException("Strategy label '" + strategy->GetLabel() + + "' is ambiguous for this player"); + } + } + std::vector source; + source.reserve(p_labels.size()); + for (const auto &label : p_labels) { + const auto it = current.find(label); + source.push_back((it != current.end()) ? it->second : -1); + } + std::vector old_to_new(p_player->m_strategies.size(), -1); + for (size_t i = 0; i < source.size(); ++i) { + if (source[i] >= 0) { + old_to_new[source[i]] = static_cast(i); + } + } + std::vector old_radices; + for (const auto &player : m_players) { + old_radices.push_back(player->m_strategies.size()); + } + + IncrementVersion(); + std::vector> newStrategies; + newStrategies.reserve(p_labels.size()); + for (size_t i = 0; i < p_labels.size(); ++i) { + if (source[i] >= 0) { + newStrategies.push_back(p_player->m_strategies[source[i]]); + } + else { + newStrategies.push_back( + std::make_shared(p_player.get(), static_cast(i) + 1, p_labels[i])); + } + } + for (const auto &strategy : p_player->m_strategies) { + if (declared.count(strategy->GetLabel()) == 0) { + strategy->Invalidate(); + } + } + p_player->m_strategies = std::move(newStrategies); + RebuildTable(old_radices, p_player->GetNumber() - 1, old_to_new); +} + //------------------------------------------------------------------------ // GameTableRep: Factory functions //------------------------------------------------------------------------ @@ -640,8 +712,8 @@ GameTableRep::NewMixedStrategyProfile(const Rational &, const StrategySupportPro /// This rebuilds a new table of outcomes after the game has been /// redimensioned (change in the number of strategies). See the declaration /// in gametable.h for the meaning of p_deletedPlayer/p_deletedDigit. -void GameTableRep::RebuildTable(const std::vector &old_radices, long p_deletedPlayer, - long p_deletedDigit) +void GameTableRep::RebuildTable(const std::vector &old_radices, long p_player, + const std::vector &p_oldToNew) { std::vector old_strides(old_radices.size()); long stride = 1; @@ -663,21 +735,23 @@ void GameTableRep::RebuildTable(const std::vector &old_radices, long p_del if (m_results[old_index] == nullptr) { continue; } - if (p_deletedPlayer >= 0 && - (old_index / old_strides[p_deletedPlayer]) % old_radices[p_deletedPlayer] == - p_deletedDigit) { - // This contingency used the strategy that was deleted. - continue; - } long new_index = 0; + bool dropped = false; for (size_t i = 0; i < m_players.size(); ++i) { long digit = (old_index / old_strides[i]) % old_radices[i]; - if (static_cast(i) == p_deletedPlayer && digit > p_deletedDigit) { - --digit; + if (static_cast(i) == p_player) { + digit = p_oldToNew[digit]; + if (digit < 0) { + // This contingency used a strategy that was removed. + dropped = true; + break; + } } new_index += digit * new_strides[i]; } - newResults[new_index] = m_results[old_index]; + if (!dropped) { + newResults[new_index] = m_results[old_index]; + } } m_results.swap(newResults); IndexStrategies(); diff --git a/src/games/gametable.h b/src/games/gametable.h index f79a09815b..3f453a698c 100644 --- a/src/games/gametable.h +++ b/src/games/gametable.h @@ -40,8 +40,11 @@ class GameTableRep : public GameExplicitRep { /// @name Private auxiliary functions //@{ - void RebuildTable(const std::vector &old_radices, long p_deletedPlayer = -1, - long p_deletedDigit = -1); + /// Rebuild the outcome table after the strategies of p_player have changed. + /// p_oldToNew maps old strategy indices to new index, or to -1 if the strategy was removed. + /// With p_player < 0, all indices are unchanged and only the table's dimensions are updated. + void RebuildTable(const std::vector &old_radices, long p_player = -1, + const std::vector &p_oldToNew = {}); //@} public: @@ -101,6 +104,7 @@ class GameTableRep : public GameExplicitRep { GameStrategy NewStrategy(const GamePlayer &, const std::string &) override; void DeleteStrategy(const GameStrategy &p_strategy) override; void RelabelStrategies(const GamePlayer &, const std::map &) override; + void SetStrategies(const GamePlayer &, const std::vector &) override; //@} /// @name Writing data files From e5993eabc34fb2c2a95d3e94aff22a89290e8687 Mon Sep 17 00:00:00 2001 From: drdkad Date: Tue, 18 Aug 2026 12:16:38 +0100 Subject: [PATCH 02/10] Expose Game.set_strategies in pygambit --- src/pygambit/gambit.pxd | 1 + src/pygambit/game.pxi | 153 +++++++++++++++++++++++----------------- 2 files changed, 88 insertions(+), 66 deletions(-) diff --git a/src/pygambit/gambit.pxd b/src/pygambit/gambit.pxd index 0bfa555af0..07bece0b77 100644 --- a/src/pygambit/gambit.pxd +++ b/src/pygambit/gambit.pxd @@ -335,6 +335,7 @@ cdef extern from "games/game.h": c_GameStrategy NewStrategy(c_GamePlayer, string) except +ValueError void DeleteStrategy(c_GameStrategy) except + void RelabelStrategies(c_GamePlayer, stdmap[string, string]) except +ValueError + void SetStrategies(c_GamePlayer, stdvector[string]) except +ValueError int MixedProfileLength() except + c_GameInfoset GetInfoset(int) except +IndexError diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index 1887c05d28..6efe1c8cc1 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -2532,72 +2532,6 @@ class Game: resolved_outcome = cython.cast(Outcome, self._resolve_outcome(outcome, "set_outcome")) self.game.deref().SetOutcome(resolved_node.node, resolved_outcome.outcome) - def add_strategy(self, player: Player | str, label: str) -> Strategy: - """Add a new strategy to the set of strategies for `player`. - - .. versionchanged:: 16.7.0 - A label is now required and must be nonempty and unique among the - player's strategies. - - Parameters - ---------- - player : Player or str - The player to create the new strategy for - label : str - The label for the new strategy. Must be nonempty and not already in use - by another of the player's strategies. - - Returns - ------- - Strategy - The newly-created strategy - - Raises - ------ - MismatchError - If `player` is a `Player` from a different game. - UndefinedOperationError - If called on a game which has an extensive representation. - ValueError - If `label` is empty or is already the label of another of the player's strategies. - """ - if self.is_tree: - raise UndefinedOperationError( - "Adding strategies is only applicable to games in strategic form" - ) - resolved_player = cython.cast(Player, - self._resolve_player(player, "add_strategy")) - return Strategy.wrap( - self.game.deref().NewStrategy(resolved_player.player, label.encode("utf-8")) - ) - - def delete_strategy(self, strategy: Strategy | str) -> None: - """Delete `strategy` from the game. - - Parameters - ---------- - strategy : Strategy or str - The strategy to delete - - Raises - ------ - MismatchError - If `strategy` is a `strategy` from a different game. - UndefinedOperationError - If called on a game which has an extensive representation, or if `strategy` is the - only strategy for its player. - """ - if self.is_tree: - raise UndefinedOperationError( - "Deleting strategies is only applicable to games in strategic form" - ) - resolved_strategy = cython.cast( - Strategy, self._resolve_strategy(strategy, "delete_strategy") - ) - if len(resolved_strategy.player.strategies) == 1: - raise UndefinedOperationError("Cannot delete the only strategy for a player") - self.game.deref().DeleteStrategy(resolved_strategy.strategy) - def relabel_strategies(self, player: Player | str, labels: typing.Mapping[str, str], @@ -2676,6 +2610,93 @@ class Game: return self.game.deref().RelabelStrategies(resolved_player.player, c_labels) + def set_strategies(self, + player: Player | str, + strategies: list[str], + drop: bool = False, + add: bool = True) -> None: + """Set the strategies of `player` to be `strategies`, matching by label. + + - An entry of `strategies` matching the label of a current strategy refers to + that strategy, keeping the outcomes at its contingencies; + - An entry matching no current strategy creates a new strategy there, + with no outcome at any of its contingencies; + - A current strategy whose label is not in `strategies` is deleted, + along with the outcomes at its contingencies. + - Listing the current labels in a new order reorders the strategies, + permuting the payoff table to match. + + The defaults permit creation and forbid deletion. + + .. versionadded:: 17.0.0 + Subsumes and replaces `Game.add_strategy` and `Game.delete_strategy`. + + Parameters + ---------- + player : Player or str + The player whose strategies to set. + strategies : list of str + The labels of the strategies the player is to have, in order. Must be + nonempty and without duplicates; each label must be a valid, nonempty label. + drop : bool, default False + Deleting strategies is destructive, so it must be explicitly confirmed: + if any current strategy is missing from `strategies` and `drop` is + `False`, the operation raises without modifying the game. + add : bool, default True + If `False`, entries of `strategies` matching no current strategy raise. + + Raises + ------ + MismatchError + If `player` is a `Player` from a different game. + KeyError + If `player` is a string matching no player. + TypeError + If `strategies` is a string, or not an iterable of strings. + UndefinedOperationError + If the game has a tree representation, where the strategies are derived + from the tree; or if `strategies` is empty. + ValueError + If a label in `strategies` is repeated, empty, or invalid. + + See Also + -------- + relabel_strategies : Change the labels of strategies, keeping the table unchanged. + set_actions : The analogous operation on the actions of an information set. + """ + if self.is_tree: + raise UndefinedOperationError( + "Setting strategies is only applicable to games in strategic form" + ) + resolved_player = cython.cast(Player, self._resolve_player(player, "set_strategies")) + if isinstance(strategies, str) or not hasattr(strategies, "__iter__"): + raise TypeError("set_strategies(): strategies must be an iterable of str") + labels = list(strategies) + for label in labels: + if not isinstance(label, str): + raise TypeError("set_strategies(): strategies must be an iterable of str") + if not labels: + raise UndefinedOperationError("set_strategies(): `strategies` must be a nonempty list") + current = [strategy.label for strategy in resolved_player.strategies] + if len(set(current)) != len(current): + raise ValueError( + "set_strategies(): the player has duplicate strategy labels, " + "so matching by label is not well-defined" + ) + added = [label for label in labels if label not in current] + if added and not add: + raise ValueError(f"set_strategies(): would create new strategies {added}") + missing = [label for label in current if label not in labels] + if missing and not drop: + raise ValueError( + f"set_strategies(): would delete strategies {missing} and the outcomes " + f"at their contingencies; pass drop=True to confirm" + ) + c_labels = stdvector[string]() + for label in labels: + c_labels.push_back(label.encode("utf-8")) + self.game.deref().SetStrategies(resolved_player.player, c_labels) + @dataclasses.dataclass class NodeCoordinates: From 5aaaf5a55392a37f7d212664a1e3932bdc0e6c28 Mon Sep 17 00:00:00 2001 From: drdkad Date: Tue, 18 Aug 2026 16:39:11 +0100 Subject: [PATCH 03/10] update api.rst --- doc/pygambit.api.rst | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/pygambit.api.rst b/doc/pygambit.api.rst index bb739d8333..161b671148 100644 --- a/doc/pygambit.api.rst +++ b/doc/pygambit.api.rst @@ -86,8 +86,6 @@ Transforming game components Game.add_outcome Game.delete_outcome Game.set_outcome - Game.add_strategy - Game.delete_strategy Game.relabel_strategies From 0527c7c26b6759c85d13e02674ab224895d622fd Mon Sep 17 00:00:00 2001 From: drdkad Date: Tue, 18 Aug 2026 16:41:27 +0100 Subject: [PATCH 04/10] update tests --- tests/test_players.py | 92 +++++++++++++++++++---------------------- tests/test_strategic.py | 43 +++++++++++++++++++ 2 files changed, 85 insertions(+), 50 deletions(-) diff --git a/tests/test_players.py b/tests/test_players.py index 0b58047238..ccb0135f77 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -140,29 +140,21 @@ def test_extensive_game_add_player(): assert len(pl1.strategies) == 1 -def test_strategic_game_add_strategy(): +def test_strategic_game_set_strategies_add(): game = gbt.Game.new_table([2, 2]) pl1, pl2 = game.players - game.add_strategy(pl1, "new strategy") + game.set_strategies(pl1, [s.label for s in pl1.strategies] + ["new strategy"]) assert len(pl1.strategies) == 3 # This second add also ensures that we are testing the case where there # are null outcomes in the table - game.add_strategy(pl2, "new strategy") + game.set_strategies(pl2, [s.label for s in pl2.strategies] + ["new strategy"]) assert len(pl2.strategies) == 3 -def test_extensive_game_add_strategy(): +def test_extensive_game_set_strategies(): game = gbt.Game.new_tree(["Alice"]) - assert len(game.players["Alice"].strategies) == 1 with pytest.raises(gbt.UndefinedOperationError): - game.add_strategy(game.players["Alice"], "new strategy") - - -def test_strategic_game_delete_strategy(): - game = gbt.Game.new_table([2, 2]) - pl1 = next(iter(game.players)) - game.delete_strategy(next(iter(pl1.strategies))) - assert len(pl1.strategies) == 1 + game.set_strategies(game.players["Alice"], ["new strategy"]) def _tag_contingencies(game: gbt.Game) -> None: @@ -178,23 +170,24 @@ def _tag_contingencies(game: gbt.Game) -> None: outcome[player] = int(f"{pl_index}{strategy.label}") -def test_strategic_game_delete_strategy_preserves_other_payoffs(): +def test_strategic_game_set_strategies_drop_preserves_other_payoffs(): game = gbt.Game.new_table([4, 2, 2]) pl1, pl2, pl3 = game.players _tag_contingencies(game) # Record expected payoffs by label (a stable identity), for the - # strategies of pl1 that survive deleting its second strategy. - surviving = [s for s in pl1.strategies if s.label != "2"] + # strategies of pl1 that survive dropping its second strategy. + surviving = [s.label for s in pl1.strategies if s.label != "2"] expected = { (s1.label, s2.label, s3.label): tuple(game[s1, s2, s3][p] for p in (pl1, pl2, pl3)) - for s1 in surviving for s2 in pl2.strategies for s3 in pl3.strategies + for s1 in pl1.strategies if s1.label in surviving + for s2 in pl2.strategies for s3 in pl3.strategies } - game.delete_strategy(pl1.strategies["2"]) + game.set_strategies(pl1, surviving, drop=True) - assert len(pl1.strategies) == 3 + assert [s.label for s in pl1.strategies] == surviving for s1 in pl1.strategies: for s2 in pl2.strategies: for s3 in pl3.strategies: @@ -203,37 +196,32 @@ def test_strategic_game_delete_strategy_preserves_other_payoffs(): assert actual == expected[key] -def test_strategic_game_delete_first_strategy_preserves_other_payoffs(): +def test_strategic_game_set_strategies_drop_first_preserves_other_payoffs(): game = gbt.Game.new_table([3, 2]) pl1, pl2 = game.players _tag_contingencies(game) - surviving = [s for s in pl1.strategies if s.label != "1"] + surviving = [s.label for s in pl1.strategies if s.label != "1"] expected = { (s1.label, s2.label): tuple(game[s1, s2][p] for p in (pl1, pl2)) - for s1 in surviving for s2 in pl2.strategies + for s1 in pl1.strategies if s1.label in surviving + for s2 in pl2.strategies } - game.delete_strategy(pl1.strategies["1"]) + game.set_strategies(pl1, surviving, drop=True) - assert len(pl1.strategies) == 2 + assert [s.label for s in pl1.strategies] == surviving for s1 in pl1.strategies: for s2 in pl2.strategies: key = (s1.label, s2.label) assert tuple(game[s1, s2][p] for p in (pl1, pl2)) == expected[key] -def test_strategic_game_delete_last_strategy(): +def test_strategic_game_set_strategies_empty(): game = gbt.Game.new_table([1, 2]) pl1 = next(iter(game.players)) with pytest.raises(gbt.UndefinedOperationError): - game.delete_strategy(next(iter(pl1.strategies))) - - -def test_extensive_game_delete_strategy(): - game = gbt.Game.new_tree(["Alice"]) - with pytest.raises(gbt.UndefinedOperationError): - game.delete_strategy(next(iter(game.players["Alice"].strategies))) + game.set_strategies(pl1, [], drop=True) def test_player_strategy_by_label(): @@ -244,23 +232,28 @@ def test_player_strategy_by_label(): @pytest.mark.parametrize("label", games.VALID_LABELS) -def test_add_strategy_label_valid(label): +def test_set_strategies_label_valid(label): game = gbt.Game.new_table([2, 2]) - strategy = game.add_strategy(next(iter(game.players)), label) - assert strategy.label == label + pl1 = next(iter(game.players)) + game.set_strategies(pl1, [s.label for s in pl1.strategies] + [label]) + assert [s.label for s in pl1.strategies][-1] == label @pytest.mark.parametrize("label", games.INVALID_LABELS) -def test_add_strategy_label_invalid_raises_valueerror(label): +def test_set_strategies_label_invalid_raises_valueerror(label): game = gbt.Game.new_table([2, 2]) + pl1 = next(iter(game.players)) with pytest.raises(ValueError): - game.add_strategy(next(iter(game.players)), label) + game.set_strategies(pl1, [s.label for s in pl1.strategies] + [label]) -def test_add_strategy_requires_label(): +def test_set_strategies_requires_iterable_of_str(): game = gbt.Game.new_table([2, 2]) + pl1 = next(iter(game.players)) + with pytest.raises(TypeError): + game.set_strategies(pl1, "12") with pytest.raises(TypeError): - game.add_strategy(next(iter(game.players))) + game.set_strategies(pl1, [1, 2]) def test_strategy_label_empty_raises_valueerror(): @@ -374,7 +367,7 @@ def test_player_get_min_payoff_null_outcome(): pl1, pl2 = game.players assert pl1.min_payoff == 1 assert pl2.min_payoff == 2 - game.add_strategy(pl1, "new strategy") + game.set_strategies(pl1, [s.label for s in pl1.strategies] + ["new strategy"]) # Currently the outcomes associated with the new entries in the table # are null outcomes. So now minimum payoff should be zero from those. for player in game.players: @@ -400,27 +393,26 @@ def test_player_get_max_payoff_null_outcome(): pl1, pl2 = game.players assert pl1.max_payoff == -1 assert pl2.max_payoff == -2 - game.add_strategy(pl1, "new strategy") + game.set_strategies(pl1, [s.label for s in pl1.strategies] + ["new strategy"]) # Currently the outcomes associated with the new entries in the table # are null outcomes. So now minimum payoff should be zero from those. for player in game.players: assert player.max_payoff == 0 -def test_add_strategy_duplicate_label_raises_and_leaves_game_unchanged(): +def test_set_strategies_duplicate_label_raises_and_leaves_game_unchanged(): game = gbt.Game.new_table([2, 2]) pl = next(iter(game.players)) - existing = next(iter(pl.strategies)).label - count_before = len(pl.strategies) + labels = [s.label for s in pl.strategies] with pytest.raises(ValueError): - game.add_strategy(pl, existing) - assert len(pl.strategies) == count_before + game.set_strategies(pl, labels + [labels[0]]) + assert [s.label for s in pl.strategies] == labels -def test_add_strategy_empty_label_raises_and_leaves_game_unchanged(): +def test_set_strategies_empty_label_raises_and_leaves_game_unchanged(): game = gbt.Game.new_table([2, 2]) pl = next(iter(game.players)) - count_before = len(pl.strategies) + labels = [s.label for s in pl.strategies] with pytest.raises(ValueError): - game.add_strategy(pl, "") - assert len(pl.strategies) == count_before + game.set_strategies(pl, labels + [""]) + assert [s.label for s in pl.strategies] == labels diff --git a/tests/test_strategic.py b/tests/test_strategic.py index 6fd70ef237..2791c9c117 100644 --- a/tests/test_strategic.py +++ b/tests/test_strategic.py @@ -125,3 +125,46 @@ def test_relabel_strategies_tree_game_raises(): game = games.read_from_file("stripped_down_poker.efg") with pytest.raises(gbt.UndefinedOperationError): game.relabel_strategies(game.players["Alice"], {"11": "XY"}) + + +def _payoffs_by_label(game: gbt.Game) -> dict: + one, two = game.players + return {(s.label, t.label): (game[s, t][one], game[s, t][two]) + for s in one.strategies for t in two.strategies} + + +def test_set_strategies_reorder_carries_outcomes(): + """Reordering permutes the payoff table: each contingency keeps the outcome + it had, identified by the labels of its strategies.""" + game = gbt.Game.from_arrays([[1, 2], [3, 4]], [[5, 6], [7, 8]]) + player, _ = game.players + a, b = (s.label for s in player.strategies) + kept = list(player.strategies) + before = _payoffs_by_label(game) + game.set_strategies(player, [b, a]) + assert [s.label for s in player.strategies] == [b, a] + assert list(player.strategies) == list(reversed(kept)) + assert _payoffs_by_label(game) == before + + +def test_set_strategies_add_drop_and_reorder_together(): + """A single call can create, delete, and reorder; the surviving strategy keeps + the outcomes at its contingencies.""" + game = gbt.Game.from_arrays([[1, 2], [3, 4]], [[5, 6], [7, 8]]) + player, other = game.players + a, b = (s.label for s in player.strategies) + kept = {t.label: game[a, t.label][player] for t in other.strategies} + game.set_strategies(player, ["X", a], drop=True) + assert [s.label for s in player.strategies] == ["X", a] + assert {t.label: game[a, t.label][player] for t in other.strategies} == kept + + +def test_set_strategies_unconfirmed_drop_and_disabled_add_raise(): + game = gbt.Game.new_table([2, 2]) + player, _ = game.players + a, b = (s.label for s in player.strategies) + with pytest.raises(ValueError): + game.set_strategies(player, [a]) + with pytest.raises(ValueError): + game.set_strategies(player, [a, b, "X"], add=False) + assert [s.label for s in player.strategies] == [a, b] From 89c6c69ba01912b6d3932889261f25eb870d32c8 Mon Sep 17 00:00:00 2001 From: drdkad Date: Tue, 18 Aug 2026 17:02:58 +0100 Subject: [PATCH 05/10] add Changelog entries --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 57cbe6baab..15e50a6042 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,8 @@ ### Added - Added `Game.relabel_strategies`, which simultaneously reassigns the labels of a player's strategies. (#1047) +- Added `Game.set_strategies`, which declares the ordered list of a player's strategies in a game in strategic form, + matching by label. (#1056) ### Fixed - `Game.add_player`, on a game in strategic form, no longer leaves the new player's sole @@ -12,6 +14,8 @@ ### Removed - Assigning to `Strategy.label` has been removed; use `Game.relabel_strategies`, which enforces nonempty, unique labels. (#1047) +- `Game.add_strategy` and `Game.delete_strategy` have been removed; use `Game.set_strategies`, + specifying the labels of the strategies the player is to have. (#1056) ## [17.0.0-alpha.1] - 2026-08-13 From 219bf3859249503680b337423d612bc5f8b886fb Mon Sep 17 00:00:00 2001 From: drdkad Date: Tue, 18 Aug 2026 17:21:59 +0100 Subject: [PATCH 06/10] update api.rst --- doc/pygambit.api.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/pygambit.api.rst b/doc/pygambit.api.rst index 161b671148..86e5a7a5c3 100644 --- a/doc/pygambit.api.rst +++ b/doc/pygambit.api.rst @@ -87,6 +87,7 @@ Transforming game components Game.delete_outcome Game.set_outcome Game.relabel_strategies + Game.set_strategies Information about the game From 346ae1bf19d557ac10135ddef0d0765daea298fd Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Wed, 19 Aug 2026 07:45:37 +0100 Subject: [PATCH 07/10] Update GUI to use `SetStrategies` and remove `NewStrategy` and `DeleteStrategy` --- src/games/game.h | 7 ------ src/games/gametable.cc | 50 +---------------------------------------- src/games/gametable.h | 7 ++---- src/gui/gamedoc.cc | 17 ++++++++++++-- src/gui/nfgpanel.cc | 7 +++++- src/gui/nfgtable.cc | 14 ++++++++++-- src/pygambit/gambit.pxd | 2 -- 7 files changed, 36 insertions(+), 68 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index ab65be3aa1..68e47f35d1 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -1370,13 +1370,6 @@ class GameRep : public std::enable_shared_from_this { } return *std::next(strategies.begin(), p_index - 1); } - /// Creates a new strategy for the player - virtual GameStrategy NewStrategy(const GamePlayer &p_player, const std::string &p_label) - { - throw UndefinedException(); - } - /// Remove the strategy from the game - virtual void DeleteStrategy(const GameStrategy &p_strategy) { throw UndefinedException(); } /// Simultaneously reassign strategy labels for a player: keys are current labels. /// Keys of p_labels are current action labels; values are their replacements. virtual void RelabelStrategies(const GamePlayer &, const std::map &) diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 90141268b4..43ccbbf131 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -527,54 +527,6 @@ void GameTableRep::DeleteOutcome(const GameOutcome &p_outcome) // GameTableRep: Strategies //------------------------------------------------------------------------ -GameStrategy GameTableRep::NewStrategy(const GamePlayer &p_player, const std::string &p_label) -{ - if (p_player->GetGame().get() != this) { - throw MismatchException(); - } - p_player->CheckStrategyLabel(p_label, {}); - auto strategy = std::make_shared(p_player.get(), - p_player->m_strategies.size() + 1, p_label); - IncrementVersion(); - std::vector old_radices; - for (const auto &player : m_players) { - old_radices.push_back(player->m_strategies.size()); - } - p_player->m_strategies.push_back(strategy); - RebuildTable(old_radices); - return p_player->m_strategies.back(); -} - -void GameTableRep::DeleteStrategy(const GameStrategy &p_strategy) -{ - const auto player = p_strategy->GetPlayer().get(); - if (player->m_game != this) { - throw MismatchException(); - } - if (player->GetStrategies().size() == 1) { - return; - } - - IncrementVersion(); - std::vector old_radices; - for (const auto &pl : m_players) { - old_radices.push_back(pl->m_strategies.size()); - } - const auto deletedIt = std::find(player->m_strategies.begin(), player->m_strategies.end(), - std::shared_ptr(p_strategy)); - const long deletedDigit = deletedIt - player->m_strategies.begin(); - std::vector old_to_new(player->m_strategies.size(), -1); - long newDigit = 0; - for (size_t i = 0; i < old_to_new.size(); ++i) { - if (static_cast(i) != deletedDigit) { - old_to_new[i] = newDigit++; - } - } - player->m_strategies.erase(deletedIt); - RebuildTable(old_radices, player->GetNumber() - 1, old_to_new); - p_strategy->Invalidate(); -} - void GameTableRep::RelabelStrategies(const GamePlayer &p_player, const std::map &p_labels) { @@ -711,7 +663,7 @@ GameTableRep::NewMixedStrategyProfile(const Rational &, const StrategySupportPro /// This rebuilds a new table of outcomes after the game has been /// redimensioned (change in the number of strategies). See the declaration -/// in gametable.h for the meaning of p_deletedPlayer/p_deletedDigit. +/// in gametable.h for the meaning of p_player/p_oldToNew. void GameTableRep::RebuildTable(const std::vector &old_radices, long p_player, const std::vector &p_oldToNew) { diff --git a/src/games/gametable.h b/src/games/gametable.h index 3f453a698c..c178a18d6d 100644 --- a/src/games/gametable.h +++ b/src/games/gametable.h @@ -42,9 +42,8 @@ class GameTableRep : public GameExplicitRep { //@{ /// Rebuild the outcome table after the strategies of p_player have changed. /// p_oldToNew maps old strategy indices to new index, or to -1 if the strategy was removed. - /// With p_player < 0, all indices are unchanged and only the table's dimensions are updated. - void RebuildTable(const std::vector &old_radices, long p_player = -1, - const std::vector &p_oldToNew = {}); + void RebuildTable(const std::vector &old_radices, long p_player, + const std::vector &p_oldToNew); //@} public: @@ -101,8 +100,6 @@ class GameTableRep : public GameExplicitRep { /// @name Strategies //@{ - GameStrategy NewStrategy(const GamePlayer &, const std::string &) override; - void DeleteStrategy(const GameStrategy &p_strategy) override; void RelabelStrategies(const GamePlayer &, const std::map &) override; void SetStrategies(const GamePlayer &, const std::vector &) override; //@} diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index 91d2ba2d98..3a5bd3a8c0 100644 --- a/src/gui/gamedoc.cc +++ b/src/gui/gamedoc.cc @@ -429,21 +429,34 @@ void GameDocument::DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label void GameDocument::DoNewStrategy(GamePlayer p_player) { + std::vector labels; std::set strategyLabels; for (const auto &strategy : p_player->GetStrategies()) { + labels.push_back(strategy->GetLabel()); strategyLabels.insert(strategy->GetLabel()); } int number = p_player->GetStrategies().size() + 1; while (strategyLabels.contains(std::to_string(number))) { number++; } - m_game->NewStrategy(p_player, std::to_string(number)); + labels.push_back(std::to_string(number)); + m_game->SetStrategies(p_player, labels); NotifyChanged(GameModificationType::GameForm); } void GameDocument::DoDeleteStrategy(GameStrategy p_strategy) { - m_game->DeleteStrategy(p_strategy); + const GamePlayer player = p_strategy->GetPlayer(); + if (player->GetStrategies().size() == 1) { + return; + } + std::vector labels; + for (const auto &strategy : player->GetStrategies()) { + if (strategy != p_strategy) { + labels.push_back(strategy->GetLabel()); + } + } + m_game->SetStrategies(player, labels); NotifyChanged(GameModificationType::GameForm); } diff --git a/src/gui/nfgpanel.cc b/src/gui/nfgpanel.cc index f1c46da084..bed71a5986 100644 --- a/src/gui/nfgpanel.cc +++ b/src/gui/nfgpanel.cc @@ -193,7 +193,12 @@ void TablePlayerPanel::OnChar(wxKeyEvent &p_event) void TablePlayerPanel::OnNewStrategy(wxCommandEvent &) { m_doc->PostPendingChanges(); - m_doc->DoNewStrategy(m_doc->GetGame()->GetPlayer(m_player)); + try { + m_doc->DoNewStrategy(m_doc->GetGame()->GetPlayer(m_player)); + } + catch (std::exception &ex) { + ExceptionDialog(this, ex.what()).ShowModal(); + } } void TablePlayerPanel::OnSetColor(wxCommandEvent &) diff --git a/src/gui/nfgtable.cc b/src/gui/nfgtable.cc index 5c4502c829..5b577898f4 100644 --- a/src/gui/nfgtable.cc +++ b/src/gui/nfgtable.cc @@ -1626,7 +1626,12 @@ void TableWidget::DeleteRowHeaderStrategy(int headerCol, int headerRow) const int player = GetRowHeaderPlayer(headerCol); const int strat = GetRowHeaderStrategy(headerCol, headerRow); - m_doc->DoDeleteStrategy(GetStrategyByPlayerAndIndex(player, strat)); + try { + m_doc->DoDeleteStrategy(GetStrategyByPlayerAndIndex(player, strat)); + } + catch (std::exception &ex) { + ExceptionDialog(this, ex.what()).ShowModal(); + } } void TableWidget::DeleteColHeaderStrategy(int headerRow, int headerCol) @@ -1634,7 +1639,12 @@ void TableWidget::DeleteColHeaderStrategy(int headerRow, int headerCol) const int player = GetColHeaderPlayer(headerRow); const int strat = GetColHeaderStrategy(headerRow, headerCol); - m_doc->DoDeleteStrategy(GetStrategyByPlayerAndIndex(player, strat)); + try { + m_doc->DoDeleteStrategy(GetStrategyByPlayerAndIndex(player, strat)); + } + catch (std::exception &ex) { + ExceptionDialog(this, ex.what()).ShowModal(); + } } void TableWidget::SetPayoffCellValue(int row, int col, const wxString &value) diff --git a/src/pygambit/gambit.pxd b/src/pygambit/gambit.pxd index 07bece0b77..2a9a6d441a 100644 --- a/src/pygambit/gambit.pxd +++ b/src/pygambit/gambit.pxd @@ -332,8 +332,6 @@ cdef extern from "games/game.h": Nodes GetNodes() except + c_GameStrategy GetStrategy(int) except +IndexError - c_GameStrategy NewStrategy(c_GamePlayer, string) except +ValueError - void DeleteStrategy(c_GameStrategy) except + void RelabelStrategies(c_GamePlayer, stdmap[string, string]) except +ValueError void SetStrategies(c_GamePlayer, stdvector[string]) except +ValueError int MixedProfileLength() except + From 87fb71208af20f465c3c499da3b0945775324880 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Wed, 19 Aug 2026 08:55:59 +0100 Subject: [PATCH 08/10] Implement new UX for editing player strategies in strategic form --- ChangeLog | 8 ++ Makefile.am | 4 +- doc/gui.nfg.rst | 86 ++++++++++++++--- src/gui/gamedoc.cc | 48 +++------- src/gui/gamedoc.h | 15 ++- src/gui/labelcell.cc | 151 ----------------------------- src/gui/labelcell.h | 79 ---------------- src/gui/nfgpanel.cc | 24 ----- src/gui/nfgtable.cc | 219 +++++++++++-------------------------------- src/gui/nfgtable.h | 10 +- 10 files changed, 166 insertions(+), 478 deletions(-) delete mode 100644 src/gui/labelcell.cc delete mode 100644 src/gui/labelcell.h diff --git a/ChangeLog b/ChangeLog index c198de23a2..55a332f070 100644 --- a/ChangeLog +++ b/ChangeLog @@ -17,12 +17,20 @@ ### Changed - All long-running calculations are now done in a thread in the GUI process, rather than calling the command-line executables as external processes. (#990) +- The GUI's strategic game table gained an "Edit strategies" dialog, opened by clicking any of a + player's strategy labels, supporting adding, deleting, reordering, and renaming that player's + strategies in one operation. The "+" toolbar icon for adding a strategy, and the strategy + label's right-click "Delete strategy" menu, are removed, superseded by the dialog. (#1056) ### Removed - Assigning to `Strategy.label` has been removed; use `Game.relabel_strategies`, which enforces nonempty, unique labels. (#1047) - `Game.add_strategy` and `Game.delete_strategy` have been removed; use `Game.set_strategies`, specifying the labels of the strategies the player is to have. (#1056) +- The C++ `Game` interface's `NewStrategy` and `DeleteStrategy` methods have been removed; they + were subsumed by `SetStrategies`, and by this point were used only in support of `pygambit`'s + `add_strategy`/`delete_strategy` and the GUI's strategy add/delete controls, which now build the + equivalent `SetStrategies` call directly. (#1056) ## [17.0.0-alpha.1] - 2026-08-13 diff --git a/Makefile.am b/Makefile.am index 59f3c06c45..bb19a273ad 100644 --- a/Makefile.am +++ b/Makefile.am @@ -495,14 +495,14 @@ gambit_SOURCES = \ src/gui/editlabel.h \ src/gui/edittext.cc \ src/gui/edittext.h \ - src/gui/labelcell.cc \ - src/gui/labelcell.h \ src/gui/dlabout.cc \ src/gui/dlabout.h \ src/gui/dleditmove.cc \ src/gui/dleditmove.h \ src/gui/dleditnode.cc \ src/gui/dleditnode.h \ + src/gui/dleditstrategies.cc \ + src/gui/dleditstrategies.h \ src/gui/dlefglayout.cc \ src/gui/dlefglayout.h \ src/gui/dlefglegend.cc \ diff --git a/doc/gui.nfg.rst b/doc/gui.nfg.rst index 87d1e02721..c19855ca8d 100644 --- a/doc/gui.nfg.rst +++ b/doc/gui.nfg.rst @@ -126,27 +126,85 @@ continues to identify the player to whom it belongs. does not change the players, strategies, outcomes, or payoffs. -Changing players and strategies -------------------------------- +Adding players +-------------- To add an additional player to the game, use the menu item :menuselection:`Edit --> Add player`, or the corresponding toolbar icon . The newly created player has one strategy, by default labeled with the number :guilabel:`1`. -Gambit supports arbitrary numbers of strategies for each player. To -add a new strategy for a player, click the new strategy icon located -to the left of that player's name. - -To edit the names of strategies, click on any cell in the strategic -game table where the strategy label appears, and edit the label using -the edit control. - -Right-clicking a strategy label creates a popup context menu. -This menu offers the ability to delete the selected strategy -from the game. It is not possible to delete a player's only strategy. - +Editing strategies +------------------- + +A player's strategies can be added, removed, reordered, and renamed by +clicking any cell in the strategic game table where one of that +player's strategy labels appears. This opens an :guilabel:`Edit +strategies` dialog for that player, titled with the player's own +label, listing a row for each of the player's strategies and showing +its label. + +Adding, removing, and reordering strategies +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Clicking :guilabel:`+`, below the list of strategies, adds a new +strategy with an automatically generated numeric label, which can be +relabeled like any other strategy. Each strategy's row has a +:guilabel:`✕` button to remove it, and :guilabel:`↑`/:guilabel:`↓` +buttons to move it earlier or later among the player's other +strategies. + +Removing a strategy does not immediately discard the payoffs at the +contingencies that use it. Instead, the row is shown disabled, with +its label struck through, and its :guilabel:`✕` button is replaced by +a :guilabel:`↺` (restore) button, so that what is about to be +destroyed remains visible -- and reversible -- until the dialog is +confirmed. Clicking :guilabel:`↺` returns the strategy, and the +payoffs at every contingency involving it, exactly as they were. +Nothing is actually deleted until :guilabel:`OK` is clicked; clicking +:guilabel:`Cancel` leaves the player's strategies entirely unchanged. +At least one strategy must remain: the last remaining strategy cannot +be removed. + +Renaming versus reordering +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Typing a new label into a strategy's row, and moving that row to a +different position with the :guilabel:`↑`/:guilabel:`↓` buttons, are +two distinct operations. They can be combined, but neither substitutes +for the other: + ++ Changing the text in a row renames that strategy, wherever it ends + up in the list. The payoffs at every contingency involving it travel + with it: renaming a strategy never changes any payoff. ++ Moving a row up or down changes the strategy's position among the + player's other strategies, permuting the payoff table to match. Each + strategy keeps its own payoffs as it moves: reordering a strategy + never changes what it is called. + +Because these are separate operations, two strategies can have their +labels swapped in place -- for instance, relabeling :guilabel:`Cooperate` +to :guilabel:`Defect` and :guilabel:`Defect` to :guilabel:`Cooperate` at +the same time, since all the relabeling for a player happens +simultaneously when :guilabel:`OK` is clicked -- without touching the +payoffs at any contingency. This is different from using the +:guilabel:`↑`/:guilabel:`↓` buttons to swap the *positions* of +:guilabel:`Cooperate` and :guilabel:`Defect`, which exchanges the +payoffs at the contingencies where each is chosen, while leaving each +strategy's own label attached to it. + +To help keep track of which is which while editing, a strategy whose +label has been changed from what it started as is shown in italic blue +text, and shows the original label in a tooltip on hover; a newly +added strategy is shown in bold green text. + +Strategy labels must be nonempty, and unique among the strategies +currently kept for the player (a removed strategy's label is not +considered). Any field that currently violates one of these rules is +highlighted, and a description of the problem is shown below the list +of strategies; the :guilabel:`OK` button is disabled until all fields +are valid. Editing payoffs diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index f9d493e659..067a331db3 100644 --- a/src/gui/gamedoc.cc +++ b/src/gui/gamedoc.cc @@ -427,46 +427,28 @@ void GameDocument::DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label NotifyChanged(GameModificationType::GameLabels); } -void GameDocument::DoNewStrategy(GamePlayer p_player) +void GameDocument::DoSetStrategies(GamePlayer p_player, + const std::vector &p_stableLabels, + const std::vector &p_labels) { - std::vector labels; - std::set strategyLabels; - for (const auto &strategy : p_player->GetStrategies()) { - labels.push_back(strategy->GetLabel()); - strategyLabels.insert(strategy->GetLabel()); - } - int number = p_player->GetStrategies().size() + 1; - while (strategyLabels.contains(std::to_string(number))) { - number++; - } - labels.push_back(std::to_string(number)); - m_game->SetStrategies(p_player, labels); - NotifyChanged(GameModificationType::GameForm); -} + // Phase 1: structure (which strategies exist, and in what order), resolved purely from + // p_stableLabels -- untouched by any pending rename in p_labels. + m_game->SetStrategies(p_player, p_stableLabels); -void GameDocument::DoDeleteStrategy(GameStrategy p_strategy) -{ - const GamePlayer player = p_strategy->GetPlayer(); - if (player->GetStrategies().size() == 1) { - return; - } - std::vector labels; - for (const auto &strategy : player->GetStrategies()) { - if (strategy != p_strategy) { - labels.push_back(strategy->GetLabel()); + // Phase 2: relabeling, applied once the structure has settled, so a label freed up by + // a deletion in phase 1 is available for reuse here. + std::map relabels; + for (size_t i = 0; i < p_stableLabels.size(); i++) { + if (p_stableLabels[i] != p_labels[i]) { + relabels[p_stableLabels[i]] = p_labels[i]; } } - m_game->SetStrategies(player, labels); + if (!relabels.empty()) { + m_game->RelabelStrategies(p_player, relabels); + } NotifyChanged(GameModificationType::GameForm); } -void GameDocument::DoSetStrategyLabel(GameStrategy p_strategy, const wxString &p_label) -{ - m_game->RelabelStrategies(p_strategy->GetPlayer(), - {{p_strategy->GetLabel(), p_label.ToStdString(wxConvUTF8)}}); - NotifyChanged(GameModificationType::GameLabels); -} - void GameDocument::DoSetInfosetLabel(GameInfoset p_infoset, const wxString &p_label) { p_infoset->SetLabel(p_label.ToStdString(wxConvUTF8)); diff --git a/src/gui/gamedoc.h b/src/gui/gamedoc.h index a6d201032c..56e34e86a4 100644 --- a/src/gui/gamedoc.h +++ b/src/gui/gamedoc.h @@ -297,9 +297,18 @@ class GameDocument { void DoSetTitle(const wxString &p_title, const wxString &p_comment); GamePlayer DoNewPlayer(); void DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label); - void DoNewStrategy(GamePlayer p_player); - void DoDeleteStrategy(GameStrategy p_strategy); - void DoSetStrategyLabel(GameStrategy p_strategy, const wxString &p_label); + /// Declare the strategies of `p_player` in a single operation, covering any combination + /// of adding, deleting, reordering, and relabeling strategies. + /// + /// `p_stableLabels` identifies each strategy as it was before this edit (an existing + /// strategy's current label, or a placeholder for one newly created); `p_labels` is what + /// that same strategy, by position, is to be labeled after the edit. Structure (which + /// strategies exist, and in what order) is resolved first, purely from `p_stableLabels`; + /// labels are then reassigned from `p_stableLabels` to `p_labels`. Doing so in this order + /// means a rename that reuses a label freed up by a simultaneous deletion never collides + /// with the not-yet-renamed original. + void DoSetStrategies(GamePlayer p_player, const std::vector &p_stableLabels, + const std::vector &p_labels); void DoSetInfosetLabel(GameInfoset p_infoset, const wxString &p_label); void DoRelabelActions(GameInfoset p_infoset, const std::map &p_labels); /// Declare the actions of `p_infoset` in a single operation, covering any combination diff --git a/src/gui/labelcell.cc b/src/gui/labelcell.cc deleted file mode 100644 index c670b00e62..0000000000 --- a/src/gui/labelcell.cc +++ /dev/null @@ -1,151 +0,0 @@ -// -// This file is part of Gambit -// Copyright (c) 1994-2026, The Gambit Project (https://www.gambit-project.org) -// -// FILE: src/gui/labelcell.cc -// Implementation of wxGrid editor for Gambit labels -// -// This program is free software; you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation; either version 2 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program; if not, write to the Free Software -// Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. -// - -#ifndef WX_PRECOMP -#include -#endif // WX_PRECOMP - -#include "labelcell.h" - -namespace Gambit::GUI { - -//! -//! GetCellSize() only reports what's directly set on a cell's own attr; it -//! does not detect "am I covered by an earlier cell's forward-reaching -//! span" (confirmed empirically -- it returns CellSpan_None, not -//! CellSpan_Inside, for a covered cell in this codebase's GetAttr()-computed -//! span design, where only the anchor cell's attr ever gets an explicit -//! SetSize()). So the anchor has to be found by scanning backward and -//! checking whether an earlier Main-kind cell's span reaches forward far -//! enough to cover (row, col). -//! -static void ResolveSpanAnchor(wxGrid *p_grid, int &p_row, int &p_col) -{ - for (int row = p_row; row >= 0; --row) { - int numRows, numCols; - if (p_grid->GetCellSize(row, p_col, &numRows, &numCols) == wxGrid::CellSpan_Main) { - if (row + numRows > p_row) { - p_row = row; - } - break; - } - } - - for (int col = p_col; col >= 0; --col) { - int numRows, numCols; - if (p_grid->GetCellSize(p_row, col, &numRows, &numCols) == wxGrid::CellSpan_Main) { - if (col + numCols > p_col) { - p_col = col; - } - break; - } - } -} - -void LabelCellEditor::Create(wxWindow *parent, wxWindowID id, wxEvtHandler *evtHandler) -{ - auto *textCtrl = new LabelTextCtrl(parent, id, wxEmptyString, wxDefaultPosition, wxDefaultSize, - wxTE_PROCESS_TAB | wxTE_CENTER | wxBORDER_NONE); - SetWindow(textCtrl); - if (evtHandler) { - textCtrl->PushEventHandler(evtHandler); - } -} - -//! -//! Keeps the full cell width (so a longer replacement value has room to -//! grow into), but the height must be the *exact* single-line height, not -//! padded: a single-line wxTextCtrl does not itself center its text -//! vertically when its own height exceeds one line, so any extra height -//! here just reproduces the top-alignment problem one level in. Centering -//! the (exactly one line tall) control within the taller cell via -//! DoPositionEditor is what actually provides the vertical centering. -//! -void LabelCellEditor::PositionInCell(int row, int col) -{ - const wxRect rect = m_grid->CellToRect(row, col); - const wxSize size(rect.width, Text()->GetBestSize().GetHeight()); - DoPositionEditor(size, rect, wxALIGN_CENTRE, wxALIGN_CENTRE_VERTICAL); -} - -void LabelCellEditor::BeginEdit(int row, int col, wxGrid *grid) -{ - m_grid = grid; - m_row = row; - m_col = col; - ResolveSpanAnchor(grid, m_row, m_col); - - m_startValue = grid->GetTable()->GetValue(m_row, m_col); - Text()->SetValue(m_startValue); - // In case SetSize() already ran (before m_grid was set above) with a rect - // that isn't span-aware, apply the correct one now too. - PositionInCell(m_row, m_col); - Text()->SelectAll(); - Text()->SetFocus(); -} - -bool LabelCellEditor::EndEdit(int, int, const wxGrid *, const wxString &oldval, wxString *newval) -{ - const wxString value = Text()->GetNormalizedValue(); - - if (value.empty()) { - wxBell(); - Text()->SetFocus(); - return false; - } - - if (value == oldval) { - return false; - } - - m_newValue = value; - *newval = value; - return true; -} - -void LabelCellEditor::ApplyEdit(int row, int col, wxGrid *grid) -{ - grid->GetTable()->SetValue(row, col, m_newValue); -} - -void LabelCellEditor::Reset() { Text()->SetValue(m_startValue); } - -void LabelCellEditor::SetSize(const wxRect &) -{ - if (m_grid) { - PositionInCell(m_row, m_col); - } -} - -void LabelCellEditor::StartingKey(wxKeyEvent &event) -{ - const wxChar ch = event.GetUnicodeKey(); - if (ch != WXK_NONE) { - Text()->SetValue(wxString(ch)); - Text()->SetInsertionPointEnd(); - } - else { - event.Skip(); - } -} - -} // namespace Gambit::GUI diff --git a/src/gui/labelcell.h b/src/gui/labelcell.h deleted file mode 100644 index 7ccdb6246c..0000000000 --- a/src/gui/labelcell.h +++ /dev/null @@ -1,79 +0,0 @@ -// -// This file is part of Gambit -// Copyright (c) 1994-2026, The Gambit Project (https://www.gambit-project.org) -// -// FILE: src/gui/labelcell.h -// Declaration of wxGrid editor for Gambit labels -// -// This program is free software; you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation; either version 2 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program; if not, write to the Free Software -// Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. -// - -#ifndef GAMBIT_GUI_LABELCELLEDITOR_H -#define GAMBIT_GUI_LABELCELLEDITOR_H - -#include - -#include "editlabel.h" - -namespace Gambit::GUI { - -//! -//! Cell editor for strategy/action label renaming. Wraps LabelTextCtrl, -//! which does its own live whitespace normalization; this class only -//! handles the wxGridCellEditor lifecycle (open/commit/cancel) around it. -//! -class LabelCellEditor final : public wxGridCellEditor { - wxString m_startValue; - wxString m_newValue; - - /// Cell this editor is currently attached to, kept so SetSize() can - /// recompute the correct on-screen rect itself -- see the comment there. - wxGrid *m_grid{nullptr}; - int m_row{0}; - int m_col{0}; - - LabelTextCtrl *Text() const { return static_cast(GetWindow()); } - - /// Positions the editor within (row, col)'s cell: full width, natural - /// single-line height, centered vertically. See the definition for why. - void PositionInCell(int row, int col); - -public: - LabelCellEditor() = default; - - void Create(wxWindow *parent, wxWindowID id, wxEvtHandler *evtHandler) override; - void BeginEdit(int row, int col, wxGrid *grid) override; - bool EndEdit(int row, int col, const wxGrid *grid, const wxString &oldval, - wxString *newval) override; - void ApplyEdit(int row, int col, wxGrid *grid) override; - void Reset() override; - - /// wxGrid's own editor-positioning logic isn't aware of merged (spanned) - /// cells the way CellToRect() is, so the correct rect for a spanned - /// header cell is recomputed here directly rather than trusting whatever - /// rect wxGrid passes in. - void SetSize(const wxRect &rect) override; - - /// Injects the keystroke that triggered editing (typing directly on a - /// selected cell, rather than clicking/F2) into the editor. - void StartingKey(wxKeyEvent &event) override; - - wxGridCellEditor *Clone() const override { return new LabelCellEditor(); } - wxString GetValue() const override { return Text()->GetValue(); } -}; - -} // namespace Gambit::GUI - -#endif // GAMBIT_GUI_LABELCELLEDITOR_H diff --git a/src/gui/nfgpanel.cc b/src/gui/nfgpanel.cc index bed71a5986..776ac4851a 100644 --- a/src/gui/nfgpanel.cc +++ b/src/gui/nfgpanel.cc @@ -36,7 +36,6 @@ #include "edittext.h" #include "bitmaps/color.xpm" -#include "bitmaps/newrow.xpm" #include "bitmaps/person.xpm" namespace Gambit::GUI { @@ -78,8 +77,6 @@ class TablePlayerPanel final : public wxPanel { /// @name Event handlers //@{ - /// The add strategy icon is clicked - void OnNewStrategy(wxCommandEvent &); /// The set color icon is clicked void OnSetColor(wxCommandEvent &); /// Start the editing of the player label @@ -115,16 +112,6 @@ TablePlayerPanel::TablePlayerPanel(wxWindow *p_parent, NfgPanel *p_nfgPanel, Gam wxStaticBitmap *playerIcon = new TablePlayerIcon(this, m_player); labelSizer->Add(playerIcon, 0, wxALL | wxALIGN_CENTER, 0); - if (!m_doc->GetGame()->IsTree()) { - auto *addStrategyIcon = new wxBitmapButton(this, wxID_ANY, wxBitmap(newrow_xpm), - wxDefaultPosition, wxDefaultSize, wxNO_BORDER); - addStrategyIcon->SetToolTip(_("Add a strategy for this player")); - - labelSizer->Add(addStrategyIcon, 0, wxALL | wxALIGN_CENTER, 0); - Connect(addStrategyIcon->GetId(), wxEVT_COMMAND_BUTTON_CLICKED, - wxCommandEventHandler(TablePlayerPanel::OnNewStrategy)); - } - auto *setColorIcon = new wxBitmapButton(this, wxID_ANY, wxBitmap(color_xpm), wxDefaultPosition, wxDefaultSize, wxNO_BORDER); setColorIcon->SetToolTip(_("Change the color for this player")); @@ -190,17 +177,6 @@ void TablePlayerPanel::OnChar(wxKeyEvent &p_event) } } -void TablePlayerPanel::OnNewStrategy(wxCommandEvent &) -{ - m_doc->PostPendingChanges(); - try { - m_doc->DoNewStrategy(m_doc->GetGame()->GetPlayer(m_player)); - } - catch (std::exception &ex) { - ExceptionDialog(this, ex.what()).ShowModal(); - } -} - void TablePlayerPanel::OnSetColor(wxCommandEvent &) { wxColourData data; diff --git a/src/gui/nfgtable.cc b/src/gui/nfgtable.cc index 5b577898f4..ed32e044a2 100644 --- a/src/gui/nfgtable.cc +++ b/src/gui/nfgtable.cc @@ -33,11 +33,11 @@ #include "renratio.h" // special renderer/editor for rational numbers #include "editlabel.h" -#include "labelcell.h" #include "gamedoc.h" #include "nfgpanel.h" #include "nfgtable.h" +#include "dleditstrategies.h" #include "dlexcept.h" namespace Gambit::GUI { @@ -187,30 +187,23 @@ class RowPlayerTable final : public wxGridTableBase { int m_numRows{0}; int m_numCols{0}; RowHeaderCellRenderer *m_renderer; - LabelCellEditor *m_editor; public: explicit RowPlayerTable(TableWidget *p_table) - : m_table(p_table), m_renderer(new RowHeaderCellRenderer(p_table)), - m_editor(new LabelCellEditor()) + : m_table(p_table), m_renderer(new RowHeaderCellRenderer(p_table)) { // Freshly-constructed wxGridCellWorker-derived objects start unreferenced; // this is our own table's stake, matched by DecRef() in the destructor. - // Each SetRenderer()/SetEditor() call below additionally consumes one - // reference per call (that's the ownership contract of those setters), - // so GetAttr() takes a fresh IncRef() immediately before each one -- the - // instance itself is reused across all cells and all calls, which is - // required: wxGrid expects the *same* editor/renderer object identity - // back on repeated queries for a given cell's active editor. + // Each SetRenderer() call below additionally consumes one reference per + // call (that's the ownership contract of that setter), so GetAttr() takes + // a fresh IncRef() immediately before each one -- the instance itself is + // reused across all cells and all calls, which is required: wxGrid + // expects the *same* renderer object identity back on repeated queries + // for a given cell. m_renderer->IncRef(); - m_editor->IncRef(); } - ~RowPlayerTable() override - { - m_renderer->DecRef(); - m_editor->DecRef(); - } + ~RowPlayerTable() override { m_renderer->DecRef(); } int GetNumberRows() override { return m_numRows; } int GetNumberCols() override { return m_numCols; } @@ -234,20 +227,12 @@ wxString RowPlayerTable::GetValue(int row, int col) return wxString::FromUTF8(m_table->GetStrategyByPlayerAndIndex(player, strat)->GetLabel()); } -void RowPlayerTable::SetValue(int row, int col, const wxString &value) +void RowPlayerTable::SetValue(int, int, const wxString &) { - const wxString label = LabelTextCtrl::Normalize(value, true); - - if (label.empty()) { - wxBell(); - return; - } - - const wxString result = m_table->RenameRowHeaderStrategy(col, row, label); - if (!result.empty() && GetView()) { - wxWindow *win = GetView(); - win->CallAfter([win, result] { ExceptionDialog(win, result.ToStdString()).ShowModal(); }); - } + // Row header cells are read-only: editing a player's strategies goes entirely through + // TableWidget::EditStrategies(), triggered on left-click rather than through wxGrid's own + // cell-editing machinery. This override exists only to satisfy the pure virtual + // wxGridTableBase interface. } wxGridCellAttr *RowPlayerTable::GetAttr(int row, int col, wxGridCellAttr::wxAttrKind) @@ -260,16 +245,13 @@ wxGridCellAttr *RowPlayerTable::GetAttr(int row, int col, wxGridCellAttr::wxAttr // since the "neighbour" past a span boundary is a different strategy's // header cell, not empty space. attr->SetOverflow(false); + attr->SetReadOnly(true); if (m_table->GetRowHeaderColCount() > 0) { attr->SetTextColour(m_table->GetPlayerColor(m_table->GetRowHeaderPlayer(col))); - m_editor->IncRef(); - attr->SetEditor(m_editor); - attr->SetReadOnly(m_table->IsReadOnly()); } else { attr->SetTextColour(*wxBLACK); - attr->SetReadOnly(true); } // GetAttr() constructs a fresh attr on every call, so the span has to be @@ -352,7 +334,6 @@ class RowPlayerGrid final : public TableGridBase { RowPlayerTable *m_gridTable; void OnCellLeftClick(wxGridEvent &) override; - void OnCellRightClick(wxGridEvent &); bool ShowPlayerDropMenu(int p_index, int p_player, const wxString &p_label, const wxPoint &p_pos); @@ -381,8 +362,6 @@ RowPlayerGrid::RowPlayerGrid(TableWidget *p_parent) // and OS drag events over the cell area is the internal grid window, not // the outer wxGrid object, so the drop target has to be registered there. GetGridWindow()->SetDropTarget(new TableWidgetDropTarget(this)); - - Bind(wxEVT_GRID_CELL_RIGHT_CLICK, &RowPlayerGrid::OnCellRightClick, this); } namespace { @@ -463,39 +442,25 @@ void ShowStrategyPopup(wxGrid *p_grid, int p_row, int p_col, const GameStrategy void RowPlayerGrid::OnCellLeftClick(wxGridEvent &p_event) { - if (!m_table->IsReadOnly() || m_table->GetRowHeaderColCount() == 0) { + if (m_table->GetRowHeaderColCount() == 0) { TableGridBase::OnCellLeftClick(p_event); return; } - const int row = p_event.GetRow(); const int col = p_event.GetCol(); const int player = m_table->GetRowHeaderPlayer(col); - const int strategy = m_table->GetRowHeaderStrategy(col, row); - const auto gameStrategy = m_table->GetStrategyByPlayerAndIndex(player, strategy); - if (!GetStrategyTooltip(gameStrategy).empty()) { - ShowStrategyPopup(this, row, col, gameStrategy); - } -} -void RowPlayerGrid::OnCellRightClick(wxGridEvent &p_event) -{ - if (m_table->GetRowHeaderColCount() == 0 || m_table->IsReadOnly()) { - p_event.Skip(); + if (m_table->IsReadOnly()) { + const int row = p_event.GetRow(); + const int strategy = m_table->GetRowHeaderStrategy(col, row); + const auto gameStrategy = m_table->GetStrategyByPlayerAndIndex(player, strategy); + if (!GetStrategyTooltip(gameStrategy).empty()) { + ShowStrategyPopup(this, row, col, gameStrategy); + } return; } - const int row = p_event.GetRow(); - const int col = p_event.GetCol(); - - wxMenu menu; - wxMenuItem *deleteItem = menu.Append(wxID_DELETE, _("Delete strategy")); - deleteItem->Enable(m_table->CanDeleteRowHeaderStrategy(col, row)); - menu.Bind( - wxEVT_MENU, - [this, row, col](wxCommandEvent &) { m_table->DeleteRowHeaderStrategy(col, row); }, - wxID_DELETE); - PopupMenu(&menu, p_event.GetPosition()); + m_table->EditStrategies(player); } bool RowPlayerGrid::ShowPlayerDropMenu(int p_index, int p_player, const wxString &p_label, @@ -611,22 +576,15 @@ class ColPlayerTable final : public wxGridTableBase { int m_numRows{0}; int m_numCols{0}; ColHeaderCellRenderer *m_renderer; - LabelCellEditor *m_editor; public: explicit ColPlayerTable(TableWidget *p_table) - : m_table(p_table), m_renderer(new ColHeaderCellRenderer(p_table)), - m_editor(new LabelCellEditor()) + : m_table(p_table), m_renderer(new ColHeaderCellRenderer(p_table)) { m_renderer->IncRef(); - m_editor->IncRef(); } - ~ColPlayerTable() override - { - m_renderer->DecRef(); - m_editor->DecRef(); - } + ~ColPlayerTable() override { m_renderer->DecRef(); } int GetNumberRows() override { return m_numRows; } int GetNumberCols() override { return m_numCols; } @@ -649,20 +607,12 @@ wxString ColPlayerTable::GetValue(int row, int col) return wxString::FromUTF8(m_table->GetStrategyByPlayerAndIndex(player, strat)->GetLabel()); } -void ColPlayerTable::SetValue(int row, int col, const wxString &value) +void ColPlayerTable::SetValue(int, int, const wxString &) { - const wxString label = LabelTextCtrl::Normalize(value, true); - - if (label.empty()) { - wxBell(); - return; - } - - const wxString result = m_table->RenameColHeaderStrategy(row, col, label); - if (!result.empty() && GetView()) { - wxWindow *win = GetView(); - win->CallAfter([win, result] { ExceptionDialog(win, result.ToStdString()).ShowModal(); }); - } + // Column header cells are read-only: editing a player's strategies goes entirely through + // TableWidget::EditStrategies(), triggered on left-click rather than through wxGrid's own + // cell-editing machinery. This override exists only to satisfy the pure virtual + // wxGridTableBase interface. } wxGridCellAttr *ColPlayerTable::GetAttr(int row, int col, wxGridCellAttr::wxAttrKind) @@ -675,16 +625,13 @@ wxGridCellAttr *ColPlayerTable::GetAttr(int row, int col, wxGridCellAttr::wxAttr // since the "neighbour" past a span boundary is a different strategy's // header cell, not empty space. attr->SetOverflow(false); + attr->SetReadOnly(true); if (m_table->GetColHeaderRowCount() > 0) { attr->SetTextColour(m_table->GetPlayerColor(m_table->GetColHeaderPlayer(row))); - m_editor->IncRef(); - attr->SetEditor(m_editor); - attr->SetReadOnly(m_table->IsReadOnly()); } else { attr->SetTextColour(*wxBLACK); - attr->SetReadOnly(true); } // See the equivalent comment in RowPlayerTable::GetAttr: span has to be @@ -763,7 +710,6 @@ class ColPlayerGrid final : public TableGridBase { ColPlayerTable *m_gridTable; void OnCellLeftClick(wxGridEvent &) override; - void OnCellRightClick(wxGridEvent &); bool ShowPlayerDropMenu(int p_index, int p_player, const wxString &p_label, const wxPoint &p_pos); @@ -791,45 +737,29 @@ ColPlayerGrid::ColPlayerGrid(TableWidget *p_parent) // and OS drag events over the cell area is the internal grid window, not // the outer wxGrid object, so the drop target has to be registered there. GetGridWindow()->SetDropTarget(new TableWidgetDropTarget(this)); - - Bind(wxEVT_GRID_CELL_RIGHT_CLICK, &ColPlayerGrid::OnCellRightClick, this); } void ColPlayerGrid::OnCellLeftClick(wxGridEvent &p_event) { - if (!m_table->IsReadOnly() || m_table->GetColHeaderRowCount() == 0) { + if (m_table->GetColHeaderRowCount() == 0) { TableGridBase::OnCellLeftClick(p_event); return; } const int row = p_event.GetRow(); - const int col = p_event.GetCol(); const int player = m_table->GetColHeaderPlayer(row); - const int strategy = m_table->GetColHeaderStrategy(row, col); - const auto gameStrategy = m_table->GetStrategyByPlayerAndIndex(player, strategy); - if (!GetStrategyTooltip(gameStrategy).empty()) { - ShowStrategyPopup(this, row, col, gameStrategy); - } -} -void ColPlayerGrid::OnCellRightClick(wxGridEvent &p_event) -{ - if (m_table->GetColHeaderRowCount() == 0 || m_table->IsReadOnly()) { - p_event.Skip(); + if (m_table->IsReadOnly()) { + const int col = p_event.GetCol(); + const int strategy = m_table->GetColHeaderStrategy(row, col); + const auto gameStrategy = m_table->GetStrategyByPlayerAndIndex(player, strategy); + if (!GetStrategyTooltip(gameStrategy).empty()) { + ShowStrategyPopup(this, row, col, gameStrategy); + } return; } - const int row = p_event.GetRow(); - const int col = p_event.GetCol(); - - wxMenu menu; - wxMenuItem *deleteItem = menu.Append(wxID_DELETE, _("Delete strategy")); - deleteItem->Enable(m_table->CanDeleteColHeaderStrategy(row, col)); - menu.Bind( - wxEVT_MENU, - [this, row, col](wxCommandEvent &) { m_table->DeleteColHeaderStrategy(row, col); }, - wxID_DELETE); - PopupMenu(&menu, p_event.GetPosition()); + m_table->EditStrategies(player); } bool ColPlayerGrid::ShowPlayerDropMenu(int p_index, int p_player, const wxString &p_label, @@ -1593,54 +1523,25 @@ void TableWidget::RenderGame(wxDC &p_dc, int p_marginX, int p_marginY) DrawGridToDC(m_payoffGrid, p_dc); } -wxString TableWidget::RenameRowHeaderStrategy(int headerCol, int headerRow, const wxString &value) -{ - const int player = GetRowHeaderPlayer(headerCol); - const int strat = GetRowHeaderStrategy(headerCol, headerRow); - - try { - m_doc->DoSetStrategyLabel(GetStrategyByPlayerAndIndex(player, strat), value); - } - catch (std::exception &ex) { - return wxString::FromUTF8(ex.what()); - } - return ""; -} - -wxString TableWidget::RenameColHeaderStrategy(int headerRow, int headerCol, const wxString &value) +void TableWidget::EditStrategies(int player) { - const int player = GetColHeaderPlayer(headerRow); - const int strat = GetColHeaderStrategy(headerRow, headerCol); - - try { - m_doc->DoSetStrategyLabel(GetStrategyByPlayerAndIndex(player, strat), value); - } - catch (std::exception &ex) { - return wxString::FromUTF8(ex.what()); + const GamePlayer gamePlayer = m_doc->GetGame()->GetPlayer(player); + EditStrategiesDialog dialog(this, gamePlayer); + if (dialog.ShowModal() != wxID_OK) { + return; } - return ""; -} -void TableWidget::DeleteRowHeaderStrategy(int headerCol, int headerRow) -{ - const int player = GetRowHeaderPlayer(headerCol); - const int strat = GetRowHeaderStrategy(headerCol, headerRow); - - try { - m_doc->DoDeleteStrategy(GetStrategyByPlayerAndIndex(player, strat)); - } - catch (std::exception &ex) { - ExceptionDialog(this, ex.what()).ShowModal(); + std::vector stableLabels, labels; + for (int i = 0; i < dialog.NumStrategies(); i++) { + if (dialog.IsDeleted(i)) { + continue; + } + stableLabels.push_back(dialog.GetStableLabel(i)); + labels.push_back(dialog.GetStrategyLabel(i).ToStdString(wxConvUTF8)); } -} - -void TableWidget::DeleteColHeaderStrategy(int headerRow, int headerCol) -{ - const int player = GetColHeaderPlayer(headerRow); - const int strat = GetColHeaderStrategy(headerRow, headerCol); try { - m_doc->DoDeleteStrategy(GetStrategyByPlayerAndIndex(player, strat)); + m_doc->DoSetStrategies(gamePlayer, stableLabels, labels); } catch (std::exception &ex) { ExceptionDialog(this, ex.what()).ShowModal(); @@ -1687,16 +1588,4 @@ GameStrategy TableWidget::GetStrategyByPlayerAndIndex(int player, int strategy) return *std::next(strategies.begin(), strategy - 1); } -bool TableWidget::CanDeleteRowHeaderStrategy(int headerCol, int) const -{ - const int player = GetRowHeaderPlayer(headerCol); - return GetSupport().GetStrategies(GetSupport().GetGame()->GetPlayer(player)).size() > 1; -} - -bool TableWidget::CanDeleteColHeaderStrategy(int headerRow, int) const -{ - const int player = GetColHeaderPlayer(headerRow); - return GetSupport().GetStrategies(GetSupport().GetGame()->GetPlayer(player)).size() > 1; -} - } // namespace Gambit::GUI diff --git a/src/gui/nfgtable.h b/src/gui/nfgtable.h index 9febb9ea98..904e325189 100644 --- a/src/gui/nfgtable.h +++ b/src/gui/nfgtable.h @@ -406,16 +406,12 @@ class TableWidget final : public wxPanel { void GetSVG(const wxString &p_filename, int marginX, int marginY); /// Prints the game as currently displayed, centered on the DC void RenderGame(wxDC &p_dc, int marginX, int marginY); - wxString RenameRowHeaderStrategy(int headerCol, int headerRow, const wxString &value); - wxString RenameColHeaderStrategy(int headerRow, int headerCol, const wxString &value); - - bool CanDeleteRowHeaderStrategy(int headerCol, int headerRow) const; - bool CanDeleteColHeaderStrategy(int headerRow, int headerCol) const; - void DeleteRowHeaderStrategy(int headerCol, int headerRow); - void DeleteColHeaderStrategy(int headerRow, int headerCol); void SetPayoffCellValue(int row, int col, const wxString &value); //@} + + /// Opens the "Edit strategies" dialog for `player`, committing any changes made. + void EditStrategies(int player); }; } // namespace Gambit::GUI #endif // GAMBIT_GUI_NFGTABLE_H From a08a504032bb7ce5ab1133960cb3377ea7c804f2 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Wed, 19 Aug 2026 09:22:04 +0100 Subject: [PATCH 09/10] Adding missing source for new dialog --- src/gui/dleditstrategies.cc | 479 ++++++++++++++++++++++++++++++++++++ src/gui/dleditstrategies.h | 58 +++++ 2 files changed, 537 insertions(+) create mode 100644 src/gui/dleditstrategies.cc create mode 100644 src/gui/dleditstrategies.h diff --git a/src/gui/dleditstrategies.cc b/src/gui/dleditstrategies.cc new file mode 100644 index 0000000000..46bcce58f2 --- /dev/null +++ b/src/gui/dleditstrategies.cc @@ -0,0 +1,479 @@ +// +// This file is part of Gambit +// Copyright (c) 1994-2026, The Gambit Project (https://www.gambit-project.org) +// +// FILE: src/gui/dleditstrategies.cc +// Dialog for viewing and editing the strategies of a player +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation; either version 2 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. +// + +#include +#ifndef WX_PRECOMP +#include +#endif // WX_PRECOMP +#include + +#include +#include +#include + +#include "gambit.h" +#include "dleditstrategies.h" +#include "editlabel.h" + +namespace Gambit::GUI { + +namespace { +const wxColour kInvalidLabelBg(255, 220, 220); +const wxColour kNewLabelColour(0, 102, 0); // new strategy: bold, dark green +const wxColour kRenamedLabelColour(0, 51, 204); // renamed strategy: italic, strong blue +} // namespace + +// An editable, variable-length list of a player's strategies, colour-coded like a +// source-control diff (added/renamed/deleted). +// +// Each row's *stable label* (its label before this dialog touched it, or a placeholder for a +// newly added row) never changes; it's what `SetStrategies()` uses to match the row to its +// underlying strategy regardless of what the user has typed into its *current* label text. +// Deleting an existing row marks it deleted and disables it, with its delete button becoming +// a restore button, rather than removing it outright -- a newly added row has nothing to +// preserve, so deleting one of those does remove it. +class StrategyPanel final : public wxScrolledWindow { +public: + struct Row { + std::string stableLabel; + bool isNew = false; // true for a row added in this dialog, with no prior strategy at all + bool isDeleted = false; // true for an existing row marked for deletion (kept, disabled) + wxString currentLabelText; + LabelTextCtrl *labelCtrl = nullptr; + wxButton *upButton = nullptr; + wxButton *downButton = nullptr; + wxButton *deleteButton = nullptr; // doubles as the restore button when isDeleted + }; + +private: + std::vector m_rows; + wxBoxSizer *m_topSizer; + wxColour m_defaultBg; + std::function m_onChanged; + // False until the constructor completes, so Rebuild()'s initial call can't notify the + // owning dialog through its m_strategyPanel pointer before that's been assigned. + bool m_ready = false; + // True from the moment a row mutation starts until its (deferred) Rebuild() finishes. + // Destroying/rebuilding row controls can fire a focus-kill event on whatever control had + // focus, reentrantly, while m_rows or the controls it points to are mid-mutation; handlers + // that touch either must no-op while this is true rather than act on stale state. + bool m_rebuilding = false; + + std::string NextPlaceholderLabel() const; + int ActiveCount() const + { + return static_cast( + std::count_if(m_rows.begin(), m_rows.end(), [](const Row &r) { return !r.isDeleted; })); + } + void Rebuild(); + // Colours a (non-deleted) row's label to reflect whether it's new, renamed from its + // stable label, or unchanged, and sets/clears the "renamed from" tooltip to match. + static void UpdateRowStyle(Row &row); + void NotifyChanged() const + { + if (m_ready) { + m_onChanged(); + } + } + +public: + StrategyPanel(wxWindow *p_parent, const GamePlayer &p_player, + const std::function &p_onChanged); + + int NumStrategies() const { return static_cast(m_rows.size()); } + bool IsDeleted(int p_index) const { return m_rows.at(p_index).isDeleted; } + std::string GetStableLabel(int p_index) const { return m_rows.at(p_index).stableLabel; } + wxString GetStrategyLabel(int p_index) const + { + return m_rows.at(p_index).labelCtrl->GetNormalizedValue(); + } + + void AddStrategy(); + // Marks an existing row deleted (or restores an already-deleted one); removes a row added + // in this dialog outright. A no-op if this would leave no active (non-deleted) rows. + void ToggleDeleted(int p_index); + void MoveStrategyUp(int p_index); + void MoveStrategyDown(int p_index); + + // Highlights any empty or duplicate strategy labels among active (non-deleted) rows, and + // returns a description of the first problem found, or an empty string if all are valid. + wxString ValidateLabels(); +}; + +StrategyPanel::StrategyPanel(wxWindow *p_parent, const GamePlayer &p_player, + const std::function &p_onChanged) + : wxScrolledWindow(p_parent, wxID_ANY, wxDefaultPosition, wxDefaultSize, + wxVSCROLL | wxTAB_TRAVERSAL), + m_onChanged(p_onChanged) +{ + for (const auto &strategy : p_player->GetStrategies()) { + Row row; + row.stableLabel = strategy->GetLabel(); + row.currentLabelText = wxString::FromUTF8(row.stableLabel); + m_rows.push_back(std::move(row)); + } + + m_topSizer = new wxBoxSizer(wxVERTICAL); + SetSizer(m_topSizer); + SetScrollRate(0, FromDIP(10)); + + Rebuild(); + + const wxSize bestSize = m_topSizer->CalcMin(); + SetMinSize(wxSize(FromDIP(360), std::min(bestSize.GetHeight(), FromDIP(250)))); + + if (!m_rows.empty()) { + m_defaultBg = m_rows.front().labelCtrl->GetBackgroundColour(); + } + + m_ready = true; +} + +std::string StrategyPanel::NextPlaceholderLabel() const +{ + std::set used; + for (const auto &row : m_rows) { + used.insert(row.stableLabel); + used.insert(row.currentLabelText.ToStdString(wxConvUTF8)); + } + int number = static_cast(m_rows.size()) + 1; + while (contains(used, std::to_string(number))) { + number++; + } + return std::to_string(number); +} + +void StrategyPanel::UpdateRowStyle(Row &row) +{ + // Colour alone can be hard to tell apart (or perceive at all); weight/slant give the same + // information a second way, so the state still reads even if the colours don't. + wxFont font = row.labelCtrl->GetFont(); + font.SetWeight(wxFONTWEIGHT_NORMAL); + font.SetStyle(wxFONTSTYLE_NORMAL); + + if (row.isNew) { + font.SetWeight(wxFONTWEIGHT_BOLD); + row.labelCtrl->SetForegroundColour(kNewLabelColour); + row.labelCtrl->UnsetToolTip(); + } + else if (row.currentLabelText.ToStdString(wxConvUTF8) != row.stableLabel) { + font.SetStyle(wxFONTSTYLE_ITALIC); + row.labelCtrl->SetForegroundColour(kRenamedLabelColour); + row.labelCtrl->SetToolTip( + wxString::Format(_("Renamed from \"%s\""), wxString::FromUTF8(row.stableLabel))); + } + else { + row.labelCtrl->SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT)); + row.labelCtrl->UnsetToolTip(); + } + row.labelCtrl->SetFont(font); + row.labelCtrl->Refresh(); +} + +void StrategyPanel::Rebuild() +{ + m_rebuilding = true; + m_topSizer->Clear(true); // destroys the previous row controls; each Row's own state + // (stableLabel, currentLabelText) isn't owned by them + + auto *gridSizer = new wxFlexGridSizer(3, 5, 10); + gridSizer->AddGrowableCol(1, 1); + + gridSizer->AddSpacer(1); + gridSizer->Add(new wxStaticText(this, wxID_STATIC, _("Label")), 0, wxALIGN_CENTER_VERTICAL); + gridSizer->AddSpacer(1); + + const int activeCount = ActiveCount(); + const wxSize buttonSize(FromDIP(28), -1); + + for (size_t i = 0; i < m_rows.size(); i++) { + Row &row = m_rows[i]; + + wxString number; + number << (i + 1); + gridSizer->Add(new wxStaticText(this, wxID_STATIC, number), 0, + wxALIGN_CENTER_VERTICAL | wxALIGN_RIGHT); + + row.labelCtrl = new LabelTextCtrl(this, wxID_ANY, row.currentLabelText); + if (row.isDeleted) { + wxFont strikeFont = row.labelCtrl->GetFont(); + strikeFont.SetStrikethrough(true); + row.labelCtrl->SetFont(strikeFont); + row.labelCtrl->Disable(); + } + else { + UpdateRowStyle(row); + } + row.labelCtrl->Bind(wxEVT_TEXT, [this, i](wxCommandEvent &p_event) { + if (!m_rebuilding && i < m_rows.size()) { + Row &changedRow = m_rows.at(i); + changedRow.currentLabelText = changedRow.labelCtrl->GetValue(); + UpdateRowStyle(changedRow); + NotifyChanged(); + } + p_event.Skip(); + }); + row.labelCtrl->Bind(wxEVT_KILL_FOCUS, [this, i](wxFocusEvent &p_event) { + if (!m_rebuilding && i < m_rows.size()) { + Row &changedRow = m_rows.at(i); + changedRow.currentLabelText = changedRow.labelCtrl->GetNormalizedValue(); + UpdateRowStyle(changedRow); + NotifyChanged(); + } + p_event.Skip(); + }); + gridSizer->Add(row.labelCtrl, 1, wxEXPAND); + + auto *buttonSizer = new wxBoxSizer(wxHORIZONTAL); + + if (!row.isDeleted) { + row.upButton = + new wxButton(this, wxID_ANY, wxUniChar(0x2191), wxDefaultPosition, buttonSize); + row.upButton->Enable(i > 0); + row.upButton->Bind(wxEVT_BUTTON, + [this, i](wxCommandEvent &) { MoveStrategyUp(static_cast(i)); }); + buttonSizer->Add(row.upButton, 0, wxLEFT, 2); + + row.downButton = + new wxButton(this, wxID_ANY, wxUniChar(0x2193), wxDefaultPosition, buttonSize); + row.downButton->Enable(i + 1 < m_rows.size()); + row.downButton->Bind(wxEVT_BUTTON, + [this, i](wxCommandEvent &) { MoveStrategyDown(static_cast(i)); }); + buttonSizer->Add(row.downButton, 0, wxLEFT, 2); + } + else { + // Keep the delete/restore button aligned under its counterpart in other rows, in + // place of the Up/Down buttons a deleted row no longer has. Clear() already + // destroyed the previous Up/Down controls these pointers referred to; null them out + // rather than leave them dangling. + row.upButton = nullptr; + row.downButton = nullptr; + buttonSizer->AddSpacer(buttonSize.GetWidth() + 2); + buttonSizer->AddSpacer(buttonSize.GetWidth() + 2); + } + + row.deleteButton = + new wxButton(this, wxID_ANY, row.isDeleted ? wxUniChar(0x21BA) : wxUniChar(0x2715), + wxDefaultPosition, buttonSize); + row.deleteButton->SetToolTip(row.isDeleted ? _("Restore strategy") : _("Delete strategy")); + row.deleteButton->Enable(row.isDeleted || activeCount > 1); + row.deleteButton->Bind(wxEVT_BUTTON, + [this, i](wxCommandEvent &) { ToggleDeleted(static_cast(i)); }); + buttonSizer->Add(row.deleteButton, 0, wxLEFT, 2); + + gridSizer->Add(buttonSizer, 0, wxALIGN_CENTER_VERTICAL); + } + + // A trailing row with only its button-column cell populated, so the "+" control lands + // directly below the last row's Up/Down/Delete buttons rather than as a separate control + // elsewhere in the dialog. + gridSizer->AddSpacer(1); + gridSizer->AddSpacer(1); + auto *addButton = new wxButton(this, wxID_ANY, "+", wxDefaultPosition, buttonSize); + addButton->SetToolTip(_("Add strategy")); + addButton->Bind(wxEVT_BUTTON, [this](wxCommandEvent &) { AddStrategy(); }); + auto *addSizer = new wxBoxSizer(wxHORIZONTAL); + addSizer->Add(addButton, 0, wxLEFT, 2); + gridSizer->Add(addSizer, 0, wxALIGN_CENTER_VERTICAL); + + m_topSizer->Add(gridSizer, 1, wxALL | wxEXPAND, 5); + FitInside(); + Layout(); + m_rebuilding = false; + // No NotifyChanged() call here -- see m_ready's comment. Callers that mutate m_rows are + // responsible for notifying once Rebuild() returns. +} + +void StrategyPanel::AddStrategy() +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + Row row; + row.stableLabel = NextPlaceholderLabel(); + row.isNew = true; + row.currentLabelText = wxString::FromUTF8(row.stableLabel); + m_rows.push_back(std::move(row)); + // Rebuild() would destroy the very "+" button whose click is invoking this method, while + // its handler is still on the call stack -- unsafe in wx. CallAfter() defers it to once + // this event finishes dispatching; raising m_rebuilding now (not just once Rebuild() + // starts) covers the gap where m_rows is already mutated but the stale old controls are + // still alive. + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +void StrategyPanel::ToggleDeleted(int p_index) +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + Row &row = m_rows.at(p_index); + if (!row.isDeleted && ActiveCount() <= 1) { + return; // refuse to drop below one active strategy + } + if (row.isNew) { + m_rows.erase(m_rows.begin() + p_index); + } + else { + row.isDeleted = !row.isDeleted; + } + // See AddStrategy() for why this defers Rebuild() and raises m_rebuilding immediately. + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +void StrategyPanel::MoveStrategyUp(int p_index) +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + if (p_index <= 0) { + return; + } + std::swap(m_rows[p_index - 1], m_rows[p_index]); + // See AddStrategy() for why this defers Rebuild() and raises m_rebuilding immediately. + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +void StrategyPanel::MoveStrategyDown(int p_index) +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + if (p_index + 1 >= static_cast(m_rows.size())) { + return; + } + std::swap(m_rows[p_index], m_rows[p_index + 1]); + // See AddStrategy() for why this defers Rebuild() and raises m_rebuilding immediately. + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +wxString StrategyPanel::ValidateLabels() +{ + const int numStrategies = NumStrategies(); + wxString message; + + for (int i = 0; i < numStrategies; i++) { + if (m_rows[i].isDeleted) { + continue; + } + const wxString value = m_rows[i].labelCtrl->GetValue(); + bool invalid = value.empty(); + for (int other = 0; !invalid && other < numStrategies; other++) { + if (other != i && !m_rows[other].isDeleted && m_rows[other].labelCtrl->GetValue() == value) { + invalid = true; + } + } + + m_rows[i].labelCtrl->SetBackgroundColour(invalid ? kInvalidLabelBg : m_defaultBg); + m_rows[i].labelCtrl->Refresh(); + + if (invalid && message.empty()) { + message = value.empty() ? _("Strategy labels cannot be empty.") + : _("Strategy labels must be unique for the player."); + } + } + return message; +} + +//====================================================================== +// class EditStrategiesDialog +//====================================================================== + +EditStrategiesDialog::EditStrategiesDialog(wxWindow *p_parent, const GamePlayer &p_player) + : wxDialog( + p_parent, wxID_ANY, + wxString::Format(_("Edit strategies for %s"), wxString::FromUTF8(p_player->GetLabel())), + wxDefaultPosition, wxDefaultSize, wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER), + m_player(p_player) +{ + auto *topSizer = new wxBoxSizer(wxVERTICAL); + + auto *strategyBoxSizer = + new wxStaticBoxSizer(new wxStaticBox(this, wxID_STATIC, _("Strategies")), wxVERTICAL); + // StrategyPanel sizes itself to fit its own content (see its constructor); don't override + // that with a fixed estimate here, which would go stale as soon as the row count changes. + m_strategyPanel = new StrategyPanel(this, p_player, [this]() { UpdateValidation(); }); + + strategyBoxSizer->Add(m_strategyPanel, 1, wxALL | wxEXPAND, 5); + + topSizer->Add(strategyBoxSizer, 1, wxALL | wxEXPAND, 5); + + m_errorText = new wxStaticText(this, wxID_STATIC, wxEmptyString); + m_errorText->SetForegroundColour(*wxRED); + topSizer->Add(m_errorText, 0, wxLEFT | wxRIGHT | wxBOTTOM | wxEXPAND, 5); + + if (auto *buttons = CreateSeparatedButtonSizer(wxOK | wxCANCEL)) { + topSizer->Add(buttons, 0, wxALL | wxEXPAND, 5); + } + + SetSizer(topSizer); + topSizer->SetSizeHints(this); + SetSize(GetBestSize()); + SetMinSize(GetSize()); + CenterOnParent(); + + UpdateValidation(); +} + +void EditStrategiesDialog::UpdateValidation() +{ + const wxString message = m_strategyPanel->ValidateLabels(); + m_errorText->SetLabel(message); + if (auto *ok = FindWindow(wxID_OK)) { + ok->Enable(message.empty()); + } +} + +int EditStrategiesDialog::NumStrategies() const { return m_strategyPanel->NumStrategies(); } + +bool EditStrategiesDialog::IsDeleted(int p_index) const +{ + return m_strategyPanel->IsDeleted(p_index); +} + +std::string EditStrategiesDialog::GetStableLabel(int p_index) const +{ + return m_strategyPanel->GetStableLabel(p_index); +} + +wxString EditStrategiesDialog::GetStrategyLabel(int p_index) const +{ + return m_strategyPanel->GetStrategyLabel(p_index); +} + +} // namespace Gambit::GUI diff --git a/src/gui/dleditstrategies.h b/src/gui/dleditstrategies.h new file mode 100644 index 0000000000..8d037ceb68 --- /dev/null +++ b/src/gui/dleditstrategies.h @@ -0,0 +1,58 @@ +// +// This file is part of Gambit +// Copyright (c) 1994-2026, The Gambit Project (https://www.gambit-project.org) +// +// FILE: src/gui/dleditstrategies.h +// Dialog for viewing and editing the strategies of a player +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation; either version 2 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. +// + +#ifndef GAMBIT_GUI_DLEDITSTRATEGIES_H +#define GAMBIT_GUI_DLEDITSTRATEGIES_H + +#include "editlabel.h" + +namespace Gambit::GUI { +class StrategyPanel; + +class EditStrategiesDialog final : public wxDialog { + GamePlayer m_player; + StrategyPanel *m_strategyPanel; + wxStaticText *m_errorText; + + void UpdateValidation(); + +public: + // Lifecycle + EditStrategiesDialog(wxWindow *p_parent, const GamePlayer &p_player); + + // Data access (only valid when ShowModal() returns with wxID_OK) + // The number of strategy rows currently in the panel, after any adds/deletes/reorders. + // Includes rows marked deleted; check IsDeleted() before committing one. + int NumStrategies() const; + // True if the row at position `p_index` is marked for deletion (kept in the panel, shown + // disabled, but excluded from the operation this dialog commits). + bool IsDeleted(int p_index) const; + // The label the strategy at position `p_index` had (or the placeholder it was assigned, + // if newly added) before any edits made in this dialog. Identifies the strategy for the + // structural part of the operation, independent of anything typed into its label field. + std::string GetStableLabel(int p_index) const; + // The label as currently typed into the row at position `p_index`. + wxString GetStrategyLabel(int p_index) const; +}; +} // namespace Gambit::GUI + +#endif // GAMBIT_GUI_DLEDITSTRATEGIES_H From a8f8fa227d28dda9dd0b46ae6648be77675592d1 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Thu, 20 Aug 2026 10:01:00 +0100 Subject: [PATCH 10/10] Patch to problem in NewPlayer - will be supesded by other work so just temp fixing it here rather than separate commit. --- src/games/gametable.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 43ccbbf131..55811fbcef 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -503,6 +503,7 @@ GamePlayer GameTableRep::NewPlayer(const std::string &p_label) for (const auto &outcome : m_outcomes) { outcome->m_payoffs[player.get()] = Number(); } + IndexStrategies(); return player; }