Fixed crash on a divisional coupler (GrandOrgue#1891)

src/grandorgue/combinations/GODivisionalSetter.cpp

@@ -1,6 +1,6 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
@@ -349,10 +349,10 @@ void GODivisionalSetter::SwitchDivisionalTo(
     // whether the combination is defined
     bool isExist = divMap.find(divisionalIdx) != divMap.end();
     GODivisionalCombination *pCmb = isExist ? divMap[divisionalIdx] : nullptr;
+    const unsigned manualIndex = m_FirstManualIndex + manualN;
 
     if (!isExist && r_SetterState.m_IsActive) {
       // create a new combination
-      const unsigned manualIndex = m_FirstManualIndex + manualN;
 
       pCmb = new GODivisionalCombination(*m_OrganController, manualIndex, true);
       pCmb->Init(
@@ -362,8 +362,11 @@ void GODivisionalSetter::SwitchDivisionalTo(
 
     if (pCmb)
       // the combination was existing or has just been created
-      m_OrganController->GetSetter()->PushDivisional(
-        *pCmb, m_buttons[N_BUTTONS * manualN + divisionalN]);
+      m_OrganController->PushDivisional(
+        *pCmb,
+        manualIndex,
+        manualIndex,
+        m_buttons[N_BUTTONS * manualN + divisionalN]);
   }
 }
 

src/grandorgue/combinations/GOSetter.cpp

@@ -1015,73 +1015,21 @@ void GOSetter::PushGeneral(
 }
 
 void GOSetter::PushDivisional(
-  std::unordered_set<GODivisionalCombination *> &usedCmbs,
   GODivisionalCombination &cmb,
+  unsigned startManual,
+  unsigned cmbManual,
   GOButtonControl *pButtonToLight) {
-  // protection against infinite recursion
-  if (usedCmbs.insert(&cmb).second) {
+  if (cmbManual == startManual || !m_state.m_IsActive) {
     GOCombination::ExtraElementsSet elementSet;
     const GOCombination::ExtraElementsSet *pExtraSet
       = GetCrescendoAddSet(elementSet);
 
     NotifyCmbPushed(cmb.Push(m_state, pExtraSet));
-    /* only use divisional couples, if not in setter mode */
-    if (!m_state.m_IsActive) {
-      unsigned cmbManualNumber = cmb.GetManualNumber();
-      unsigned divisionalNumber = cmb.GetDivisionalNumber();
-
-      for (unsigned k = 0; k < m_OrganController->GetDivisionalCouplerCount();
-           k++) {
-        GODivisionalCoupler *coupler
-          = m_OrganController->GetDivisionalCoupler(k);
-        if (!coupler->IsEngaged())
-          continue;
-
-        for (unsigned i = 0; i < coupler->GetNumberOfManuals(); i++) {
-          if (coupler->GetManual(i) != cmbManualNumber)
-            continue;
-
-          for (unsigned int j = i + 1; j < coupler->GetNumberOfManuals(); j++) {
-            GODivisionalButtonControl *coupledDivisional
-              = m_OrganController->GetManual(coupler->GetManual(j))
-                  ->GetDivisional(divisionalNumber);
-
-            PushDivisional(
-              usedCmbs, coupledDivisional->GetCombination(), coupledDivisional);
-          }
-
-          if (coupler->IsBidirectional())
-            for (unsigned j = 0; j < coupler->GetNumberOfManuals(); j++) {
-              unsigned coupledManualIndex = coupler->GetManual(j);
-
-              if (coupledManualIndex == cmbManualNumber)
-                break;
-
-              GODivisionalButtonControl *coupledDivisional
-                = m_OrganController->GetManual(coupledManualIndex)
-                    ->GetDivisional(divisionalNumber);
-
-              PushDivisional(
-                usedCmbs,
-                coupledDivisional->GetCombination(),
-                coupledDivisional);
-            }
-          break;
-        }
-      }
-    }
     if (!pExtraSet)
-      UpdateAllSetsButtonsLight(pButtonToLight, cmb.GetManualNumber());
+      UpdateAllSetsButtonsLight(pButtonToLight, cmbManual);
   }
 }
 
-void GOSetter::PushDivisional(
-  GODivisionalCombination &cmb, GOButtonControl *pButtonToLight) {
-  std::unordered_set<GODivisionalCombination *> usedCmbs;
-
-  PushDivisional(usedCmbs, cmb, pButtonToLight);
-}
-
 void GOSetter::Next() { SetPosition(m_pos + 1); }
 
 void GOSetter::Prev() { SetPosition(m_pos - 1); }

src/grandorgue/combinations/GOSetter.h

@@ -200,11 +200,10 @@ class GOSetter : private GOSoundStateHandler,
   void PushGeneral(
     GOGeneralCombination &cmb, GOButtonControl *pButtonToLight) override;
   void PushDivisional(
-    std::unordered_set<GODivisionalCombination *> &usedCmbs,
     GODivisionalCombination &cmb,
-    GOButtonControl *pButtonToLight);
-  void PushDivisional(
-    GODivisionalCombination &cmb, GOButtonControl *pButtonToLight) override;
+    unsigned startManual,
+    unsigned cmbManual,
+    GOButtonControl *pButtonToLight) override;
 
   /*
    * If current crescendo is in override mode then returns nullptr

src/grandorgue/combinations/control/GOCombinationController.h

@@ -1,6 +1,6 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
@@ -21,14 +21,27 @@ class GOCombinationController {
    * If isFromCrescendo and it is in add mode then then does not depress other
    * buttons
    * @param cmb the cmb to activate or to set
-   * @param pButton the button to light on
-   * return if anything is changed
+   * @param pButtonToLight the button to light on. All other buttons are
+   *   ligthed off
    */
   virtual void PushGeneral(
     GOGeneralCombination &cmb, GOButtonControl *pButtonToLight)
     = 0;
+
+  /**
+   * Activate the divisional cmb. Does not process divisional couplers
+   * @param cmb the cmb to activate or to set
+   * @param startManual the manual that is pushed manually.
+   * @param cmbManual the manual where to push the divisional. It may differ
+   *   from startManual if cmbManual has a divisional coupler with startManual
+   * @param pButtonToLight the button to light on. All other buttons on the
+   *   cmbManual are ligthed off
+   */
   virtual void PushDivisional(
-    GODivisionalCombination &cmb, GOButtonControl *pButtonToLight)
+    GODivisionalCombination &cmb,
+    unsigned startManual,
+    unsigned cmbManual,
+    GOButtonControl *pButtonToLight)
     = 0;
 };
 

src/grandorgue/combinations/control/GOCombinationControllerProxy.cpp

@@ -1,6 +1,6 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
@@ -14,7 +14,10 @@ void GOCombinationControllerProxy::PushGeneral(
 }
 
 void GOCombinationControllerProxy::PushDivisional(
-  GODivisionalCombination &cmb, GOButtonControl *pButtonToLight) {
+  GODivisionalCombination &cmb,
+  unsigned startManual,
+  unsigned cmbManual,
+  GOButtonControl *pButtonToLight) {
   if (p_controller)
-    p_controller->PushDivisional(cmb, pButtonToLight);
+    p_controller->PushDivisional(cmb, startManual, cmbManual, pButtonToLight);
 }

src/grandorgue/combinations/control/GOCombinationControllerProxy.h

@@ -1,6 +1,6 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
@@ -22,7 +22,10 @@ class GOCombinationControllerProxy : public GOCombinationController {
   void PushGeneral(
     GOGeneralCombination &cmb, GOButtonControl *pButtonToLight) override;
   void PushDivisional(
-    GODivisionalCombination &cmb, GOButtonControl *pButtonToLight) override;
+    GODivisionalCombination &cmb,
+    unsigned startManual,
+    unsigned cmbManual,
+    GOButtonControl *pButtonToLight) override;
 };
 
 #endif /* GOCOMBINATIONCONTROLLERPROXY_H */

