Merge pull request #830 from matty0ung/usability

Fixes for start-up crashes

.signpath/artifact-configurations/default.xml

@@ -1,41 +1,36 @@
 <?xml version="1.0" encoding="utf-8"?>
 <!--
    .signpath/artifact-configurations/default.xml is part of Brewtarget
+
+   See .github/workflows/windows.yml for more general info on how we use SignPath to sign the Windows binaries
+
+   See https://about.signpath.io/documentation/artifact-configuration/ for the syntax for this file
 -->
 <artifact-configuration xmlns="http://signpath.io/artifact-configuration/v1">
+  <!--
+  Note, per https://github.com/SignPath/github-action-submit-signing-request, that "the used artifact configuration must
+  have a zip-file element at its root, as all artifacts are packaged as ZIP archives on GitHub by default."
+  -->
   <zip-file>
+    <!--
+    Prior to the signing step, our build will have generated two files (where x.y.z is the version number - eg 4.0.4):
+      Brewtarget x.y.z Installer.exe
+      Brewtarget x.y.z Installer.exe.sha256sum
+    We want to create a signed version of the first file, and then generate a new checksum for it.
 
-    <msi-file path="DemoExample.msi">
-      <directory path="application/SignPath Demo">
-
-        <pe-file-set>
-          <include path="Microsoft.*.dll" min-matches="0" max-matches="unbounded" />
-          <include path="Microsoft.*.exe" min-matches="0" max-matches="unbounded" />
-          <for-each>
-            <authenticode-verify />
-          </for-each>
-        </pe-file-set>
-
-        <pe-file-set>
-          <include path="Serilog.dll" product-name="Serilog" min-matches="0" />
-          <include path="Serilog.AspNetCore.dll" product-name="Serilog" product-version="7.0.0" min-matches="0" />
-        </pe-file-set>
-
-        <pe-file-set>
-          <include path="DemoExample.dll" />
-          <include path="DemoExample.exe" />
-          <for-each>
-            <authenticode-sign />
-          </for-each>
-        </pe-file-set>
-
-      </directory>
-      <authenticode-sign />
-    </msi-file>
-
-    <xml-file path="bom.xml" root-element-namespace="http://cyclonedx.org/schema/bom/1.5" root-element-name="bom">
-      <xml-sign/>
-    </xml-file>
+    Fortunately we don't have to work this out from scratch.  By manually uploading an installer to sign, SignPath will
+    also generate a sample artifact-configuration, which we can then edit as needed.
 
+    Because we are only signing a single installer, our configuration is actually very simple.
+    -->
+    <pe-file>
+      <!--
+      This means do the signing with Microsoft Authenticode, which is "the primary signing method on the Windows
+      platform".  This is equivalent to using Microsoft’s SignTool.exe.
+      -->
+      <authenticode-sign hash-algorithm="sha256"
+                         description="Brewtarget Windows Installer"
+                         description-url="https://www.brewtarget.beer" />
+    </pe-file>
   </zip-file>
 </artifact-configuration>

CHANGES.markdown

@@ -13,6 +13,20 @@ happens, so I'm now setting it to a slightly arbitrary time early in the morning
 * Additional methods for calculating IBU
 * We'll list other new features here...
 
