Correct error in reveal dialog (#687)

This corrects an error in the reveal dialog in which logic was reversed - so player names were not being displayed.

Additionally, this takes the opportunity to model wxWidgets modernisations for better design.

Author: tturocy

Makefile.am

@@ -568,7 +568,6 @@ gambit_SOURCES = \
 	src/gui/dlefglogit.cc \
 	src/gui/dlefglogit.h \
 	src/gui/dlefgreveal.cc \
-	src/gui/dlefgreveal.h \
 	src/gui/dlexcept.h \
 	src/gui/dlgameprop.cc \
 	src/gui/dlgameprop.h \

src/gui/dlefgreveal.cc

@@ -20,69 +20,109 @@
 // Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
 //
 
+#include <optional>
+
 #include <wx/wxprec.h>
 #ifndef WX_PRECOMP
 #include <wx/wx.h>
 #endif // WX_PRECOMP
 
 #include "gambit.h"
-#include "dlefgreveal.h"
+#include "gamedoc.h"
 
-namespace Gambit::GUI {
-//=========================================================================
-//                  RevealMoveDialog: Member functions
-//=========================================================================
+namespace {
+
+using namespace Gambit;
+using namespace Gambit::GUI;
+
+class RevealMoveDialog final : public wxDialog {
+  struct PlayerEntry {
+    GamePlayer player;
+    wxCheckBox *checkbox;
+  };
+  std::vector<PlayerEntry> m_entries;
+
+  void OnCheckbox(wxCommandEvent &) { UpdateButtonState(); }
+  void UpdateButtonState();
 
-RevealMoveDialog::RevealMoveDialog(wxWindow *p_parent, GameDocument *p_doc)
-  : wxDialog(p_parent, wxID_ANY, _("Reveal this move to players"), wxDefaultPosition), m_doc(p_doc)
+public:
+  RevealMoveDialog(wxWindow *p_parent, const Game &p_game);
+  std::vector<GamePlayer> GetPlayers() const;
+};
+
+RevealMoveDialog::RevealMoveDialog(wxWindow *p_parent, const Game &p_game)
+  : wxDialog(p_parent, wxID_ANY, _("Reveal this move to players"), wxDefaultPosition,
+             wxDefaultSize, wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER)
 {
   auto *topSizer = new wxBoxSizer(wxVERTICAL);
 
-  auto *playerBox = new wxStaticBoxSizer(wxHORIZONTAL, this, _("Reveal the move to players"));
+  auto *groupLabel = new wxStaticText(this, wxID_ANY, _("Reveal this move to players"));
+  auto f = groupLabel->GetFont();
+  f.SetWeight(wxFONTWEIGHT_BOLD);
+  groupLabel->SetFont(f);
+  topSizer->Add(groupLabel, wxSizerFlags().Border(wxLEFT | wxTOP | wxRIGHT, 10));
+
+  auto *playerBox = new wxBoxSizer(wxVERTICAL);
+  playerBox->AddSpacer(3);
 
-  auto *boxSizer = new wxBoxSizer(wxVERTICAL);
+  const auto &players = p_game->GetPlayers();
+  m_entries.reserve(players.size());
 
-  for (size_t pl = 1; pl <= m_doc->NumPlayers(); pl++) {
-    auto player = m_doc->GetGame()->GetPlayer(pl);
-    if (player->GetLabel().empty()) {
-      m_players.push_back(
-          new wxCheckBox(this, wxID_ANY, wxString(player->GetLabel().c_str(), *wxConvCurrent)));
+  for (const auto &player : players) {
+    wxString label;
+    if (!player->GetLabel().empty()) {
+      label = wxString::FromUTF8(player->GetLabel());
     }
     else {
-      m_players.push_back(new wxCheckBox(this, wxID_ANY, wxString::Format(_T("Player %d"), pl)));
+      label = wxString::Format("Player %u", player->GetNumber());
     }
-    m_players[pl]->SetValue(true);
-    m_players[pl]->SetForegroundColour(m_doc->GetStyle().GetPlayerColor(player));
-    boxSizer->Add(m_players[pl], 1, wxALL | wxEXPAND, 0);
+    auto *cb = new wxCheckBox(this, wxID_ANY, label);
+    cb->SetValue(true);
+    cb->Bind(wxEVT_CHECKBOX, &RevealMoveDialog::OnCheckbox, this);
+    m_entries.push_back({player, cb});
+    playerBox->Add(cb, wxSizerFlags().Expand().Border(wxLEFT | wxRIGHT | wxTOP, 4));
   }
-  playerBox->Add(boxSizer, 1, wxALL | wxEXPAND, 5);
-  topSizer->Add(playerBox, 1, wxALL | wxEXPAND, 5);
-
-  auto *buttonSizer = new wxBoxSizer(wxHORIZONTAL);
-  buttonSizer->Add(new wxButton(this, wxID_CANCEL, _("Cancel")), 0, wxALL, 5);
-  auto *okButton = new wxButton(this, wxID_OK, _("OK"));
-  okButton->SetDefault();
-  buttonSizer->Add(okButton, 0, wxALL, 5);
-  topSizer->Add(buttonSizer, 0, wxALL | wxALIGN_RIGHT, 5);
-
-  SetSizer(topSizer);
-  topSizer->Fit(this);
-  topSizer->SetSizeHints(this);
-  wxTopLevelWindowBase::Layout();
+
+  topSizer->Add(playerBox, wxSizerFlags(1).Expand().Border(wxALL, 5));
+
+  auto *buttonSizer = CreateStdDialogButtonSizer(wxOK | wxCANCEL);
+  buttonSizer->Realize();
+  topSizer->Add(buttonSizer, wxSizerFlags().Right().Border(wxALL, 10));
+
+  SetSizerAndFit(topSizer);
   CenterOnParent();
+  UpdateButtonState();
 }
 
-Array<GamePlayer> RevealMoveDialog::GetPlayers() const
+void RevealMoveDialog::UpdateButtonState()
 {
-  Array<GamePlayer> players;
+  const bool anyChecked =
+      std::any_of(m_entries.begin(), m_entries.end(),
+                  [](const PlayerEntry &entry) { return entry.checkbox->IsChecked(); });
+  FindWindow(wxID_OK)->Enable(anyChecked);
+}
 
-  for (size_t pl = 1; pl <= m_doc->NumPlayers(); pl++) {
-    if (m_players[pl]->GetValue()) {
-      players.push_back(m_doc->GetGame()->GetPlayer(pl));
+std::vector<GamePlayer> RevealMoveDialog::GetPlayers() const
+{
+  std::vector<GamePlayer> result;
+  result.reserve(m_entries.size());
+  for (const auto &[player, checkbox] : m_entries) {
+    if (checkbox->GetValue()) {
+      result.push_back(player);
     }
   }
+  return result;
+}
+} // anonymous namespace
 
-  return players;
+namespace Gambit::GUI {
+
+std::optional<std::vector<GamePlayer>> RevealMove(wxWindow *p_parent, const Game &p_game)
+{
+  if (RevealMoveDialog dialog(p_parent, p_game); dialog.ShowModal() == wxID_OK) {
+    return dialog.GetPlayers();
+  }
+  return std::nullopt;
 }
 
 } // namespace Gambit::GUI

src/gui/dlefgreveal.h

@@ -21,6 +21,7 @@
 //
 
 #include <fstream>
+#include <optional>
 
 #include <wx/wxprec.h>
 #ifndef WX_PRECOMP
@@ -54,7 +55,6 @@
 #include "dlabout.h"
 
 #include "dlinsertmove.h"
-#include "dlefgreveal.h"
 #include "dleditnode.h"
 #include "dleditmove.h"
 #include "dlefglayout.h"
@@ -978,14 +978,15 @@ void GameFrame::OnEditRemoveOutcome(wxCommandEvent &)
   }
 }
 
+std::optional<std::vector<GamePlayer>> RevealMove(wxWindow *p_parent, const Game &p_game);
+
 void GameFrame::OnEditReveal(wxCommandEvent &)
 {
-  RevealMoveDialog dialog(this, m_doc);
-
-  if (dialog.ShowModal() == wxID_OK) {
+  if (const auto players = RevealMove(this, m_doc->GetGame()); players) {
     try {
-      for (const auto &player : dialog.GetPlayers()) {
-        m_doc->DoRevealAction(m_doc->GetSelectNode()->GetInfoset(), player);
+      const auto &infoset = m_doc->GetSelectNode()->GetInfoset();
+      for (const auto &player : *players) {
+        m_doc->DoRevealAction(infoset, player);
       }
     }
     catch (std::exception &ex) {