src/grandorgue/combinations/control/GODivisionalButtonControl.cpp

@@ -1,6 +1,6 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
@@ -9,14 +9,16 @@
 
 #include "wx/intl.h"
 
-#include "combinations/GOSetter.h"
+#include "model/GOManual.h"
 #include "model/GOOrganModel.h"
 
 GODivisionalButtonControl::GODivisionalButtonControl(
-  GOOrganModel &organModel, unsigned manualNumber, bool isSetter)
+  GOOrganModel &organModel, unsigned manualNumber, unsigned divisionalIndex)
   : GOPushbuttonControl(organModel),
     r_OrganModel(organModel),
-    m_combination(organModel, manualNumber, isSetter) {}
+    m_ManualN(manualNumber),
+    m_DivisionalIndex(divisionalIndex),
+    m_combination(organModel, manualNumber, false) {}
 
 const wxString WX_MIDI_TYPE_CODE = wxT("Divisional");
 const wxString WX_MIDI_TYPE = _("Divisional");
@@ -30,17 +32,14 @@ const wxString &GODivisionalButtonControl::GetMidiType() const {
 }
 
 void GODivisionalButtonControl::Init(
-  GOConfigReader &cfg,
-  const wxString &group,
-  int divisionalNumber,
-  const wxString &name) {
+  GOConfigReader &cfg, const wxString &group, const wxString &name) {
   GOPushbuttonControl::Init(cfg, group, name);
-  m_combination.Init(group, divisionalNumber);
+  m_combination.Init(group, m_DivisionalIndex);
 }
 void GODivisionalButtonControl::Load(
-  GOConfigReader &cfg, const wxString &group, int divisionalNumber) {
+  GOConfigReader &cfg, const wxString &group) {
   GOPushbuttonControl::Load(cfg, group);
-  m_combination.Load(cfg, group, divisionalNumber);
+  m_combination.Load(cfg, group, m_DivisionalIndex);
 }
 
 void GODivisionalButtonControl::LoadCombination(GOConfigReader &cfg) {
@@ -53,5 +52,19 @@ void GODivisionalButtonControl::Save(GOConfigWriter &cfg) {
 }
 
 void GODivisionalButtonControl::Push() {
-  r_OrganModel.PushDivisional(m_combination, this);
+  for (unsigned coupledManualN :
+       r_OrganModel.GetCoupledManualsForDivisional(m_ManualN)) {
+    GOManual *pManual = r_OrganModel.GetManual(coupledManualN);
+
+    if (m_DivisionalIndex < pManual->GetDivisionalCount()) {
+      GODivisionalButtonControl *pCoupledButton
+        = pManual->GetDivisional(m_DivisionalIndex);
+
+      r_OrganModel.PushDivisional(
+        pCoupledButton->GetCombination(),
+        m_ManualN,
+        coupledManualN,
+        pCoupledButton);
+    }
+  }
 }

src/grandorgue/combinations/control/GODivisionalButtonControl.h

@@ -1,6 +1,6 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
@@ -16,26 +16,32 @@
 class GOConfigReader;
 class GOOrganModel;
 
+/**
+ * A divisional combination button for a manual. It is used only for odf-defined
+ * (old style) divisionals and not for setter (banked) divisionals
+ */
 class GODivisionalButtonControl : public GOPushbuttonControl {
 private:
   GOOrganModel &r_OrganModel;
+
+  // A manual the divisional button belongs to
+  unsigned m_ManualN;
+  // An index of divisionals in this manual
+  unsigned m_DivisionalIndex;
+  // A combination for this divisional
   GODivisionalCombination m_combination;
 
 public:
   GODivisionalButtonControl(
-    GOOrganModel &organModel, unsigned manualNumber, bool isSetter);
+    GOOrganModel &organModel, unsigned manualNumber, unsigned divisionalIndex);
 
   GODivisionalCombination &GetCombination() { return m_combination; }
   const wxString &GetMidiTypeCode() const override;
   const wxString &GetMidiType() const override;
 
-  void Init(
-    GOConfigReader &cfg,
-    const wxString &group,
-    int divisionalNumber,
-    const wxString &name);
+  void Init(GOConfigReader &cfg, const wxString &group, const wxString &name);
 
-  void Load(GOConfigReader &cfg, const wxString &group, int divisionalNumber);
+  void Load(GOConfigReader &cfg, const wxString &group);
 
   void LoadCombination(GOConfigReader &cfg);
   void Save(GOConfigWriter &cfg);

src/grandorgue/model/GODivisionalCoupler.cpp

@@ -1,18 +1,24 @@
 /*
  * Copyright 2006 Milan Digital Audio LLC
- * Copyright 2009-2023 GrandOrgue contributors (see AUTHORS)
+ * Copyright 2009-2024 GrandOrgue contributors (see AUTHORS)
  * License GPL-2.0 or later
  * (https://www.gnu.org/licenses/old-licenses/gpl-2.0.html).
  */
 
 #include "GODivisionalCoupler.h"
 
+#include <algorithm>
+
 #include <wx/intl.h>
 
 #include "config/GOConfigReader.h"
 
 #include "GOOrganModel.h"
 
+const wxString GODivisionalCoupler::WX_MIDI_TYPE_CODE
+  = wxT("DivisionalCoupler");
+const wxString GODivisionalCoupler::WX_MIDI_TYPE_DESC = _("Divisional Coupler");
+
 GODivisionalCoupler::GODivisionalCoupler(GOOrganModel &organModel)
   : GODrawstop(organModel), m_BiDirectionalCoupling(false), m_manuals(0) {}
 
@@ -47,23 +53,23 @@ void GODivisionalCoupler::SetupIsToStoreInCmb() {
   m_IsToStoreInGeneral = r_OrganModel.GeneralsStoreDivisionalCouplers();
 }
 
-void GODivisionalCoupler::ChangeState(bool on) {}
-
-unsigned GODivisionalCoupler::GetNumberOfManuals() { return m_manuals.size(); }
-
-unsigned GODivisionalCoupler::GetManual(unsigned index) {
-  return m_manuals[index];
-}
-
-bool GODivisionalCoupler::IsBidirectional() { return m_BiDirectionalCoupling; }
+std::set<unsigned> GODivisionalCoupler::GetCoupledManuals(
+  unsigned startManual) const {
+  std::set<unsigned> res;
 
-const wxString WX_MIDI_TYPE_CODE = wxT("DivisionalCoupler");
-const wxString WX_MIDI_TYPE = _("Divisional Coupler");
+  if (IsEngaged()) {
+    auto firstIt = m_manuals.cbegin();
+    auto lastIt = m_manuals.cend();
+    auto startIt = std::find(firstIt, lastIt, startManual);
 
-const wxString &GODivisionalCoupler::GetMidiTypeCode() const {
-  return WX_MIDI_TYPE_CODE;
-}
+    if (startIt != lastIt) { // startManual participates in the coupler
+      auto copyIt = m_BiDirectionalCoupling ? firstIt : startIt;
 
-const wxString &GODivisionalCoupler::GetMidiType() const {
-  return WX_MIDI_TYPE;
+      std::copy_if(
+        copyIt, lastIt, std::inserter(res, res.begin()), [=](auto x) {
+          return x != startManual;
+        });
+    }
+  }
+  return res;
 }