Fix for Brewtarget#685

src/WaterDialog.cpp

@@ -348,8 +348,7 @@ void WaterDialog::newTotals() {
       }
       btDigit_ph->setText( calculateMashpH(), 2 );
 
-   }
-   else {
+   } else {
       for (int i = 0; i < static_cast<int>(Water::Ions::numIons); ++i ) {
          Water::Ions ion = static_cast<Water::Ions>(i);
          m_ppm_digits[i]->setText( m_salt_table_model->total(ion) / allTheWaters, 0 );
@@ -365,6 +364,7 @@ void WaterDialog::removeSalts() {
       deadSalts.append( i.row() );
    }
    m_salt_table_model->removeSalts(deadSalts);
+   return;
 }
 
 //! \brief Calcuates the residual alkalinity of the mash water.
@@ -411,8 +411,7 @@ double WaterDialog::calculateSaltpH() {
 }
 
 //! \brief Calculates the pH delta caused by any salt additions.
-double WaterDialog::calculateAddedSaltpH()
-{
+double WaterDialog::calculateAddedSaltpH() {
 
    // We need the value from the salt table model, because we need all the
    // added salts, but not the base.

src/database/ObjectStoreWrapper.h

@@ -104,9 +104,12 @@ namespace ObjectStoreWrapper {
       if (id > 0 && objectStore.contains(id)) {
          return objectStore.getById(id);
       }
-      qDebug() <<
+      // If the object isn't stored in the DB then we can create a shared pointer for it, but this is dangerous as there
+      // might already be another shared pointer to it.  At minimum we should log a warning.  In the long run we should
+      // Q_ASSERT(false) here.
+      qWarning() <<
          Q_FUNC_INFO << "Creating new shared_ptr for unstored" << ne->metaObject()->className() << "#" << id << " :" <<
-         ne->name();
+         ne->name() << ".  This may be a bug - eg if a shared_ptr already exists for this object!";
       return std::shared_ptr<NE>{ne};
    }
 

src/tableModels/BtTableModel.h

@@ -32,9 +32,9 @@
 
 #include "BtFieldType.h"
 #include "measurement/UnitSystem.h"
+#include "model/NamedEntity.h"
 
 class Recipe;
-class NamedEntity;
 
 /**
  * \class BtTableModelData
@@ -75,21 +75,24 @@ class NamedEntity;
  *
  *        (I did start trying to do something clever with a common base class to try to expose functions from
  *        \c BtTableModelData to the implementation of \c BtTableModel / \c BtTableModelRecipeObserver /
- *        \c BtTableModelInventory, but it quickly starts getting more complicated than it's worth IMHO because (a)
- *        \c BtTableModelData is templated but a common base class cannot be and (b) templated functions cannot be virtual.)
+ *        \c BtTableModelInventory, but it quickly gets more complicated than it's worth IMHO because (a)
+ *        \c BtTableModelData is templated but a common base class cannot be and (b) templated functions cannot be
+ *        virtual.)
  */
 template<class NE>
 class BtTableModelData {
 protected:
    BtTableModelData() : rows{} {
       return;
    }
+   // Need a virtual destructor as we have a virtual member function
+   virtual ~BtTableModelData() = default;
 public:
    /**
     * \brief Return the \c i-th row in the model.
     *        Returns \c nullptr on failure.
     */
-   std::shared_ptr<NE> getRow(int ii) {
+   std::shared_ptr<NE> getRow(int ii) const {
       if (!(this->rows.isEmpty())) {
          if (ii >= 0 && ii < this->rows.size()) {
             return this->rows[ii];
@@ -118,12 +121,37 @@ class BtTableModelData {
       return tmp;
    }
 
+   /**
+    * \brief Given a raw pointer, find the index of the corresponding shared pointer in \c this->rows
+    *
+    *        This is useful because the Qt signals and slots framework allows the slot receiving a signal to get a raw
+    *        pointer to the object that sent the signal, and we often want to find the corresponding shared pointer in
+    *        our list.
+    *
+    *        Note that using this function is a lot safer than, say, calling ObjectStoreWrapper::getSharedFromRaw(), as
+    *        that only works for objects that are already stored in the database, something which is not guaranteed to
+    *        be the case with our rows.  (Eg in SaltTableModel, new Salts are only stored in the DB when the window is
+    *        closed with OK.)
+    *
+    *        Function name is for consistency with \c QList::indexOf
+    *
+    * \param object  what to search for
+    * \return index of object in this->rows or -1 if it's not found
+    */
+   int findIndexOf(NE const * object) const {
+      for (int index = 0; index < this->rows.size(); ++index) {
+         if (this->rows.at(index).get() == object) {
+            return index;
+         }
+      }
+      return -1;
+   }
+
 protected:
    virtual std::shared_ptr<NamedEntity> getRowAsNamedEntity(int ii) {
       return std::static_pointer_cast<NamedEntity>(this->getRow(ii));
    }
 
-
    QList< std::shared_ptr<NE> > rows;
 };
 

src/tableModels/FermentableTableModel.cpp

@@ -1,6 +1,6 @@
 /*
  * FermentableTableModel.cpp is part of Brewtarget, and is Copyright the following
- * authors 2009-2021
+ * authors 2009-2022
  * - Matt Young <mfsy@yahoo.com>
  * - Mik Firestone <mikfire@gmail.com>
  * - Philip Greggory Lee <rocketman768@gmail.com>
@@ -44,7 +44,6 @@
 #include "MainWindow.h"
 #include "measurement/Measurement.h"
 #include "measurement/Unit.h"
-#include "model/Fermentable.h"
 #include "model/Inventory.h"
 #include "model/Recipe.h"
 #include "PersistentSettings.h"
@@ -172,7 +171,8 @@ void FermentableTableModel::addFermentables(QList<std::shared_ptr<Fermentable> >
    }
 }
 
-void FermentableTableModel::removeFermentable(int fermId, std::shared_ptr<QObject> object) {
+void FermentableTableModel::removeFermentable([[maybe_unused]] int fermId,
+                                              std::shared_ptr<QObject> object) {
    this->remove(std::static_pointer_cast<Fermentable>(object));
    return;
 }
@@ -235,14 +235,13 @@ void FermentableTableModel::changedInventory(int invKey, BtStringConst const & p
    return;
 }
 
-void FermentableTableModel::changed(QMetaProperty prop, QVariant /*val*/) {
+void FermentableTableModel::changed(QMetaProperty prop, [[maybe_unused]] QVariant val) {
    qDebug() << Q_FUNC_INFO << prop.name();
 
    // Is sender one of our fermentables?
    Fermentable* fermSender = qobject_cast<Fermentable*>(sender());
    if (fermSender) {
-      auto spFermSender = ObjectStoreWrapper::getSharedFromRaw(fermSender);
-      int ii = this->rows.indexOf(spFermSender);
+      int ii = this->findIndexOf(fermSender);
       if (ii < 0) {
          return;
       }
@@ -295,7 +294,7 @@ QVariant FermentableTableModel::data(QModelIndex const & index, int role) const
             return QVariant(row->typeStringTr());
          }
          if (role == Qt::UserRole) {
-            return QVariant(row->type());
+            return QVariant(static_cast<int>(row->type()));
          }
          break;
       case FERMINVENTORYCOL:
@@ -325,15 +324,15 @@ QVariant FermentableTableModel::data(QModelIndex const & index, int role) const
             return QVariant(row->additionMethodStringTr());
          }
          if (role == Qt::UserRole) {
-            return QVariant(row->additionMethod());
+            return QVariant(static_cast<int>(row->additionMethod()));
          }
          break;
       case FERMAFTERBOIL:
          if (role == Qt::DisplayRole) {
             return QVariant(row->additionTimeStringTr());
          }
          if (role == Qt::UserRole) {
-            return QVariant(row->additionTime());
+            return QVariant(static_cast<int>(row->additionTime()));
          }
          break;
       case FERMYIELDCOL:
@@ -402,7 +401,9 @@ Qt::ItemFlags FermentableTableModel::flags(const QModelIndex& index ) const {
 }
 
 
-bool FermentableTableModel::setData(QModelIndex const & index, QVariant const & value, int role) {
+bool FermentableTableModel::setData(QModelIndex const & index,
+                                    QVariant const & value,
+                                    [[maybe_unused]] int role) {
    if (index.row() >= static_cast<int>(this->rows.size())) {
       return false;
    }
@@ -525,7 +526,7 @@ FermentableItemDelegate::FermentableItemDelegate(QObject* parent) : QItemDelegat
 }
 
 QWidget* FermentableItemDelegate::createEditor(QWidget *parent,
-                                               QStyleOptionViewItem const & option,
+                                               [[maybe_unused]] QStyleOptionViewItem const & option,
                                                QModelIndex const & index) const {
    if (index.column() == FERMTYPECOL )
    {
@@ -564,13 +565,10 @@ QWidget* FermentableItemDelegate::createEditor(QWidget *parent,
       int type = index.model()->index(index.row(), FERMTYPECOL).data(Qt::UserRole).toInt();
 
       // Hide the unsuitable item keeping the same enumeration
-      if(type == Fermentable::Grain)
-      {
-         list->item(Fermentable::Not_Mashed)->setHidden(true);
-      }
-      else
-      {
-         list->item(Fermentable::Steeped)->setHidden(true);
+      if (type == static_cast<int>(Fermentable::Type::Grain)) {
+         list->item(static_cast<int>(Fermentable::AdditionMethod::Not_Mashed))->setHidden(true);
+      } else {
+         list->item(static_cast<int>(Fermentable::AdditionMethod::Steeped))->setHidden(true);
       }
 
       return box;
@@ -645,7 +643,8 @@ void FermentableItemDelegate::setModelData(QWidget *editor, QAbstractItemModel *
    }
 }
 
-void FermentableItemDelegate::updateEditorGeometry(QWidget *editor, const QStyleOptionViewItem &option, const QModelIndex &index) const
-{
+void FermentableItemDelegate::updateEditorGeometry(QWidget * editor,
+                                                   QStyleOptionViewItem const & option,
+                                                   [[maybe_unused]] QModelIndex const & index) const {
    editor->setGeometry(option.rect);
 }

src/tableModels/FermentableTableModel.h

@@ -35,6 +35,7 @@
 #include <QWidget>
 
 #include "measurement/Unit.h"
+#include "model/Fermentable.h"
 #include "tableModels/BtTableModelInventory.h"
 
 // Forward declarations.

src/tableModels/HopTableModel.cpp

@@ -1,6 +1,6 @@
 /*
  * HopTableModel.cpp is part of Brewtarget, and is Copyright the following
- * authors 2009-2021
+ * authors 2009-2022
  * - Luke Vincent <luke.r.vincent@gmail.com>
  * - Matt Young <mfsy@yahoo.com>
  * - Mik Firestone <mikfire@gmail.com>
@@ -189,7 +189,8 @@ bool HopTableModel::remove(std::shared_ptr<Hop> hop) {
    return false;
 }
 
-void HopTableModel::removeHop(int hopId, std::shared_ptr<QObject> object) {
+void HopTableModel::removeHop([[maybe_unused]] int hopId,
+                              std::shared_ptr<QObject> object) {
    this->remove(std::static_pointer_cast<Hop>(object));
    return;
 }
@@ -220,13 +221,12 @@ void HopTableModel::changedInventory(int invKey, BtStringConst const & propertyN
    return;
 }
 
-void HopTableModel::changed(QMetaProperty prop, QVariant /*val*/) {
+void HopTableModel::changed(QMetaProperty prop, [[maybe_unused]] QVariant val) {
 
    // Find the notifier in the list
    Hop * hopSender = qobject_cast<Hop *>(sender());
    if (hopSender) {
-      auto spHopSender = ObjectStoreWrapper::getSharedFromRaw(hopSender);
-      int ii = this->rows.indexOf(spHopSender);
+      int ii = this->findIndexOf(hopSender);
       if (ii < 0) {
          return;
       }
@@ -299,7 +299,7 @@ QVariant HopTableModel::data(const QModelIndex & index, int role) const {
             return QVariant(row->useStringTr());
          }
          if (role == Qt::UserRole) {
-            return QVariant(row->use());
+            return QVariant(static_cast<int>(row->use()));
          }
          break;
       case HOPTIMECOL:
@@ -314,7 +314,7 @@ QVariant HopTableModel::data(const QModelIndex & index, int role) const {
          if (role == Qt::DisplayRole) {
             return QVariant(row->formStringTr());
          } else if (role == Qt::UserRole) {
-            return QVariant(row->form());
+            return QVariant(static_cast<int>(row->form()));
          }
          break;
       default :

src/tableModels/MashStepTableModel.cpp

@@ -1,6 +1,6 @@
 /*
  * MashStepTableModel.cpp is part of Brewtarget, and is Copyright the following
- * authors 2009-2021
+ * authors 2009-2022
  * - Matt Young <mfsy@yahoo.com>
  * - Maxime Lavigne <duguigne@gmail.com>
  * - Mik Firestone <mikfire@gmail.com>
@@ -200,12 +200,12 @@ void MashStepTableModel::mashChanged() {
    return;
 }
 
-void MashStepTableModel::mashStepChanged(QMetaProperty prop, QVariant val) {
+void MashStepTableModel::mashStepChanged(QMetaProperty prop,
+                                         [[maybe_unused]] QVariant val) {
    qDebug() << Q_FUNC_INFO;
 
-   MashStep* stepSenderRaw = qobject_cast<MashStep*>(sender());
-   if (stepSenderRaw) {
-      auto stepSender = ObjectStoreWrapper::getSharedFromRaw(stepSenderRaw);
+   MashStep* stepSender = qobject_cast<MashStep*>(sender());
+   if (stepSender) {
       if (stepSender->getMashId() != this->mashObs->key()) {
          // It really shouldn't happen that we get a notification for a MashStep that's not in the Mash we're watching,
          // but, if we do, then stop trying to process the update.
@@ -216,10 +216,10 @@ void MashStepTableModel::mashStepChanged(QMetaProperty prop, QVariant val) {
          return;
       }
 
-      int ii = this->rows.indexOf(stepSender);
+      int ii = this->findIndexOf(stepSender);
       if (ii >= 0) {
          if (prop.name() == PropertyNames::MashStep::stepNumber) {
-            this->reorderMashStep(stepSender, ii);
+            this->reorderMashStep(this->rows.at(ii), ii);
          }
 
          emit dataChanged( QAbstractItemModel::createIndex(ii, 0),
@@ -267,7 +267,7 @@ QVariant MashStepTableModel::data(QModelIndex const & index, int role) const {
          return QVariant(
             Measurement::displayAmount(
                Measurement::Amount{
-                  row->type() == MashStep::Decoction ? row->decoctionAmount_l() : row->infuseAmount_l(),
+                  row->type() == MashStep::Type::Decoction ? row->decoctionAmount_l() : row->infuseAmount_l(),
                   Measurement::Units::liters
                },
                3,
@@ -276,7 +276,7 @@ QVariant MashStepTableModel::data(QModelIndex const & index, int role) const {
             )
          );
       case MASHSTEPTEMPCOL:
-         if (row->type() == MashStep::Decoction) {
+         if (row->type() == MashStep::Type::Decoction) {
             return QVariant("---");
          }
          return QVariant(
@@ -361,7 +361,7 @@ bool MashStepTableModel::setData(QModelIndex const & index, QVariant const & val
 
       case MASHSTEPAMOUNTCOL:
          if (value.canConvert(QVariant::String)) {
-            if (row->type() == MashStep::Decoction ) {
+            if (row->type() == MashStep::Type::Decoction ) {
                MainWindow::instance().doOrRedoUpdate(
                   *row,
                   PropertyNames::MashStep::decoctionAmount_l,
@@ -387,7 +387,7 @@ bool MashStepTableModel::setData(QModelIndex const & index, QVariant const & val
          return false;
 
       case MASHSTEPTEMPCOL:
-         if (value.canConvert(QVariant::String) && row->type() != MashStep::Decoction) {
+         if (value.canConvert(QVariant::String) && row->type() != MashStep::Type::Decoction) {
             MainWindow::instance().doOrRedoUpdate(
                *row,
                PropertyNames::MashStep::infuseTemp_c,