+
+## v4.0.4
+Minor bug fixes for the 4.0.4 release (ie bugs in 4.0.4 are fixed in this 4.0.5 release).
+
+### New Features
+* None
+
+### Bug Fixes
+* Crash on new ingredient import (brewtarget v4.0.4) at startup time [828](https://github.com/Brewtarget/brewtarget/issues/828)
+*  Crash on new recipe creation [829](https://github.com/Brewtarget/brewtarget/issues/829)
+
+### Release Timestamp
+Tue, 24 Sep 2024 04:00:05 +0100
+
 ## v4.0.4
 Minor bug fixes for the 4.0.3 release (ie bugs in 4.0.3 are fixed in this 4.0.4 release).
 
@@ -26,7 +40,7 @@ Minor bug fixes for the 4.0.3 release (ie bugs in 4.0.3 are fixed in this 4.0.4
 * Edit Boil and Edit Fermentation buttons not working [826](https://github.com/Brewtarget/brewtarget/issues/826)
 
 ### Release Timestamp
-Wed, 28 Aug 2024 04:00:04 +0100
+Wed, 18 Sep 2024 04:00:04 +0100
 
 ## v4.0.3
 Minor bug fixes for the 4.0.2 release (ie bugs in 4.0.2 are fixed in this 4.0.3 release).

src/MainWindow.cpp

@@ -2506,15 +2506,11 @@ void MainWindow::editYeastOfSelectedYeastAddition() {
    return;
 }
 
-void MainWindow::newRecipe()
-{
-   QString name = QInputDialog::getText(this, tr("Recipe name"),
-                                          tr("Recipe name:"));
-   QVariant defEquipKey = PersistentSettings::value(PersistentSettings::Names::defaultEquipmentKey, -1);
-   QObject* selection = sender();
-
-   if( name.isEmpty() )
+void MainWindow::newRecipe() {
+   QString const name = QInputDialog::getText(this, tr("Recipe name"), tr("Recipe name:"));
+   if (name.isEmpty()) {
       return;
+   }
 
    std::shared_ptr<Recipe> newRec = std::make_shared<Recipe>(name);
 
@@ -2526,17 +2522,31 @@ void MainWindow::newRecipe()
       return;
    }
    ObjectStoreWrapper::insert(newRec);
-   std::shared_ptr<Boil> newBoil = std::make_shared<Boil>(QString("Boil for").arg(name));
+
+   //
+   // .:TODO:. For the moment, we still assume that every Recipe has a Boil and a Fermentation.  Also, for the moment,
+   // we also create a new boil and fermentation for every Recipe.  In time, I'd like to extend the UI so that, when you
+   // input the name for your new Recipe, you also select Mash, Boil, Fermentation (all either from "List of existing"
+   // or "Make new").
+   //
+   std::shared_ptr<Boil> newBoil = std::make_shared<Boil>(tr("Automatically-created Boil for %1").arg(name));
+   // NB: Recipe::setBoil will ensure Boil is stored in the database
    newRec->setBoil(newBoil);
-   ObjectStoreWrapper::insert(newBoil);
+   // Since we're auto-creating a Boil, it might as well start out with the "standard" profile
+   newBoil->ensureStandardProfile();
+
+   std::shared_ptr<Fermentation> newFermentation = std::make_shared<Fermentation>(tr("Automatically-created Fermentation for %1").arg(name));
+   // NB: Recipe::setFermentation will ensure Fermentation is stored in the database
+   newRec->setFermentation(newFermentation);
 
    // Set the following stuff so everything appears nice
    // and the calculations don't divide by zero... things like that.
    newRec->setBatchSize_l(18.93); // 5 gallons
    newBoil->setPreBoilSize_l(23.47);  // 6.2 gallons
    newRec->setEfficiency_pct(70.0);
 
-   // we need a valid key, so insert the recipe before we add equipment
+   // We need a valid key, so insert the recipe before we add equipment
+   QVariant const defEquipKey = PersistentSettings::value(PersistentSettings::Names::defaultEquipmentKey, -1);
    if (defEquipKey != -1) {
       auto equipment = ObjectStoreWrapper::getById<Equipment>(defEquipKey.toInt());
       // I really want to do this before we've written the object to the
@@ -2549,8 +2559,9 @@ void MainWindow::newRecipe()
       }
    }
 
-   // a new recipe will be put in a folder if you right click on a recipe or
+   // A new recipe will be put in a folder if you right click on a recipe or
    // folder. Otherwise, it goes into the main window?
+   QObject* selection = this->sender();
    if (selection) {
       TreeView* sent = qobject_cast<TreeView*>(tabWidget_Trees->currentWidget()->focusWidget());
       if (sent) {

src/database/DatabaseSchemaHelper.cpp

@@ -2302,7 +2302,7 @@ bool DatabaseSchemaHelper::migrate(Database & database, int oldVersion, int newV
    // By the magic of RAII, this will abort if we exit this function (including by throwing an exception) without
    // having called dbTransaction.commit().  (It will also turn foreign keys back on either way -- whether the
    // transaction is committed or rolled back.)
-   DbTransaction dbTransaction{database, connection, DbTransaction::DISABLE_FOREIGN_KEYS};
+   DbTransaction dbTransaction{database, connection, "Migrate", DbTransaction::DISABLE_FOREIGN_KEYS};
 
    for ( ; oldVersion < newVersion && ret; ++oldVersion ) {
       ret &= migrateNext(database, oldVersion, connection);

src/database/DbTransaction.cpp

@@ -1,5 +1,5 @@
 /*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌
- * database/DbTransaction.cpp is part of Brewtarget, and is copyright the following authors 2021-2022:
+ * database/DbTransaction.cpp is part of Brewtarget, and is copyright the following authors 2021-2024:
  *   • Matt Young <[email protected]>
  *
  * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License
@@ -19,11 +19,15 @@
 #include <QSqlError>
 
 #include "database/Database.h"
+#include "Logging.h"
 
-
-DbTransaction::DbTransaction(Database & database, QSqlDatabase & connection, DbTransaction::SpecialBehaviours specialBehaviours) :
+DbTransaction::DbTransaction(Database & database,
+                             QSqlDatabase & connection,
+                             QString const nameForLogging,
+                             DbTransaction::SpecialBehaviours specialBehaviours) :
    database{database},
    connection{connection},
+   nameForLogging{nameForLogging},
    committed{false},
    specialBehaviours{specialBehaviours} {
    // Note that, on SQLite at least, turning foreign keys on and off has to happen outside a transaction, so we have to
@@ -33,9 +37,12 @@ DbTransaction::DbTransaction(Database & database, QSqlDatabase & connection, DbT
    }
 
    bool succeeded = this->connection.transaction();
-   qDebug() << Q_FUNC_INFO << "Database transaction begin: " << (succeeded ? "succeeded" : "failed");
+   qDebug() <<
+      Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "begin: " << (succeeded ? "succeeded" : "failed");
    if (!succeeded) {
-      qCritical() << Q_FUNC_INFO << "Unable to start database transaction:" << connection.lastError().text();
+      qCritical() <<
+         Q_FUNC_INFO << "Unable to start database transaction" << this->nameForLogging << ":" << connection.lastError().text();
+      qCritical().noquote() << Q_FUNC_INFO << Logging::getStackTrace();
    }
    return;
 }
@@ -44,9 +51,11 @@ DbTransaction::~DbTransaction() {
    qDebug() << Q_FUNC_INFO;
    if (!committed) {
       bool succeeded = this->connection.rollback();
-      qDebug() << Q_FUNC_INFO << "Database transaction rollback: " << (succeeded ? "succeeded" : "failed");
+      qDebug() <<
+         Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "rollback: " << (succeeded ? "succeeded" : "failed");
       if (!succeeded) {
-         qCritical() << Q_FUNC_INFO << "Unable to rollback database transaction:" << connection.lastError().text();
+         qCritical() <<
+            Q_FUNC_INFO << "Unable to rollback database transaction" << this->nameForLogging << ":" << connection.lastError().text();
       }
    }
 
@@ -59,9 +68,11 @@ DbTransaction::~DbTransaction() {
 
 bool DbTransaction::commit() {
    this->committed = connection.commit();
-   qDebug() << Q_FUNC_INFO << "Database transaction commit: " << (this->committed ? "succeeded" : "failed");
+   qDebug() <<
+      Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "commit: " << (this->committed ? "succeeded" : "failed");
    if (!this->committed) {
-      qCritical() << Q_FUNC_INFO << "Unable to commit database transaction:" << connection.lastError().text();
+      qCritical() <<
+         Q_FUNC_INFO << "Unable to commit database transaction" << this->nameForLogging << ":" << connection.lastError().text();
    }
    return this->committed;
 }

src/database/DbTransaction.h

@@ -1,5 +1,5 @@
 /*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌
- * database/DbTransaction.h is part of Brewtarget, and is copyright the following authors 2021:
+ * database/DbTransaction.h is part of Brewtarget, and is copyright the following authors 2021-2024:
  *   • Matt Young <[email protected]>
  *
  * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License
@@ -34,7 +34,10 @@ class DbTransaction {
    /**
     * \brief Constructing a \c DbTransaction will start a DB transaction
     */
-   DbTransaction(Database & database, QSqlDatabase & connection, SpecialBehaviours specialBehaviours = NONE);
+   DbTransaction(Database & database,
+                 QSqlDatabase & connection,
+                 QString const nameForLogging = "???",
+                 SpecialBehaviours specialBehaviours = NONE);
 
    /**
     * \brief When a \c DbTransaction goes out of scope and its destructor is called, the transaction started in the
@@ -53,6 +56,9 @@ class DbTransaction {
    Database & database;
    // This is intended to be a short-lived object, so it's OK to store a reference to a QSqlDatabase object
    QSqlDatabase & connection;
+   // This is useful for diagnosing problems such as
+   // 'Unable to start database transaction: "cannot start a transaction within a transaction Unable to begin transaction"'
+   QString const nameForLogging;
    bool committed;
    int specialBehaviours;
 

src/database/ObjectStore.cpp

@@ -1409,7 +1409,9 @@ void ObjectStore::loadAll(Database * database) {
    //
    // .:TBD:. In theory we don't need a transaction if we're _only_ reading data...
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Load All %1").arg(*this->pimpl->primaryTable.tableName)};
 
    //
    // Using QSqlTableModel would save us having to write a SELECT statement, however it is a bit hard to use it to
@@ -1710,7 +1712,9 @@ int ObjectStore::insert(std::shared_ptr<QObject> object) {
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Insert %1").arg(*this->pimpl->primaryTable.tableName)};
 
    int primaryKey = this->pimpl->insertObjectInDb(connection, *object, false);
 
@@ -1749,7 +1753,9 @@ void ObjectStore::update(std::shared_ptr<QObject> object) {
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Update %1").arg(*this->pimpl->primaryTable.tableName)};
 
    //
    // Construct the SQL, which will be of the form
@@ -1875,7 +1881,11 @@ void ObjectStore::updateProperty(QObject const & object, BtStringConst const & p
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{
+      *this->pimpl->database,
+      connection,
+      QString("Update property %1 on %2").arg(*propertyName).arg(*this->pimpl->primaryTable.tableName)
+   };
 
    if (!this->pimpl->updatePropertyInDb(connection, object, propertyName)) {
       // Something went wrong.  Bailing out here will abort the transaction and avoid sending the signal.
@@ -1918,7 +1928,9 @@ std::shared_ptr<QObject> ObjectStore::defaultHardDelete(int id) {
    qDebug() << Q_FUNC_INFO << "Hard delete" << this->pimpl->m_className << "#" << id;
    auto object = this->pimpl->allObjects.value(id);
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Hard delete %1").arg(*this->pimpl->primaryTable.tableName)};
 
    // We'll use this in a couple of places below
    QVariant primaryKey{id};

src/database/ObjectStoreTyped.cpp

@@ -1041,7 +1041,7 @@ bool WriteAllObjectStoresToNewDb(Database & newDatabase, QSqlDatabase & connecti
    // having called dbTransaction.commit().  (It will also turn foreign keys back on either way -- whether the
    // transaction is committed or rolled back.)
    //
-   DbTransaction dbTransaction{newDatabase, connectionNew, DbTransaction::DISABLE_FOREIGN_KEYS};
+   DbTransaction dbTransaction{newDatabase, connectionNew, "Write All", DbTransaction::DISABLE_FOREIGN_KEYS};
 
    for (ObjectStore const * objectStore : getAllObjectStores()) {
       if (!objectStore->writeAllToNewDb(newDatabase, connectionNew)) {

src/model/Boil.cpp

@@ -67,16 +67,23 @@ Boil::Boil(QString name) :
    m_description  {""          },
    m_notes        {""          },
    m_preBoilSize_l{std::nullopt} {
+
+   CONSTRUCTOR_END
    return;
 }
 
 Boil::Boil(NamedParameterBundle const & namedParameterBundle) :
-   NamedEntity                {namedParameterBundle},
+   NamedEntity     {namedParameterBundle},
    FolderBase<Boil>{namedParameterBundle},
    StepOwnerBase<Boil, BoilStep>{},
    SET_REGULAR_FROM_NPB (m_description  , namedParameterBundle, PropertyNames::Boil::description  ),
    SET_REGULAR_FROM_NPB (m_notes        , namedParameterBundle, PropertyNames::Boil::notes        ),
    SET_REGULAR_FROM_NPB (m_preBoilSize_l, namedParameterBundle, PropertyNames::Boil::preBoilSize_l) {
+
+   // If we're being constructed from a BeerXML file, we use the property boilTime_mins for RECIPE > BOIL_TIME
+   SET_IF_PRESENT_FROM_NPB_NO_MV(Boil::setBoilTime_mins, namedParameterBundle, PropertyNames::Boil::boilTime_mins);
+
+   CONSTRUCTOR_END
    return;
 }
 
@@ -87,6 +94,8 @@ Boil::Boil(Boil const & other) :
    m_description  {other.m_description  },
    m_notes        {other.m_notes        },
    m_preBoilSize_l{other.m_preBoilSize_l} {
+
+   CONSTRUCTOR_END
    return;
 }
 
@@ -170,6 +179,7 @@ void Boil::ensureStandardProfile() {
    if (boilSteps.size() == 0 || boilSteps.at(0)->startTemp_c().value_or(100.0) > Boil::minimumBoilTemperature_c) {
       // We need to add a ramp-up (aka pre-boil) step
       auto preBoil = std::make_shared<BoilStep>(tr("Pre-boil for %1").arg(recipe->name()));
+      preBoil->setDescription(tr("Automatically-generated pre-boil step for %1").arg(recipe->name()));
       // Get the starting temperature for the ramp-up from the end temperature of the mash
       double startingTemp = Boil::minimumBoilTemperature_c - 1.0;
       if (recipe->mash()) {
@@ -188,6 +198,7 @@ void Boil::ensureStandardProfile() {
    if (boilSteps.size() < 2 || boilSteps.at(1)->startTemp_c().value_or(0.0) < Boil::minimumBoilTemperature_c) {
       // We need to add a main (aka boil proper) step
       auto mainBoil = std::make_shared<BoilStep>(tr("Main boil for %1").arg(recipe->name()));
+      mainBoil->setDescription(tr("Automatically-generated boil proper step for %1").arg(recipe->name()));
       mainBoil->setStartTemp_c(100.0);
       mainBoil->setEndTemp_c(100.0);
       this->insertStep(mainBoil, 2);
@@ -196,6 +207,7 @@ void Boil::ensureStandardProfile() {
    if (boilSteps.size() < 3 || boilSteps.at(2)->endTemp_c().value_or(100.0) > Boil::minimumBoilTemperature_c) {
       // We need to add a post-boil step
       auto postBoil = std::make_shared<BoilStep>(tr("Post-boil for %1").arg(recipe->name()));
+      postBoil->setDescription(tr("Automatically-generated post-boil step for %1").arg(recipe->name()));
       double endingTemp = 30.0;
       if (recipe->fermentation()) {
          auto fs = recipe->fermentation()->fermentationSteps();