Merge pull request #848 from matty0ung/usability

Fixes for adding mash/boil/fermentation steps
Brewtarget · Oct 5, 2024 · 339d2f2 · 339d2f2
2 parents 372c580 + dd12ee1
commit 339d2f2
Show file tree

Hide file tree

Showing 23 changed files with 421 additions and 303 deletions.
diff --git a/.github/workflows/linux-ubuntu.yml b/.github/workflows/linux-ubuntu.yml
@@ -63,7 +63,7 @@ jobs:
       # have only Python 3.6.7 and Ubuntu 20.04 only have Python 3.8.2.  However Ubuntu 22.04 has Python 3.10.6.)  There
       # are ways to get around this, but, in this context, it's simplest to use a canned GitHub action.
       #
-      - uses: actions/setup-python@v4
+      - uses: actions/setup-python@v5
         with:
           python-version: '3.10'
       - name: Install Frameworks and Libraries, and set up Meson build environment
@@ -88,6 +88,7 @@ jobs:
             $GITHUB_WORKSPACE
 
       - name: Build (with Meson)
+        id: meson-build
         working-directory: ${{github.workspace}}/mbuild
         shell: bash
         run: |
@@ -98,6 +99,7 @@ jobs:
       # The 'export QT_QPA_PLATFORM=offscreen' stops Qt's xcb sub-module trying to connect to a non-existent display
       # (which would cause the test runner to abort before running any tests).
       - name: Test (via Meson)
+        id: meson-test
         working-directory: ${{github.workspace}}/mbuild
         shell: bash
         run: |
@@ -106,12 +108,14 @@ jobs:
           meson test
 
       - name: Build (with CMake)
+        id: cmake-build
         working-directory: ${{github.workspace}}/build
         shell: bash
         run: |
           make
 
       - name: Test (via CMake)
+        id: cmake-test
         working-directory: ${{github.workspace}}/build
         shell: bash
         env:
@@ -143,20 +147,20 @@ jobs:
             mbuild/packages/linux/brewtarget*.rpm.sha256sum
           retention-days: 7
 
-      - name: Recover Debris Artifacts (CMake)
-        if: ${{ failure() }}
+      - name: Recover Debris Artifacts (Meson)
+        if: ${{ failure() && (steps.meson-build.conclusion == 'failure' || steps.meson-test.conclusion == 'failure') }}
         uses: actions/upload-artifact@v4
         with:
-          name: build-results-${{matrix.os}}
-          path: build
+          name: mbuild-results-${{matrix.os}}
+          path: mbuild
           retention-days: 1
 
-      - name: Recover Debris Artifacts (Meson)
-        if: ${{ failure() }}
+      - name: Recover Debris Artifacts (CMake)
+        if: ${{ failure() && (steps.cmake-build.conclusion == 'failure' || steps.cmake-test.conclusion == 'failure') }}
         uses: actions/upload-artifact@v4
         with:
-          name: mbuild-results-${{matrix.os}}
-          path: mbuild
+          name: build-results-${{matrix.os}}
+          path: build
           retention-days: 1
 
       # Meson test doesn't show log output on the terminal, but puts it straight to a log file.  We don't want to have

