From e5a43c1d7babbaf9450a14e5ea1e4589235ded2c Mon Sep 17 00:00:00 2001 From: mikfire Date: Wed, 31 Mar 2021 11:20:17 -0400 Subject: [PATCH 1/2] Closes #532 -- Problem with amounts in inventory The code was working as designed, I just had a bad design. I had missed this use-case, and so the original's inventory_id was copied along with the rest. The fix was to add a boolean to the new* calls in Database that allowed us to specify if we wanted a new inventory row or not. Since this allowed me to declutter some of the code, I think it is a reasonable approach. The only path I could find that needed this was from BtTreeModel::copySelected, so that method got modified to request the new inventory row. I had considered putting the code to add a new inventory row here, which had the benefit of keeping the complexity close to where it was used. But I didn't like the model having to open a transaction, so I put into the Database methods. My quick testing makes me think this is working better. --- src/BtTreeModel.cpp | 12 ++++-- src/database.cpp | 92 +++++++++++++++++++++++---------------------- src/database.h | 8 ++-- 3 files changed, 60 insertions(+), 52 deletions(-) diff --git a/src/BtTreeModel.cpp b/src/BtTreeModel.cpp index 8d9423253..b5fbf2fea 100644 --- a/src/BtTreeModel.cpp +++ b/src/BtTreeModel.cpp @@ -838,7 +838,8 @@ void BtTreeModel::copySelected(QList< QPair > toBeCopied) case BtTreeItem::FERMENTABLE: Fermentable *copyFerm, *oldFerm; oldFerm = fermentable(ndx); - copyFerm = Database::instance().newFermentable(oldFerm); // Create a deep copy. + // Create a deep copy with a new inventory row + copyFerm = Database::instance().newFermentable(oldFerm,true); if ( copyFerm ) copyFerm->setName(name); else @@ -847,7 +848,8 @@ void BtTreeModel::copySelected(QList< QPair > toBeCopied) case BtTreeItem::HOP: Hop *copyHop, *oldHop; oldHop = hop(ndx); - copyHop = Database::instance().newHop(oldHop); // Create a deep copy. + // Create a deep copy with a new inventory row + copyHop = Database::instance().newHop(oldHop,true); if ( copyHop ) copyHop->setName(name); else @@ -856,7 +858,8 @@ void BtTreeModel::copySelected(QList< QPair > toBeCopied) case BtTreeItem::MISC: Misc *copyMisc, *oldMisc; oldMisc = misc(ndx); - copyMisc = Database::instance().newMisc(oldMisc); // Create a deep copy. + // Create a deep copy with a new inventory row + copyMisc = Database::instance().newMisc(oldMisc,true); if ( copyMisc ) copyMisc->setName(name); else @@ -883,7 +886,8 @@ void BtTreeModel::copySelected(QList< QPair > toBeCopied) case BtTreeItem::YEAST: Yeast *copyYeast, *oldYeast; oldYeast = yeast(ndx); - copyYeast = Database::instance().newYeast(oldYeast); // Create a deep copy. + // Create a deep copy with a new inventory row + copyYeast = Database::instance().newYeast(oldYeast,true); if ( copyYeast ) copyYeast->setName(name); else diff --git a/src/database.cpp b/src/database.cpp index e2f9445ee..3c106f5ba 100644 --- a/src/database.cpp +++ b/src/database.cpp @@ -1470,35 +1470,33 @@ Equipment* Database::newEquipment(Equipment* other) return tmp; } -Fermentable* Database::newFermentable(Fermentable* other) +Fermentable* Database::newFermentable(Fermentable* other, bool add_inventory) { Fermentable* tmp; - bool transact = false; + add_inventory = add_inventory || other == nullptr; + sqlDatabase().transaction(); try { - // copies automatically get their inventory_id properly set - if (other) { + if (other != nullptr) { tmp = copy(other, &allFermentables); } else { - // new ingredients don't. this gets ugly fast, because we are now - // writing to two tables and need some transactional protection - sqlDatabase().transaction(); - transact = true; tmp = newIngredient(&allFermentables); + } + + if ( add_inventory ) { int invkey = newInventory( dbDefn->table(Brewtarget::FERMTABLE)); tmp->setInventoryId(invkey); } } catch (QString e) { qCritical() << QString("%1 %2").arg(Q_FUNC_INFO).arg(e); - if ( transact ) sqlDatabase().rollback(); - throw; + sqlDatabase().rollback(); + abort(); } - if ( transact ) { - sqlDatabase().commit(); - } + sqlDatabase().commit(); + if ( tmp ) { emit changed( metaProperty("fermentables"), QVariant() ); emit newFermentableSignal(tmp); @@ -1510,32 +1508,30 @@ Fermentable* Database::newFermentable(Fermentable* other) return tmp; } -Hop* Database::newHop(Hop* other) +Hop* Database::newHop(Hop* other, bool add_inventory ) { Hop* tmp; - bool transact = false; + add_inventory = add_inventory || other == nullptr; + sqlDatabase().transaction(); try { - if ( other ) { + if ( other != nullptr ) tmp = copy(other, &allHops); - } - else { - sqlDatabase().transaction(); - transact = true; + else tmp = newIngredient(&allHops); + + if ( add_inventory ) { int invkey = newInventory( dbDefn->table(Brewtarget::HOPTABLE)); tmp->setInventoryId(invkey); } } catch (QString e) { qCritical() << QString("%1 %2").arg(Q_FUNC_INFO).arg(e); - if ( transact ) sqlDatabase().rollback(); - throw; + sqlDatabase().rollback(); + abort(); } - if ( transact ) { - sqlDatabase().commit(); - } + sqlDatabase().commit(); if ( tmp ) { emit changed( metaProperty("hops"), QVariant() ); @@ -1733,32 +1729,32 @@ MashStep* Database::newMashStep(Mash* mash, bool connected) return tmp; } -Misc* Database::newMisc(Misc* other) +Misc* Database::newMisc(Misc* other, bool add_inventory) { Misc* tmp; - bool transact = false; + add_inventory = add_inventory || other == nullptr; + sqlDatabase().transaction(); try { - if ( other ) { + if ( other != nullptr ) { tmp = copy(other, &allMiscs); } else { - sqlDatabase().transaction(); - transact = true; tmp = newIngredient(&allMiscs); + } + + if ( add_inventory ) { int invkey = newInventory( dbDefn->table(Brewtarget::MISCTABLE)); tmp->setInventoryId(invkey); } } catch (QString e) { qCritical() << QString("%1 %2").arg(Q_FUNC_INFO).arg(e); - if ( transact ) sqlDatabase().rollback(); - throw; + sqlDatabase().rollback(); + abort(); } - if ( transact ) { - sqlDatabase().commit(); - } + sqlDatabase().commit(); if ( tmp ) { emit changed( metaProperty("miscs"), QVariant() ); @@ -1939,19 +1935,21 @@ Salt* Database::newSalt(Salt* other) return tmp; } -Yeast* Database::newYeast(Yeast* other) +Yeast* Database::newYeast(Yeast* other, bool add_inventory) { Yeast* tmp; - bool transact = false; + add_inventory = add_inventory || other == nullptr; + sqlDatabase().transaction(); try { - if (other) { + if (other != nullptr) { tmp = copy(other, &allYeasts); } else { - sqlDatabase().transaction(); - transact = true; tmp = newIngredient(&allYeasts); + } + + if (add_inventory) { int invkey = newInventory( dbDefn->table(Brewtarget::YEASTTABLE)); tmp->setInventoryId(invkey); } @@ -1962,11 +1960,17 @@ Yeast* Database::newYeast(Yeast* other) throw; } - if ( transact ) { - sqlDatabase().commit(); + sqlDatabase().commit(); + + if ( tmp ) { + emit changed( metaProperty("yeasts"), QVariant() ); + emit newYeastSignal(tmp); + } + else { + qCritical() << QString("%1 could not %2 yeast") + .arg(Q_FUNC_INFO) + .arg( other ? "copy" : "create"); } - emit changed( metaProperty("yeasts"), QVariant() ); - emit newYeastSignal(tmp); return tmp; } diff --git a/src/database.h b/src/database.h index de63f09b0..ec51c5c04 100644 --- a/src/database.h +++ b/src/database.h @@ -209,20 +209,20 @@ class Database : public QObject //! \returns a copy of the given note. BrewNote* newBrewNote(BrewNote* other, bool signal = true); Equipment* newEquipment(Equipment* other = nullptr); - Fermentable* newFermentable(Fermentable* other = nullptr); - Hop* newHop(Hop* other = nullptr); + Fermentable* newFermentable(Fermentable* other = nullptr, bool add_inventory = false); + Hop* newHop(Hop* other = nullptr, bool add_inventory = false); //! \returns a copy of the given recipe. Recipe* newRecipe(Recipe* other); /*! \returns a copy of the given mash. Displaces the mash currently in the * parent recipe unless \b displace is false. */ - Misc* newMisc(Misc* other = nullptr); + Misc* newMisc(Misc* other = nullptr, bool add_inventory = false); Style* newStyle(Style* other); Style* newStyle(QString name); Water* newWater(Water* other = nullptr); Salt* newSalt(Salt* other = nullptr); - Yeast* newYeast(Yeast* other = nullptr); + Yeast* newYeast(Yeast* other = nullptr, bool add_inventory = false); int insertElement(Ingredient* ins); int insertEquipment(Equipment* ins); From 3c6f3a022e2e330d33c2756c58050d7ebf816537 Mon Sep 17 00:00:00 2001 From: mikfire Date: Wed, 7 Apr 2021 13:57:28 -0400 Subject: [PATCH 2/2] Fixes the PR I messed my order of operations up and didn't do a full refresh before I addressed the issue. It caused problems, as newIngredient had changed to newNamedEntity. Its fixed now. --- src/database.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/database.cpp b/src/database.cpp index fc1a10d7a..5cb7a74db 100644 --- a/src/database.cpp +++ b/src/database.cpp @@ -1490,7 +1490,7 @@ Fermentable* Database::newFermentable(Fermentable* other, bool add_inventory) tmp = copy(other, &allFermentables); } else { - tmp = newIngredient(&allFermentables); + tmp = newNamedEntity(&allFermentables); } if ( add_inventory ) { @@ -1527,7 +1527,7 @@ Hop* Database::newHop(Hop* other, bool add_inventory ) if ( other != nullptr ) tmp = copy(other, &allHops); else - tmp = newIngredient(&allHops); + tmp = newNamedEntity(&allHops); if ( add_inventory ) { int invkey = newInventory( dbDefn->table(Brewtarget::HOPTABLE)); @@ -1750,7 +1750,7 @@ Misc* Database::newMisc(Misc* other, bool add_inventory) tmp = copy(other, &allMiscs); } else { - tmp = newIngredient(&allMiscs); + tmp = newNamedEntity(&allMiscs); } if ( add_inventory ) { @@ -1956,7 +1956,7 @@ Yeast* Database::newYeast(Yeast* other, bool add_inventory) tmp = copy(other, &allYeasts); } else { - tmp = newIngredient(&allYeasts); + tmp = newNamedEntity(&allYeasts); } if (add_inventory) {