Merge pull request #823 from matty0ung/usability

Fix migration of inventory amounts when database upgrades to v11. Plus some minor tidy-ups.
Brewtarget · Sep 7, 2024 · 3938dae · 3938dae
2 parents e9f4d9e + 9756c16
commit 3938dae
Show file tree

Hide file tree

Showing 50 changed files with 698 additions and 522 deletions.
diff --git a/CHANGES.markdown b/CHANGES.markdown
@@ -21,6 +21,8 @@ Minor bug fixes for the 4.0.3 release (ie bugs in 4.0.3 are fixed in this 4.0.4
 
 ### Bug Fixes
 * Crash editing Target Boil Size [817](https://github.com/Brewtarget/brewtarget/issues/817)
+* Crash when exporting recipe [821](https://github.com/Brewtarget/brewtarget/issues/821)
+* Inventory not displaying (further fixes) [814](https://github.com/Brewtarget/brewtarget/issues/814)
 
 ### Release Timestamp
 Wed, 28 Aug 2024 04:00:04 +0100

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -94,7 +94,7 @@ if(APPLE AND NOT CMAKE_OSX_ARCHITECTURES)
    # Per https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_ARCHITECTURES.html, "the value of this variable should
    # be set prior to the first project() or enable_language() command invocation because it may influence configuration
    # of the toolchain and flags".
-   set(CMAKE_OSX_ARCHITECTURES x86_64) # Build intel 64-bit binary.
+   set(CMAKE_OSX_ARCHITECTURES x86_64, arm64) # Build both x86_64 and arm64 for Apple silicon.
    #set(CMAKE_OSX_ARCHITECTURES i386 x86_64) # Build intel binary.
    #set(CMAKE_OSX_ARCHITECTURES ppc i386 ppc64 x86_64) # Build universal binary.
 endif()
@@ -805,7 +805,7 @@ configure_file(src/config.in src/config.h)
 #=======================================================================================================================
 # We don't need to list the embedded resource files themselves here, just the "Resource Collection File" that lists what
 # they are.
-set(filesToCompile_qrc "${CMAKE_CURRENT_SOURCE_DIR}/${PROJECT_NAME}.qrc")
+set(filesToCompile_qrc "${CMAKE_CURRENT_SOURCE_DIR}/resources.qrc")
 
 #=======================================================================================================================
 #=========================================== Files included with the program ===========================================

diff --git a/meson.build b/meson.build
@@ -1211,7 +1211,7 @@ installSubDir_translations = installSubDir_data + '/translations_qm'
 #=======================================================================================================================
 
 # Compile Qt's resources collection files (.qrc) into C++ files for compilation
-generatedFromQrc = qt.compile_resources(sources : projectName + '.qrc')
+generatedFromQrc = qt.compile_resources(sources : 'resources.qrc')
 
 # Compile Qt's ui files (.ui) into header files.
 generatedFromUi = qt.compile_ui(sources : uiFiles)

diff --git a/brewtarget.qrc → resources.qrc b/brewtarget.qrc → resources.qrc
@@ -1,6 +1,6 @@
 <!DOCTYPE RCC>
 <!--
- * brewtarget.qrc is part of Brewtarget, and is copyright the following authors 2009-2024:
+ * resources.qrc is part of Brewtarget, and is copyright the following authors 2009-2024:
  *   • Aidan Roberts <[email protected]>
  *   • Idar Lund <[email protected]>
  *   • Mark de Wever <[email protected]>

diff --git a/src/database/DatabaseSchemaHelper.cpp b/src/database/DatabaseSchemaHelper.cpp
@@ -1872,13 +1872,46 @@ namespace {
          //
          // At this point, all hop and fermentable amounts will be weights, because prior versions of the DB did not
          // support measuring them by volume.
+         //
+         // There is a bit of a gotcha waiting for us here.  In the old schema, where each hop refers to a
+         // hop_in_inventory row, there can and will be multiple hops sharing the same inventory row.  In the new
+         // schema, where each hop_in_inventory row refers to a hop, different hops cannot share an inventory.  (This is
+         // all by design, as we ultimately want to be able to manage multiple inventory entries per hop.  That way you
+         // can separately track the Fuggles that you bought last year (and need to use up) from the ones you bought
+         // last week (so you won't run out when you use up last year's).
+         //
+         // Although we have, by this point, deleted the "child" entries in hop that signified "use of hop in recipe"
+         // (replacing them with entries in hop_in_recipe).  There can still be multiple entries for "the same" hop in
+         // the hop table, all sharing the same hop_in_inventory row.  Specifically this is because of hops marked
+         // deleted or not displayable.  The simple answer would be to ignore deleted and non-displayable hops.  But
+         // this creates another problem because there can be inventory entries for hops that are only deleted.  And,
+         // whilst we might have a natural instinct to just delete the hop_in_inventory rows that have no corresponding
+         // displayable not-deleted hop, that feels wrong as we would be throwing data away that might one day be
+         // needed (eg if someone wants to undelete a hop that they deleted in error).
+         //
+         // So, we do something "clever".  For the "update hop_in_inventory rows to point at hop rows" query, we feed it
+         // the list of hops in a sort of reverse order, specifically such that the deleted and non-displayable ones
+         // occur before the others.  Thus, where there are multiple hop rows pointing to the same hop_in_inventory row,
+         // the latter will get updated multiple times and, if there is a corresponding displayable non-deleted hop,
+         // that will be the last one that the hop_in_inventory row is updated to point to, so it will "win" over the
+         // others.  Despite sounding a bit complicated, it's makes the SQL simpler than a lot of other approaches!
+         //
+         // It _should_ be the case that there is at most one displayable non-deleted hop per row of hop_in_inventory.
+         // However, just in case this is ever not true, we go a bit further and say that, after ordering hops so that
+         // all the deleted and non-displayable ones are parsed first, the remaining ones are put in reverse ID order,
+         // which means the oldest entries (with the smallest IDs) get processed last and are most likely to "win" in
+         // conflicts.  At the moment at least, it seems the most reasonable tie-breaker.
+         //
+         // Of course, all the above applies, mutatis mutandis, to fermentables, miscs, and yeasts as well.
+         //
          {QString("UPDATE hop_in_inventory "
                   "SET hop_id = h.id, "
                       "unit = 'kilograms' "
                   "FROM ("
                      "SELECT id, "
                             "inventory_id "
-                     "FROM hop"
+                     "FROM hop "
+                     "ORDER BY NOT deleted, display, id DESC "
                   ") AS h "
                   "WHERE hop_in_inventory.id = h.inventory_id")},
          {QString("UPDATE fermentable_in_inventory "
@@ -1887,7 +1920,8 @@ namespace {
                   "FROM ("
                      "SELECT id, "
                             "inventory_id "
-                     "FROM fermentable"
+                     "FROM fermentable "
+                     "ORDER BY NOT deleted, display, id DESC "
                   ") AS f "
                   "WHERE fermentable_in_inventory.id = f.inventory_id ")},
          // For misc, we need to support both weights and volumes.
@@ -1898,7 +1932,8 @@ namespace {
                      "SELECT id, "
                             "inventory_id, "
                             "CASE WHEN amount_is_weight THEN 'kilograms' ELSE 'liters' END AS unit "
-                     "FROM misc"
+                     "FROM misc "
+                     "ORDER BY NOT deleted, display, id DESC "
                   ") AS m "
                   "WHERE misc_in_inventory.id = m.inventory_id")},
          // HOWEVER, for inventory purposes, yeast is actually stored as "number of packets" (aka quanta in various old
@@ -1909,7 +1944,8 @@ namespace {
                   "FROM ("
                      "SELECT id, "
                             "inventory_id "
-                     "FROM yeast"
+                     "FROM yeast "
+                     "ORDER BY NOT deleted, display, id DESC "
                   ") AS y "
                   "WHERE yeast_in_inventory.id = y.inventory_id")},
          // Now we transferred info across, we don't need the inventory_id column on hop or fermentable

diff --git a/src/editors/FermentableEditor.cpp b/src/editors/FermentableEditor.cpp
@@ -181,6 +181,7 @@ void FermentableEditor::readFieldsFromEditItem(std::optional<QString> propName)
    if (!propName || *propName == PropertyNames::Fermentable::fermentability_pct       ) { this->lineEdit_fermentability_pct       ->setQuantity    (m_editItem->fermentability_pct       ()); if (propName) { return; } }
    if (!propName || *propName == PropertyNames::Fermentable::betaGlucan_ppm           ) { this->lineEdit_betaGlucan               ->setQuantity    (m_editItem->betaGlucan_ppm               ()); if (propName) { return; } }
 
+   this->label_id_value->setText(QString::number(m_editItem->key()));
    return;
 }
 

diff --git a/src/editors/HopEditor.cpp b/src/editors/HopEditor.cpp
@@ -160,6 +160,7 @@ void HopEditor::readFieldsFromEditItem(std::optional<QString> propName) {
    if (!propName || *propName == PropertyNames::Hop::polyphenols_pct) { this->lineEdit_polyphenols->setQuantity(m_editItem->polyphenols_pct()); if (propName) { return; } }
    if (!propName || *propName == PropertyNames::Hop::xanthohumol_pct) { this->lineEdit_xanthohumol->setQuantity(m_editItem->xanthohumol_pct()); if (propName) { return; } }
 
+   this->label_id_value->setText(QString::number(m_editItem->key()));
    return;
 }
 

diff --git a/src/editors/MiscEditor.cpp b/src/editors/MiscEditor.cpp
@@ -92,6 +92,7 @@ void MiscEditor::readFieldsFromEditItem(std::optional<QString> propName) {
    if (!propName || *propName == PropertyNames::Misc::producer   ) { this->lineEdit_producer ->setTextCursor(m_editItem->producer ()); if (propName) { return; } }
    if (!propName || *propName == PropertyNames::Misc::productId  ) { this->lineEdit_productId->setTextCursor(m_editItem->productId()); if (propName) { return; } }
 
+   this->label_id_value->setText(QString::number(m_editItem->key()));
    return;
 }
 

diff --git a/src/editors/YeastEditor.cpp b/src/editors/YeastEditor.cpp
@@ -144,6 +144,7 @@ void YeastEditor::readFieldsFromEditItem(std::optional<QString> propName) {
    if (!propName || *propName == PropertyNames::Yeast::killerProducingKlusToxin ) { this->boolCombo_killerProducingKlusToxin ->setValue(m_editItem->killerProducingKlusToxin ()); if (propName) { return; } }
    if (!propName || *propName == PropertyNames::Yeast::killerNeutral            ) { this->boolCombo_killerNeutral            ->setValue(m_editItem->killerNeutral            ()); if (propName) { return; } }
 
+   this->label_id_value->setText(QString::number(m_editItem->key()));
    return;
 }
 

diff --git a/src/utils/PropertyPath.cpp b/src/utils/PropertyPath.cpp
@@ -138,7 +138,7 @@ QVariant PropertyPath::getValue(NamedEntity const & obj) const {
    NamedEntity const * ne = &obj;
    for (auto const property : this->m_properties) {
       // Normally keep the next line commented out otherwise it generates too many lines in the log file
-      qDebug() << Q_FUNC_INFO << "Looking at" << *property;
+//      qDebug() << Q_FUNC_INFO << "Looking at" << *property;
 
       if (property == this->m_properties.last()) {
          //
@@ -157,10 +157,10 @@ QVariant PropertyPath::getValue(NamedEntity const & obj) const {
          QMetaProperty neMetaProperty = neMetaObject->property(propertyIndex);
 
          // Normally keep this log statement commented out otherwise it generates too many lines in the log file
-         qDebug() <<
-            Q_FUNC_INFO << "Request to get" << this->m_path << "on" << obj.metaObject()->className() << "(=" <<
-            *property << "on" << ne->metaObject()->className() << "); property type =" << neMetaProperty.typeName() <<
-            "; readable =" << neMetaProperty.isReadable();
+//         qDebug() <<
+//            Q_FUNC_INFO << "Request to get" << this->m_path << "on" << obj.metaObject()->className() << "(=" <<
+//            *property << "on" << ne->metaObject()->className() << "); property type =" << neMetaProperty.typeName() <<
+//            "; readable =" << neMetaProperty.isReadable();
 
          if (neMetaProperty.isReadable()) {
             retVal = ne->property(**property);

diff --git a/translations/bt_ca.ts b/translations/bt_ca.ts
@@ -7114,6 +7114,14 @@ El volum final al primari és de %1.</translation>
         <source>Amount Type</source>
         <translation type="unfinished">Tipus de quantitat</translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -7182,11 +7190,11 @@ El volum final al primari és de %1.</translation>
     </message>
     <message>
         <source>Temp. lag time</source>
-        <translation type="unfinished">Lapse entre temparatures</translation>
+        <translation type="obsolete">Lapse entre temparatures</translation>
     </message>
     <message>
         <source>Lag time</source>
-        <translation type="unfinished">Lapse de temps</translation>
+        <translation type="obsolete">Lapse de temps</translation>
     </message>
     <message>
         <source>End temp.</source>

diff --git a/translations/bt_cs.ts b/translations/bt_cs.ts
@@ -6966,6 +6966,14 @@ Celkový objem pro hlavní kvašení je %1.</translation>
         <source>Amount Type</source>
         <translation type="unfinished">Druh množství</translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -7034,11 +7042,11 @@ Celkový objem pro hlavní kvašení je %1.</translation>
     </message>
     <message>
         <source>Temp. lag time</source>
-        <translation type="unfinished">Prodleva</translation>
+        <translation type="obsolete">Prodleva</translation>
     </message>
     <message>
         <source>Lag time</source>
-        <translation type="unfinished">Čas pro odpočinek</translation>
+        <translation type="obsolete">Čas pro odpočinek</translation>
     </message>
     <message>
         <source>End temp.</source>

diff --git a/translations/bt_de.ts b/translations/bt_de.ts
@@ -7015,6 +7015,14 @@ Das endgültige Volumen in der Hauptgärung beträgt %1.</translation>
         <source>Amount Type</source>
         <translation type="unfinished">Mengenangabe</translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -7083,11 +7091,11 @@ Das endgültige Volumen in der Hauptgärung beträgt %1.</translation>
     </message>
     <message>
         <source>Temp. lag time</source>
-        <translation type="unfinished">Temperatur-Anstiegszeit</translation>
+        <translation type="obsolete">Temperatur-Anstiegszeit</translation>
     </message>
     <message>
         <source>Lag time</source>
-        <translation type="unfinished">Verzögerung</translation>
+        <translation type="obsolete">Verzögerung</translation>
     </message>
     <message>
         <source>End temp.</source>

diff --git a/translations/bt_el.ts b/translations/bt_el.ts
@@ -7000,6 +7000,14 @@ The final volume in the primary is %1.</source>
         <source>Amount Type</source>
         <translation type="unfinished">Μονάδα μέτρησης</translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -7068,11 +7076,11 @@ The final volume in the primary is %1.</source>
     </message>
     <message>
         <source>Temp. lag time</source>
-        <translation type="unfinished">Temp. lag time</translation>
+        <translation type="obsolete">Temp. lag time</translation>
     </message>
     <message>
         <source>Lag time</source>
-        <translation type="unfinished">Lag time</translation>
+        <translation type="obsolete">Lag time</translation>
     </message>
     <message>
         <source>End temp.</source>

diff --git a/translations/bt_en.ts b/translations/bt_en.ts
@@ -5408,6 +5408,14 @@ The final volume in the primary is %1.</source>
         <source>Amount Type</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -5474,14 +5482,6 @@ The final volume in the primary is %1.</source>
         <source>Free rise is used to indicate a fermentation step where the exothermic fermentation is allowed to raise the temperature without restriction</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>Temp. lag time</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>Lag time</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>End temp.</source>
         <translation type="unfinished"></translation>

diff --git a/translations/bt_es.ts b/translations/bt_es.ts
@@ -7074,6 +7074,14 @@ El volumen final en el primario es %1.</translation>
         <source>Amount Type</source>
         <translation type="unfinished">Tipo de Cantidad</translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -7142,11 +7150,11 @@ El volumen final en el primario es %1.</translation>
     </message>
     <message>
         <source>Temp. lag time</source>
-        <translation type="unfinished">Lapso de la temp.</translation>
+        <translation type="obsolete">Lapso de la temp.</translation>
     </message>
     <message>
         <source>Lag time</source>
-        <translation type="unfinished">Tiempo del lapso</translation>
+        <translation type="obsolete">Tiempo del lapso</translation>
     </message>
     <message>
         <source>End temp.</source>

diff --git a/translations/bt_et.ts b/translations/bt_et.ts
@@ -5500,6 +5500,14 @@ The final volume in the primary is %1.</source>
         <source>Amount Type</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>ID in database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The ID of this fermentable in the database.  Sometimes useful for debugging.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>fermentationEditor</name>
@@ -5566,14 +5574,6 @@ The final volume in the primary is %1.</source>
         <source>Free rise is used to indicate a fermentation step where the exothermic fermentation is allowed to raise the temperature without restriction</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>Temp. lag time</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>Lag time</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>End temp.</source>
         <translation type="unfinished"></translation>
-Original file line number
+Diff line change
@@ Expand Up @@
        if (!propName || *propName == PropertyNames::Fermentable::fermentability_pct       ) { this->lineEdit_fermentability_pct       ->setQuantity    (m_editItem->fermentability_pct       ()); if (propName) { return; } }
        if (!propName || *propName == PropertyNames::Fermentable::betaGlucan_ppm           ) { this->lineEdit_betaGlucan               ->setQuantity    (m_editItem->betaGlucan_ppm               ()); if (propName) { return; } }
+       this->label_id_value->setText(QString::number(m_editItem->key()));
        return;
     }
@@ Expand Down @@