diff --git a/CHANGES.markdown b/CHANGES.markdown
@@ -20,10 +20,12 @@ Bug fixes for the 4.0.5 release (ie bugs in 4.0.5 are fixed in this 4.0.6 releas
 * None
 
 ### Bug Fixes
+* Can't add fermentation steps [831](https://github.com/Brewtarget/brewtarget/issues/831)
 * Ingredient inventory edits not saved [832](https://github.com/Brewtarget/brewtarget/issues/832)
 * Unsatisfied dependency for Brewtarget update in ubuntu 24.01 [840](https://github.com/Brewtarget/brewtarget/issues/840)
 * Cmake error on Linux Mint 22 Wilma [843](https://github.com/Brewtarget/brewtarget/issues/843)
 * Binaries are not signed on Windows [827](https://github.com/Brewtarget/brewtarget/issues/827)
+* Adding Mash Step causes core dump [847](https://github.com/Brewtarget/brewtarget/issues/847)
 
 ### Release Timestamp
 Wed, 2 Oct 2024 04:00:06 +0100

diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
@@ -535,29 +535,6 @@ class MainWindow::impl {
       return;
    }
 
-   template<class StepOwnerClass, class StepClass>
-   void addStepToStepOwner(std::shared_ptr<StepOwnerClass> stepOwner, std::shared_ptr<StepClass> step) {
-      qDebug() << Q_FUNC_INFO;
-      //
-      // Mash/Boil/Fermentation Steps are a bit different from most other NamedEntity objects in that they don't really
-      // have an independent existence.  Taking Mash as an example, if you ask a Mash to remove a MashStep then it will
-      // also tell the ObjectStore to delete it, but, when we're adding a MashStep to a Mash it's easier (for eg the
-      // implementation of undo/redo) if we add it to the ObjectStore before we call Mash::addMashStep().
-      //
-      ObjectStoreWrapper::insert(step);
-      this->m_self.doOrRedoUpdate(
-         newUndoableAddOrRemove(*stepOwner,
-                                &StepOwnerClass::addStep,
-                                step,
-                                &StepOwnerClass::removeStep,
-                                tr("Add %1 step to recipe").arg(StepOwnerClass::localisedName()))
-      );
-      // We don't need to call this->pimpl->m_mashStepTableModel->addMashStep(mashStep) etc here because the change to
-      // the mash/boil/ferementation will already have triggered the necessary updates to
-      // this->pimpl->m_mashStepTableModel/this->pimpl->m_boilStepTableModel/etc.
-      return;
-   }
-
 //   template<class StepClass> auto & getStepTableModel() const;
 //   template<std::same_as<MashStep        > T> auto & getTableModel<T>() const { return *this->        m_mashStepTableModel; }
 //   template<std::same_as<BoilStep        > T> auto & getTableModel<T>() const { return *this->        m_boilStepTableModel; }
@@ -585,18 +562,21 @@ class MainWindow::impl {
    void setStepOwner(std::shared_ptr<Mash> stepOwner) {
       this->m_recipeObs->setMash(stepOwner);
       this->m_mashStepTableModel->setMash(stepOwner);
+      this->m_mashStepEditor->setStepOwner(stepOwner);
       this->m_self.mashButton->setMash(stepOwner);
       return;
    }
    void setStepOwner(std::shared_ptr<Boil> stepOwner) {
       this->m_recipeObs->setBoil(stepOwner);
       this->m_boilStepTableModel->setBoil(stepOwner);
+      this->m_boilStepEditor->setStepOwner(stepOwner);
       this->m_self.boilButton->setBoil(stepOwner);
       return;
    }
    void setStepOwner(std::shared_ptr<Fermentation> stepOwner) {
       this->m_recipeObs->setFermentation(stepOwner);
       this->m_fermentationStepTableModel->setFermentation(stepOwner);
+      this->m_fermentationStepEditor->setStepOwner(stepOwner);
       this->m_self.fermentationButton->setFermentation(stepOwner);
       return;
    }
@@ -616,13 +596,17 @@ class MainWindow::impl {
 
       std::shared_ptr<typename StepClass::StepOwnerClass> stepOwner =
          this->m_recipeObs->get<typename StepClass::StepOwnerClass>();
-      if (!stepOwner) {
+      if (stepOwner) {
+         // This seems a bit circular, but guarantees that the editor knows to which step owner (eg Mash) the new step
+         // (eg MashStep) should be added.
+         this->setStepOwner(stepOwner);
+      } else {
          auto defaultStepOwner = std::make_shared<typename StepClass::StepOwnerClass>();
          ObjectStoreWrapper::insert(defaultStepOwner);
          this->setStepOwner(defaultStepOwner);
       }
 
-      // This ultimately gets stored in MainWindow::addMashStepToMash() etc
+      // This ultimately gets stored in MainWindow::addStepToStepOwner() etc
       auto step = std::make_shared<StepClass>("");
       this->showStepEditor(step);
       return;
@@ -2360,16 +2344,50 @@ void MainWindow::removeSelectedYeastAddition() {
    return;
 }
 
-void MainWindow::addMashStepToMash(std::shared_ptr<MashStep> mashStep) {
-   this->pimpl->addStepToStepOwner(this->pimpl->m_recipeObs->mash(), mashStep);
+
+template<class StepOwnerClass, class StepClass>
+void MainWindow::addStepToStepOwner(StepOwnerClass & stepOwner, std::shared_ptr<StepClass> step) {
+   qDebug() << Q_FUNC_INFO;
+   //
+   // Mash/Boil/Fermentation Steps are a bit different from most other NamedEntity objects in that they don't really
+   // have an independent existence.  Taking Mash as an example, if you ask a Mash to remove a MashStep then it will
+   // also tell the ObjectStore to delete it, but, when we're adding a MashStep to a Mash it's easier (for eg the
+   // implementation of undo/redo) if we add it to the ObjectStore before we call Mash::addMashStep().
+   //
+   // However, normally, at this point, the new step will already have been added to the DB by
+   // EditorBase::doSaveAndClose.  So we are just belt-and-braces here checking whether it needs to be added.
+   //
+   if (step->key() < 0) {
+      qWarning() << Q_FUNC_INFO << step->metaObject()->className() << "unexpectedly not in DB, so inserting it now.";
+      ObjectStoreWrapper::insert(step);
+   }
+   this->doOrRedoUpdate(
+      newUndoableAddOrRemove(stepOwner,
+                             &StepOwnerClass::addStep,
+                             step,
+                             &StepOwnerClass::removeStep,
+                             tr("Add %1 step to recipe").arg(StepOwnerClass::localisedName()))
+   );
+   // We don't need to call this->pimpl->m_mashStepTableModel->addMashStep(mashStep) etc here because the change to
+   // the mash/boil/ferementation will already have triggered the necessary updates to
+   // this->pimpl->m_mashStepTableModel/this->pimpl->m_boilStepTableModel/etc.
+   return;
+}
+
+template<class StepOwnerClass, class StepClass>
+void MainWindow::addStepToStepOwner(std::shared_ptr<StepOwnerClass> stepOwner, std::shared_ptr<StepClass> step) {
+   this->addStepToStepOwner(*stepOwner, step);
+}
+void MainWindow::addStepToStepOwner(std::shared_ptr<MashStep> mashStep) {
+   this->addStepToStepOwner(this->pimpl->m_recipeObs->mash(), mashStep);
    return;
 }
-void MainWindow::addBoilStepToBoil(std::shared_ptr<BoilStep> boilStep) {
-   this->pimpl->addStepToStepOwner(this->pimpl->m_recipeObs->boil(), boilStep);
+void MainWindow::addStepToStepOwner(std::shared_ptr<BoilStep> boilStep) {
+   this->addStepToStepOwner(this->pimpl->m_recipeObs->boil(), boilStep);
    return;
 }
-void MainWindow::addFermentationStepToFermentation(std::shared_ptr<FermentationStep> fermentationStep) {
-   this->pimpl->addStepToStepOwner(this->pimpl->m_recipeObs->fermentation(), fermentationStep);
+void MainWindow::addStepToStepOwner(std::shared_ptr<FermentationStep> fermentationStep) {
+   this->addStepToStepOwner(this->pimpl->m_recipeObs->fermentation(), fermentationStep);
    return;
 }
 

diff --git a/src/MainWindow.h b/src/MainWindow.h
@@ -126,6 +126,15 @@ class MainWindow : public QMainWindow, public Ui::mainWindow {
     */
    template<class NE> void addIngredientToRecipe(NE & ne);
 
+   //! \brief Actually add the new mash step to (the mash of) the recipe (in an undoable way).
+   template<class StepOwnerClass, class StepClass>
+   void addStepToStepOwner(StepOwnerClass & stepOwner, std::shared_ptr<StepClass> step);
+   template<class StepOwnerClass, class StepClass>
+   void addStepToStepOwner(std::shared_ptr<StepOwnerClass> stepOwner, std::shared_ptr<StepClass> step);
+   void addStepToStepOwner(std::shared_ptr<MashStep        > step);
+   void addStepToStepOwner(std::shared_ptr<BoilStep        > step);
+   void addStepToStepOwner(std::shared_ptr<FermentationStep> step);
+
 public slots:
 
    //! \brief Accepts Recipe changes, and takes appropriate action to show the changes.
@@ -195,11 +204,6 @@ public slots:
    void addBoilStep();
    void addFermentationStep();
 
-   //! \brief Actually add the new mash step to (the mash of) the recipe (in an undoable way).
-   void addMashStepToMash                (std::shared_ptr<MashStep        > step);
-   void addBoilStepToBoil                (std::shared_ptr<BoilStep        > step);
-   void addFermentationStepToFermentation(std::shared_ptr<FermentationStep> step);
-
    //! \brief Move currently selected mash step down.
    void moveSelectedMashStepUp();
    void moveSelectedBoilStepUp();

diff --git a/src/database/ObjectStore.cpp b/src/database/ObjectStore.cpp
@@ -1183,6 +1183,8 @@ class ObjectStore::impl {
       qDebug() <<
          Q_FUNC_INFO << "Inserting" << object.metaObject()->className() << "main table row with database query " <<
          queryString;
+      // Uncomment the following to track down errors where we're trying to insert an object to the database twice
+//      qDebug().noquote() << Q_FUNC_INFO << Logging::getStackTrace();
 
       //
       // Bind the values

diff --git a/src/editors/BoilStepEditor.cpp b/src/editors/BoilStepEditor.cpp
@@ -20,6 +20,7 @@
 
 BoilStepEditor::BoilStepEditor(QWidget* parent, QString const editorName) :
    QDialog{parent},
+   StepEditorBase<BoilStepEditor, BoilStep>{},
    EditorBase<BoilStepEditor, BoilStep, BoilStepEditorOptions>(editorName) {
    this->setupUi(this);
    this->postSetupUiInit({

diff --git a/src/editors/BoilStepEditor.h b/src/editors/BoilStepEditor.h
@@ -32,6 +32,7 @@
  */
 class BoilStepEditor : public QDialog,
                        public Ui::boilStepEditor,
+                       public StepEditorBase<BoilStepEditor, BoilStep>,
                        public EditorBase<BoilStepEditor, BoilStep, BoilStepEditorOptions> {
    Q_OBJECT
 

diff --git a/src/editors/EditorBase.h b/src/editors/EditorBase.h
@@ -17,6 +17,7 @@
 #define EDITORS_EDITORBASE_H
 #pragma once
 
+#include <concepts>
 #include <memory>
 #include <variant>
 #include <vector>
@@ -29,10 +30,52 @@
 #include "BtHorizontalTabs.h"
 #include "database/ObjectStoreWrapper.h"
 #include "editors/EditorBaseField.h"
+#include "MainWindow.h"
 #include "model/NamedEntity.h"
 #include "model/Recipe.h" // Need to include this this to be able to cast Recipe to QObject
 #include "utils/CuriouslyRecurringTemplateBase.h"
 
+/**
+ * \brief Extra base class for \c MashStepEditor, \c BoilStepEditor and \c FermentationStepEditor to inherit from
+ *        \b before inheriting from \c EditorBase.  This provides the functionality that, if we are creating a new step,
+ *        rather than editing an existing one, then we need to be able to set the owner (ie the \c Mash, \c Boil or
+ *        \c Fermentation) when we click \b Save, because the steps have no independent existence without their owner.
+ */
+template<class Derived> class StepEditorPhantom;
+template<class Derived, class NE>
+class StepEditorBase : public CuriouslyRecurringTemplateBase<StepEditorPhantom, Derived> {
+public:
+   StepEditorBase() :
+      m_stepOwner{nullptr} {
+      return;
+   }
+
+   void setStepOwner(std::shared_ptr<typename NE::OwnerClass> stepOwner) {
+      this->m_stepOwner = stepOwner;
+      return;
+   }
+
+   /**
+    * \brief If the step being edited is new, then, when save is clicked, this should be called
+    */
+   void addStepToStepOwner(std::shared_ptr<NE> step) {
+      //
+      // Going via MainWindow makes the action undoable
+      //
+      Q_ASSERT(this->m_stepOwner);
+      MainWindow::instance().addStepToStepOwner(*this->m_stepOwner, step);
+      return;
+   }
+
+   /**
+    * \brief I tried putting this in \c EditorBase and having \c NE::OwnerClass eavluated only when it is valid  (ie
+    *        when HasStepOwner<editorBaseOptions> is true), and some replaced with some dummy type otherwise.  This
+    *        seems quite to do.  Eg \c std::conditional requires both its class parameters to be valid.  So an extra
+    *        CRTP base class was born.
+    */
+   std::shared_ptr<typename NE::OwnerClass> m_stepOwner;
+};
+
 /**
  * \brief This is used as a template parameter to turn on and off various \b small features in \c EditorBase (in
  *        conjunction with the concepts defined below).
@@ -337,6 +380,16 @@ class EditorBase : public CuriouslyRecurringTemplateBase<EditorPhantom, Derived>
       return true;
    }
 
+   //! No-op version
+   void updateStepOwnerIfNeeded() requires (!std::is_base_of<StepEditorBase<Derived, NE>, Derived>::value) {
+      return;
+   }
+   //! Substantive version
+   void updateStepOwnerIfNeeded() requires (std::is_base_of<StepEditorBase<Derived, NE>, Derived>::value) {
+      this->derived().addStepToStepOwner(this->m_editItem);
+      return;
+   }
+
    /**
     * \brief Subclass should call this from its \c save slot
     */
@@ -357,6 +410,7 @@ class EditorBase : public CuriouslyRecurringTemplateBase<EditorPhantom, Derived>
          ObjectStoreWrapper::insert(this->m_editItem);
       }
       this->writeLateFieldsToEditItem();
+      this->updateStepOwnerIfNeeded();
 
       this->derived().setVisible(false);
       return;

diff --git a/src/editors/FermentationStepEditor.cpp b/src/editors/FermentationStepEditor.cpp
@@ -20,6 +20,7 @@
 
 FermentationStepEditor::FermentationStepEditor(QWidget* parent, QString const editorName) :
    QDialog{parent},
+   StepEditorBase<FermentationStepEditor, FermentationStep>{},
    EditorBase<FermentationStepEditor, FermentationStep, FermentationStepEditorOptions>(editorName) {
    this->setupUi(this);
    this->postSetupUiInit({

diff --git a/src/editors/FermentationStepEditor.h b/src/editors/FermentationStepEditor.h
@@ -32,6 +32,7 @@
  */
 class FermentationStepEditor : public QDialog,
                                public Ui::fermentationStepEditor,
+                               public StepEditorBase<FermentationStepEditor, FermentationStep>,
                                public EditorBase<FermentationStepEditor, FermentationStep, FermentationStepEditorOptions> {
    Q_OBJECT
 

diff --git a/src/editors/MashStepEditor.cpp b/src/editors/MashStepEditor.cpp
@@ -23,7 +23,8 @@
 
 MashStepEditor::MashStepEditor(QWidget* parent, QString const editorName) :
    QDialog{parent},
-   EditorBase<MashStepEditor, MashStep, MashStepEditorOptions>(editorName) {
+   StepEditorBase<MashStepEditor, MashStep>{},
+   EditorBase<MashStepEditor, MashStep, MashStepEditorOptions>{editorName} {
    this->setupUi(this);
    this->postSetupUiInit({
       //

diff --git a/src/editors/MashStepEditor.h b/src/editors/MashStepEditor.h
@@ -35,6 +35,7 @@
  */
 class MashStepEditor : public QDialog,
                        public Ui::mashStepEditor,
+                       public StepEditorBase<MashStepEditor, MashStep>,
                        public EditorBase<MashStepEditor, MashStep, MashStepEditorOptions> {
    Q_OBJECT