Merge pull request #684 from matty0ung/3.0.X

More Mac build fixes plus fix for Linux desktop icon etc locations plus Water Chemistry crash fix

.github/workflows/mac.yml

@@ -99,7 +99,8 @@ jobs:
         cd build
         pwd
         make package
-        ls -ltr
+        pwd
+        tree -sh
 
     - name: Upload Mac Packages (Installers)
       if: ${{ success() }}

CMakeLists.txt

@@ -165,9 +165,19 @@ if(UNIX AND NOT APPLE)
       set(EXEC_PREFIX ${CMAKE_INSTALL_PREFIX})
    endif()
 
-   set(installSubDir_data "share/${CMAKE_PROJECT_NAME}")
-   set(installSubDir_doc  "share/doc/${CMAKE_PROJECT_NAME}")
-   set(installSubDir_bin  "bin")
+   set(installSubDir_data         "share/${CMAKE_PROJECT_NAME}")
+   set(installSubDir_doc          "share/doc/${CMAKE_PROJECT_NAME}")
+   set(installSubDir_bin          "bin")
+   # According to https://specifications.freedesktop.org/menu-spec/menu-spec-1.0.html#paths, .desktop files need to live
+   # in one of the $XDG_DATA_DIRS/applications/.  (Note that $XDG_DATA_DIRS is a colon-separated list of directories, typically
+   # defaulting to /usr/local/share/:/usr/share/. but on another system it might be
+   # /usr/share/plasma:/usr/local/share:/usr/share:/var/lib/snapd/desktop:/var/lib/snapd/desktop).  When combined with
+   # CMAKE_INSTALL_PREFIX, "share/applications" should end up being one of these.
+   set(installSubDir_applications "share/applications")
+   # It's a similar but slightly more complicated story for where to put icons.  (See
+   # https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#directory_layout for all the
+   # details.)
+   set(installSubDir_icons        "share/icons")
 
 elseif(WIN32)
    #============================================ Windows Install Directories ===========================================
@@ -1380,6 +1390,20 @@ elseif(APPLE)
    # but when otool is run by fixup_bundle, it looks for @rpath/libxalanMsg.112.dylib and will not accept any other
    # name for the library.   So, we change ".0.dylib" at the end of the library file name to ".dylib".
    #
+   # Then you have to remember, as commented elsewhere, the DESTINATION option of the install() command must be a
+   # relative path; otherwise installed files are ignored by CPack - ie won't end up in the DMG file.  (Yes, this does
+   # contrast with the requirement of fixup_bundle to have an absolute path.  You have to stay on your toes with CMake.)
+   # This is where it gets a bit confusing(!)  We seem to have two copies of the bundle in the build tree:
+   #    1: ${fileName_executable}.app
+   #    2: _CPack_Packages/Darwin/DragNDrop/${CMAKE_PROJECT_NAME}_${PROJECT_VERSION}_x86_64/ALL_IN_ONE/${fileName_executable}.app
+   # They look to be the same, except that installing with an absolute path beginning
+   # ${CMAKE_BINARY_DIR}/${fileName_executable}.app/ puts things in the first of these (but not the second), while
+   # installing with a relative path puts them in the second (but not the first).
+   #
+   # AFAICT it's the second bundle that ends up in the DMG file even though it's the first one that we're running
+   # fixup_bundle on.  For the moment I'm just going to install libxalanMsg.112.dylib into both, on the grounds that it
+   # surely can't hurt.
+   #
    # One day, I hope we will find a more elegant and less brittle way to do all this!
    #
    find_library(XalanCMessage_LIBRARY NAMES xalanMsg)
@@ -1388,6 +1412,9 @@ elseif(APPLE)
    string(REGEX REPLACE "\.0\.dylib$" ".dylib" XalanCMessage_LIBRARY_tweaked "${XalanCMessage_LIBRARY_withoutPath}")
    message(STATUS "Found libxalanMsg at ${XalanCMessage_LIBRARY} which resolves to ${XalanCMessage_LIBRARY_resolved}")
    message(STATUS "Tweaked libxalanMsg file name from ${XalanCMessage_LIBRARY_withoutPath} to ${XalanCMessage_LIBRARY_tweaked}")
+   install(FILES ${XalanCMessage_LIBRARY_resolved}
+           DESTINATION ${fileName_executable}.app/Contents/Frameworks/
+           RENAME ${XalanCMessage_LIBRARY_tweaked})
    install(FILES ${XalanCMessage_LIBRARY_resolved}
            DESTINATION ${CMAKE_BINARY_DIR}/${fileName_executable}.app/Contents/Frameworks/
            RENAME ${XalanCMessage_LIBRARY_tweaked})
@@ -1484,13 +1511,16 @@ install(FILES ${filesToInstall_sounds}
 if(UNIX AND NOT APPLE)
    #----------- Linux -----------
    # Install the icons
+   # Per https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#install_icons, "installing a
+   # svg icon in $prefix/share/icons/hicolor/scalable/apps means most desktops will have one icon that works for all
+   # sizes".
    install(FILES ${filesToInstall_icons}
-           DESTINATION "${installSubDir_data}/icons/hicolor/scalable/apps/"
+           DESTINATION "${installSubDir_icons}/hicolor/scalable/apps/"
            COMPONENT ${DATA_INSTALL_COMPONENT})
 
    # Install the .desktop file
    install(FILES ${filesToInstall_desktop}
-           DESTINATION "${installSubDir_data}/applications"
+           DESTINATION "${installSubDir_applications}"
            COMPONENT ${DATA_INSTALL_COMPONENT})
 
    # Install friendly-format change log aka release notes

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 :