diff --git a/CHANGES.markdown b/CHANGES.markdown index 791c4aad..2379c2c7 100644 --- a/CHANGES.markdown +++ b/CHANGES.markdown @@ -21,9 +21,10 @@ Bug fixes and minor enhancements. ### Bug Fixes * Crash (stack overflow in Qt) on some Windows builds during "check for new version" [866](https://github.com/Brewtarget/brewtarget/issues/866) +* Crash when copying recipe, then on adding new steps in mash, ferment, boil, others [868](https://github.com/Brewtarget/brewtarget/issues/868) ### Release Timestamp -Tue, 29 Oct 2024 04:00:09 +0100 +Sun, 3 Nov 2024 04:00:09 +0100 ## v4.0.8 Bug fixes and minor enhancements. diff --git a/src/model/NamedEntity.cpp b/src/model/NamedEntity.cpp index d7cdfb84..0ad42814 100644 --- a/src/model/NamedEntity.cpp +++ b/src/model/NamedEntity.cpp @@ -461,9 +461,12 @@ void NamedEntity::prepareForPropertyChange(BtStringConst const & propertyName) { void NamedEntity::propagatePropertyChange(BtStringConst const & propertyName, bool notify) const { if (!this->m_propagationAndSignalsEnabled) { - qDebug() << - Q_FUNC_INFO << "Not propagating" << *propertyName << "change on" << this->metaObject()->className() << - "as m_propagationAndSignalsEnabled unset"; + // + // Normally leave this log statement commented out as otherwise it can generate a lot of lines in the log files + // +// qDebug() << +// Q_FUNC_INFO << "Not propagating" << *propertyName << "change on" << this->metaObject()->className() << +// "as m_propagationAndSignalsEnabled unset"; return; } diff --git a/src/model/OwnedByRecipe.h b/src/model/OwnedByRecipe.h index 5402bdfe..c9b07864 100755 --- a/src/model/OwnedByRecipe.h +++ b/src/model/OwnedByRecipe.h @@ -68,6 +68,10 @@ class OwnedByRecipe : public NamedEntity { int recipeId() const; std::shared_ptr recipe() const; + // TODO: ownerId / setOwnerId are needed by OwnedSet. Should ideally merge them with recipeId / setRecipeId + inline void setOwnerId(int const val) { setRecipeId(val); return; } + inline int ownerId() const { return recipeId(); } + protected: virtual bool isEqualTo(NamedEntity const & other) const override; diff --git a/src/model/OwnedSet.h b/src/model/OwnedSet.h new file mode 100644 index 00000000..5a5a004b --- /dev/null +++ b/src/model/OwnedSet.h @@ -0,0 +1,607 @@ +/*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ + * model/OwnedSet.h is part of Brewken, and is copyright the following authors 2024: + * • Matt Young + * + * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later + * version. + * + * Brewtarget is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with this program. If not, see + * . + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌*/ +#ifndef MODEL_OWNEDSET_H +#define MODEL_OWNEDSET_H +#pragma once + +#include +#include + +#include +#include + +#include "utils/BtStringConst.h" +#include "utils/TypeTraits.h" + + +struct OwnedSetOptions { + /** + * \brief In an enumerated set, items are in a strict order and are accordingly numbered (starting from 1) with a + * step number, which is accessed via \c seqNum / \c setSeqNum. \c OwnedSet::items returns items in + * this order. In a non-enumerated set, the \c OwnedSet class does not manage the order of the items and + * returns them in arbitrary order. (Typically there may be a weak ordering -- eg addition time for a + * \c RecipeAddition subclass or brew date for a \c BrewnNote -- but it is not managed inside of \c OwnedSet. + */ + bool enumerated = false; + + /** + * \brief By default, when an object owning a set is copied, we want to do a deep copy of that set - ie create a new + * set containing copies of each item in the original set. This is what \c copyable being \c true means. + * + * The only other option is that, when the owning object is copied, the set is not copied at all, and the set + * on the newly-created copy is empty. This is, for instance, what we do with any \c BrewNote objects on a + * \c Recipe. This is what \c copyable being \c false means. + * + * + * + * + * Note that it can never be possible to do a shallow copy of an \c OwnedSet, since an item in such a set + * cannot meaningfully have two different owners. + */ + bool copyable = true; +}; +template struct is_Enumerated : public std::integral_constant{}; +template struct is_Copyable : public std::integral_constant{}; +// See comment in utils/TypeTraits.h for definition of CONCEPT_FIX_UP (and why, for now, we need it) +template concept CONCEPT_FIX_UP IsEnumerated = is_Enumerated::value; +template concept CONCEPT_FIX_UP IsCopyable = is_Copyable ::value; + +/** + * \brief Template class that handles ownership of either: + * - an "unordered" set of things - eg \c Recipe ownership of \c RecipeAdditionFermentable, + * \c RecipeAdditionHop etc; or + * - an "ordered", aka "enumerated" list of "steps" - eg \c Mash ownership of \c MashSteps + * + * For an "unordered" set, we do not have a strict ordering of the owned items (eg two hop additions could happen + * at the same time). + * + * \param Owner class needs to inherit from \c NamedEntity + * \param Item class also needs to inherit from \c NamedEntity, plus implement: \c ownerId, \c setOwnerId + * For an enumerated set, \c Item also needs to implement \c seqNum, \c setSeqNum + * \param propertyName is the name of the property that we use to signal changes to the size of the owned set (eg item + * added or removed). It is typically the name of the property holding this \c OwnedSet object, but the value + * that we send with the \c changed signal is simply the new size of the set. + * \param itemChangedSlot is a member function slot on \c Owner that can receive \c NamedEntity::changed signals from + * \c Item objects in this set. Typically that member function just needs to call our \c acceptItemChange + * function. It is simpler to go via \c Owner because the owner object is able to call \c QObject::sender to + * get the sender and pass it to us. (From looking at the Qt source code eg at + * https://github.com/qt/qtbase/blob/dev/src/corelib/kernel/qobject.cpp, it seems \c QObject::sender will return + * null if a slot on the object in question is not being invoked, so there is no use us trying to find ways to + * call \c this->m_owner.sender(), as we'll always just get back \c nullptr.) + * \param ownedSetOptions See \c OwnedSetOptions + */ +template +class OwnedSet { +public: + /** + * \brief Minimal constructor. Note that the reason we do not just initialise everything here is that the object + * stores on which we depend might not yet themselves be initialised when this constructor is called. + * + * \param owner should be a subclass of \c NamedEntity and we should be being called from its constructor + */ + OwnedSet(Owner & owner) : + m_owner{owner} { + // + // Note that it is always correct in this constructor for m_itemIds to start out as an empty set. It is only used + // when adding items to an owner that has not yet been stored in the DB. Either a brand new owner is just being + // created, so it doesn't yet have any items in this OwnedSet, or we just read it out of the DB (in which case + // we'll be able, in due course, to get the owned items from the relevant object store (once that has been + // populated from the DB) so we don't need a list of item IDs. + // + return; + } + + /** + * \brief "Copy" constructor for when we want a no-op copy. + */ + OwnedSet(Owner & owner, [[maybe_unused]] OwnedSet const & other) requires (!IsCopyable) : + m_owner{owner} { + return; + } + + /** + * \brief "Copy" constructor for when we want a deep copy. + */ + OwnedSet(Owner & owner, OwnedSet const & other) requires (IsCopyable) : + m_owner{owner} { + // Deep copy of Steps + auto otherItems = other.items(); + for (auto item : otherItems) { + // Make a copy of the current Item object we're looking at in the other OwnedSet + auto itemToAdd = std::make_shared(*item); + + // The owner won't yet have an ID, so we can't give it to the new Item + itemToAdd->setOwnerId(-1); + + // However, if we insert the new Item in the object store, that will give it its own ID + ObjectStoreWrapper::insert(itemToAdd); + + // Store the ID of the copy Item + // If and when we get our ID then we can give it to our Items + this->m_itemIds.append(itemToAdd->key()); + + // Connect signals so that we are notified when there are changes to the Item we just added to our Owner. + this->connectItemChangedSignal(itemToAdd); + } + + return; + } + + /** + * \brief We have to delete the default copy constructor that the compiler would generate because that would copy + * m_owner, which we don't want. Instead we need to force callers to provide the new owner as a parameter + * (via the constructor above). + */ + OwnedSet(OwnedSet const & other) = delete; + + /** + * \brief Similarly, we don't want copy assignment happening. + */ + OwnedSet & operator=(OwnedSet const & other) = delete; + + ~OwnedSet() = default; + + /** + * \brief If one of the items in our set changes, we receive its "changed" signal and emit a "changed" signal for the + * the set. + * + * TBD: At the moment we emit the same signal for "number of items in the set changed" and "a property of one + * of the items in the set changed". In future, we could do something more sophisticated here if need + * be. + */ + void acceptItemChange(Item const & item, [[maybe_unused]] QMetaProperty prop, [[maybe_unused]] QVariant val) { + // If one of our steps changed, our pseudo properties may also change, so we need to emit some signals + if (item.ownerId() == this->m_owner.key()) { + emit this->m_owner.changed(this->m_owner.metaProperty(*propertyName), QVariant()); + } + return; + } + +private: + /** + * \brief Connect an item's "changed" signal to us. + * + * Unusually, we don't worry about disconnecting this later. The \c Item object (\c item) will never belong + * to any other \c OwnedSet, and Qt will do the disconnection itself when \c item is destroyed. + */ + void connectItemChangedSignal(std::shared_ptr item) { + this->m_owner.connect(item.get(), &NamedEntity::changed, &this->m_owner, itemChangedSlot); + return; + } + + /** + * \brief Connect all our item's "changed" signals to us + * + * Needs to be called by our Owner \b after all the calls to ObjectStoreTyped::getInstance().loadAll() + */ + void connectAllItemChangedSignals() { + for (auto item : this->items()) { + this->connectItemChangedSignal(item); + } + return; + } + + //! No-op version + void putInOrder([[maybe_unused]] QList> items) const requires (!IsEnumerated) { + return; + } + //! Substantive version + void putInOrder(QList> items) const requires (IsEnumerated) { + std::sort(items.begin(), + items.end(), + [](std::shared_ptr const lhs, std::shared_ptr const rhs) { + return lhs->seqNum() < rhs->seqNum(); + }); + return; + } + + //! No-op version + void normaliseSeqNums([[maybe_unused]] QList> items) const requires (!IsEnumerated) { + return; + } + //! Substantive version + void normaliseSeqNums(QList> items) const requires (IsEnumerated) { + for (int ii = 0; ii < items.size(); ++ii) { + // + // Canonical item numbering starts from 1, which is +1 on canonical indexing! + // + int const canonicalSeqNum = ii + 1; + if (items[ii]->seqNum() != canonicalSeqNum) { + items[ii]->setSeqNum(canonicalSeqNum); + } + } + return; + } + +public: + QList> items() const { + // + // The Owner object (eg Recipe) owns its Items (eg RecipeAdditionFermentables, RecipeAdditionHops, etc). But, + // it's the Item that knows which Owner it's in rather than the Owner which knows which Items it has, so we have + // to ask. The only exception to this is if the Owner is not yet stored in the DB, in which case there is not yet + // any Owner ID to give to the Items, so we store an internal list of them. + // + int const ownerId = this->m_owner.key(); + + QList> items; + if (ownerId < 0) { + for (int ii : this->m_itemIds) { + items.append(ObjectStoreWrapper::getById(ii)); + } + // We don't need to sort here as we assume m_itemIds is already in the correct order (if there is one) + + } else { + items = ObjectStoreWrapper::findAllMatching( + [ownerId](std::shared_ptr const item) {return item->ownerId() == ownerId && !item->deleted();} + ); + } + + // + // The object store does not guarantee what order it returned the items in, so, if they are enumerated, we need + // to put them in the right order. The same comment applies to our m_itemIds list. For enumerated sets, we + // _could_ enforce that the order in m_itemIds is the same as the ordering implied by seqNum() on the set + // members, but this would make other parts of the code a bit more complicated (where we share logic between + // enumerated and non-enumerated sets) for little if no gain. + // + this->putInOrder(items); + + // + // It can be that, although they are in the right order, the items are not canonically numbered. If this happens, + // it looks a bit odd in the UI -- eg because you have Instructions in a Recipe starting with Instruction #2 as + // the first one. We _could_ fix this in the UI layer, but it's easier to do it here -- and, since we're never + // talking about more than a handful of items (often less than 10, usually less than 20, pretty much always less + // than 30), the absolute overhead of doing so should be pretty small. + // + this->normaliseSeqNums(items); + + return items; + } + + /** + * \brief For the moment we only do the unenumerated version of this. For the enumerated version, we would implement + * in terms of \c items(). + */ + QVector itemIds() const { + int const ownerId = this->m_owner.key(); + if (ownerId < 0) { + return this->m_itemIds; + } + + return ObjectStoreWrapper::idsOfAllMatching( + [ownerId](Item const * item) { return item->ownerId() == ownerId; } + ); + } + +private: + /** + * \brief Adds a new item to the set. This is private because, for enumerated sets, we need to handle step number in + * the public functions that calls this one -- so we don't want it to be possible for this to be called from + * outside the class for an enumerated set. + */ + std::shared_ptr extend(std::shared_ptr item) { + if (this->m_owner.key() > 0) { + qDebug() << + Q_FUNC_INFO << "Add" << Item::staticMetaObject.className() << "#" << item->key() << "to" << + Owner::staticMetaObject.className() << "#" << this->m_owner.key(); + item->setOwnerId(this->m_owner.key()); + } + + // Item needs to be in the DB for us to add it to the Owner + if (item->key() < 0) { + qDebug() << + Q_FUNC_INFO << "Inserting" << Item::staticMetaObject.className() << "in DB for" << + Owner::staticMetaObject.className() << "#" << this->m_owner.key() << "(" << item->ownerId() << ")"; + ObjectStoreWrapper::insert(item); + } + + Q_ASSERT(item->key() > 0); + + // + // If the Owner itself is not yet stored in the DB then it needs to hang on to its list of Items so that, when the + // Owner does get stored, it can tell all the Items what their Owner ID is (see doSetKey()). + // + // (Conversely, if the Owner is in the DB, then we don't need to do anything further. We can get all our Items + // any time by just asking the relevant ObjectStore for all Items with Owner ID the same as ours.) + // + if (this->m_owner.key() < 0) { + qDebug() << + Q_FUNC_INFO << "Adding" << Item::staticMetaObject.className() << "#" << item->key() << "to" << + Owner::staticMetaObject.className() << "#" << this->m_owner.key(); + // + // See comment above in items() for why, even in an enumerated set, we just append to this list rather than + // inserting the ID at its "correct" position. + // + this->m_itemIds.append(item->key()); + } + + // Now we changed the size of the set, have the owner tell people about it + emit this->m_owner.changed(this->m_owner.metaProperty(*propertyName), this->items().size()); + + return item; + } + +public: + /** + * \brief For enumerated owned sets, inserts the supplied item at the specified position in the list. If there is + * already an item in that position, it (and all subsequent ones) will be bumped one place down the list. + * + * \param item + * \param seqNum counted from 1 (or 0 to append to the end of the list) + */ + std::shared_ptr insert(std::shared_ptr item, + int const seqNum) requires (IsEnumerated) { + auto existingItems = this->items(); + + // We'll treat any out of range sequence number as meaning "append to the end" + if (seqNum < 1 || seqNum > existingItems.size() + 1) { + seqNum = existingItems.size() + 1; + } + + // Note per https://en.cppreference.com/w/cpp/ranges/drop_view that dropping more than the number of elements is + // OK (and just gives an empty range. + for (auto existingItem : std::ranges::drop_view(existingItems, seqNum - 1)) { + existingItem->setSeqNum(existingItem->seqNum() + 1); + } + + item->setSeqNum(seqNum); + + return this->extend(item); + } + + /** + * \brief Adds a new item (at the end of the current list if it's an enumerated owned set) + */ + std::shared_ptr add(std::shared_ptr item) requires (!IsEnumerated) { + return this->extend(item); + } + std::shared_ptr add(std::shared_ptr item) requires (IsEnumerated) { + return this->insert(item, 0); + } + + /** + * \brief Remove the specified item from the set and delete it from the DB + * + * \return Pointer to the removed item (which caller now owns) + */ + std::shared_ptr remove(std::shared_ptr item) { + // It's a coding error if we try to remove an item that didn't belong to the owner + Q_ASSERT(item->ownerId() == this->m_owner.key()); + + // Disassociate the Item from its Owner + item->setOwnerId(-1); + + // As per add(), if we're not yet stored in the database, then we also need to update our list of Items. + if (this->m_owner.key() < 0) { + int indexOfItem = this->m_itemIds.indexOf(item->key()); + if (indexOfItem < 0 ) { + // This shouldn't happen, but it doesn't inherently break anything, so just log a warning and carry on + qWarning() << + Q_FUNC_INFO << "Tried to remove" << Item::staticMetaObject.className() << "#" << item->key() << + " (from unsaved" << Owner::staticMetaObject.className() << "#" << this->m_owner.key() << + ") but couldn't find it"; + } else { + this->m_itemIds.removeAt(indexOfItem); + } + } + + // + // Since a Owner owns its Items, we need to remove the Item from the DB when we remove it from the Owner. It then + // makes sense (in the context of undo/redo) to put the Item object back into "new" state, which ObjectStoreTyped + // will do for us. + // + ObjectStoreWrapper::hardDelete(item); + + // + // Note that this call to items() will also call this->normaliseSeqNums(), so, in an enumerated set, item sequence + // numbers will be adjusted/corrected for the fact that an item has been removed. + // + auto currentItems = this->items(); + + // Now we changed the size of the set, have the owner tell people about it + emit this->m_owner.changed(this->m_owner.metaProperty(*propertyName), currentItems.size()); + + return item; + } + + /** + * \brief Remove all items from the set and delete them from the DB + */ + void removeAll() { + auto items = this->items(); + qDebug() << + Q_FUNC_INFO << "Removing" << items.size() << Item::staticMetaObject.className() << "objects from" << + Owner::staticMetaObject.className() << "#" << this->m_owner.key(); + + if (items.size() > 0) { + for (auto item : items) { + ObjectStoreWrapper::hardDelete(*item); + } + this->m_itemIds.clear(); + + // Now we changed the size of the set, have the owner tell people about it + emit this->m_owner.changed(this->m_owner.metaProperty(*propertyName), 0); + } + return; + } + + /** + * \brief Sets all the items (in the supplied order if this is an enumerated set) + */ + void setAll(QList> const & val) { + this->removeAll(); + for (auto item : val) { + this->add(item); + } + return; + } + + /** + * \return Number of items in the set + */ + size_t size() const { + return this->items().size(); + } + + /*! + * \brief Swap the positions of Items \c lhs and \c rhs in an enumerated set + */ + void swap(Item & lhs, Item & rhs) requires (IsEnumerated) { + // It's a coding error if either of the items does not belong to this set + Q_ASSERT(lhs.ownerId() == this->m_owner.key()); + Q_ASSERT(rhs.ownerId() == this->m_owner.key()); + + // It's also a coding error if we're trying to swap a item with itself + Q_ASSERT(lhs.key() != rhs.key()); + + this->normaliseSeqNums(); + + qDebug() << + Q_FUNC_INFO << "Swapping items" << lhs.seqNum() << "(#" << lhs.key() << ") and " << rhs.seqNum() << " (#" << + rhs.key() << ")"; + + // Make sure we don't send notifications until the end (hence the false parameter on setSeqNum). + int temp = lhs.seqNum(); + lhs.setSeqNum(rhs.seqNum(), false); + rhs.setSeqNum(temp, false); + + // + // If the owner hasn't yet been put in the DB then we also need to swap things in our local list of item IDs + // + if (this->m_owner.key() < 0) { + int lhsIndex = this->m_itemIds.indexOf(lhs.key()); + int rhsIndex = this->m_itemIds.indexOf(rhs.key()); + // It's a coding error if we couldn't find either of the items + Q_ASSERT(lhsIndex >= 0); + Q_ASSERT(rhsIndex >= 0); + + this->m_itemIds.swapItemsAt(lhsIndex, rhsIndex); + } + + emit this->m_owner.itemsChanged(); + return; + } + + /** + * \brief Needs to be called from \c Owner::setKey + */ + void doSetKey(int key) { + qDebug() << + Q_FUNC_INFO << "Setting" << Owner::staticMetaObject.className() << "#" << this->m_owner.key() << "key on" << + this->m_itemIds.size() << Item::staticMetaObject.className() << "objects"; + // Give our ID (key) to our Items + for (auto itemId : this->m_itemIds) { + if (!ObjectStoreWrapper::contains(itemId)) { + // This is almost certainly a coding error, as each Item is owned by one Owner, but we can + // (probably) recover by ignoring the missing Item. + qCritical() << + Q_FUNC_INFO << "Unable to retrieve" << Item::staticMetaObject.className() << "#" << itemId << + "for" << Owner::staticMetaObject.className() << "#" << this->m_owner.key(); + } else { + ObjectStoreWrapper::getById(itemId)->setOwnerId(key); + } + } + return; + } + + /** + * \brief Needs to be called from Owner::hardDeleteOwnedEntities (which is virtual) + */ + void doHardDeleteOwnedEntities() { + // It's the Item that stores its Owner ID, so all we need to do is delete our Items then the subsequent database + // delete of this Owner won't hit any foreign key problems. + for (auto item : this->items()) { + ObjectStoreWrapper::hardDelete(*item); + } + return; + } + + /** + * \brief For an enumerated set, returns the item at the specified position, if it exists, or \c nullptr if not + * + * \param seqNum counted from 1 + */ + std::shared_ptr itemAt(int const seqNum) const requires (IsEnumerated) { + Q_ASSERT(seqNum > 0); + auto items = this->items(); + if (items.size() >= seqNum) { + return items[seqNum - 1]; + } + return nullptr; + } + + /** + * \brief For an enumerated set, sets (or unsets) the item at the specified position. + * + * Note this is different from \c insert(), as: + * - If there is a item in the specified position it will be overwritten rather than bumped down the list + * - Calling this with non-null value (ie not std::nullopt) for second and later items will ensure prior + * item(s) exist by creating default ones if necessary. + * - Calling this with a null value will delete any subsequent items. (Doesn't make sense for third item to + * become second in the context of this function.) + * + * \param item The item to set, or \c nullptr to unset it + * \param seqNum + */ + void setAt(std::shared_ptr item, int const seqNum) { + Q_ASSERT(seqNum > 0); + auto items = this->items(); + if (items.size() >= seqNum) { + // We already have a item of the number supplied, and possibly some subsequent ones + + if (item) { + // This is an easy case: we're replacing an existing item. This isn't the most efficient way to do things, + // but it has the merit of being less code, and the absolute overhead of doing things this way is small, + // because we are never dealing with huge sets (in fact not even more than a few dozen items). + this->remove(items[seqNum - 1]); + this->insert(item, seqNum); + return; + } + + // Caller supplied nullptr, so we're deleting this item and all the ones after it + for (int seqNumToDelete = items.size(); seqNumToDelete >= seqNum; --seqNumToDelete) { + this->remove(items[seqNumToDelete]); + } + return; + } + + // There isn't a item of the number supplied + if (!item) { + // Nothing to do if caller supplied std::nullopt + return; + } + + // We have to ensure any prior items exist + for (int seqNumToCreate = items.size(); seqNumToCreate < seqNum; ++seqNumToCreate) { + this->insert(std::make_shared(), seqNumToCreate); + } + this->insert(item, seqNum); + + return; + } + +private: + //================================================ MEMBER VARIABLES ================================================= + Owner & m_owner; + //! Note that this list is not in any particular order (see comments in \c items member function) + QVector m_itemIds = {}; +}; + + +#endif diff --git a/src/model/Recipe.cpp b/src/model/Recipe.cpp index 89f9b132..37754e0c 100644 --- a/src/model/Recipe.cpp +++ b/src/model/Recipe.cpp @@ -55,6 +55,7 @@ #include "model/MashStep.h" #include "model/Misc.h" #include "model/NamedParameterBundle.h" +#include "model/OwnedSet.h" #include "model/RecipeAdditionFermentable.h" #include "model/RecipeAdditionHop.h" #include "model/RecipeAdditionMisc.h" @@ -132,58 +133,6 @@ namespace { return false; } - /** - * \brief Decide whether the supplied instance of (subclass of) NamedEntity needs to be copied before being added to - * a recipe. - * - * \param var The Hop/Fermentable/etc that we want to add to a Recipe. We'll either add it directly or make a copy - * of it and add that. - * - * \return A copy of var if it needs to be copied (either because it has no parent or because it is already used in - * another recipe), or var itself otherwise - */ - template std::shared_ptr copyIfNeeded(NE & var) { - // It's the caller's responsibility to ensure var is already in an ObjectStore - Q_ASSERT(var.key() > 0); - - // - // If the supplied Hop/Fermentable/etc has no parent then we need to make a copy of it, because it's the master - // instance of that Hop/Fermentable/etc. - // - // Otherwise, if it has a parent, then whether we need to make a copy depends on whether it is already used in a - // recipe (_including_ this one, because the same ingredient can be added more than once to a recipe - eg Hops - // added at different times). - // - // All this logic is handled in isUnusedInstanceOfUseOf() because it's the same process for checking it's OK to - // delete something when it's been removed from a Recipe. - // - if (isUnusedInstanceOfUseOf(var)) { - return ObjectStoreWrapper::getById(var.key()); - } - - qDebug() << Q_FUNC_INFO << "Making copy of " << var.metaObject()->className() << "#" << var.key(); - - // We need to make a copy... - auto copy = std::make_shared(var); - // ...then make sure the copy is a "child" (ie "instance of use of")... - copy->makeChild(var); - // ...and finally ensure the copy is stored. - ObjectStoreWrapper::insert(copy); - return copy; - } - template<> std::shared_ptr copyIfNeeded(RecipeAdditionFermentable & var) = delete; - template<> std::shared_ptr copyIfNeeded(RecipeAdditionHop & var) = delete; - template<> std::shared_ptr copyIfNeeded(RecipeAdditionMisc & var) = delete; - template<> std::shared_ptr copyIfNeeded(RecipeAdditionYeast & var) = delete; - template<> std::shared_ptr copyIfNeeded(RecipeAdjustmentSalt & var) = delete; - template<> std::shared_ptr copyIfNeeded(RecipeUseOfWater & var) = delete; - template<> std::shared_ptr copyIfNeeded(Fermentable & var) = delete; - template<> std::shared_ptr copyIfNeeded(Hop & var) = delete; - template<> std::shared_ptr copyIfNeeded(Misc & var) = delete; - template<> std::shared_ptr copyIfNeeded(Yeast & var) = delete; - template<> std::shared_ptr copyIfNeeded(Salt & var) = delete; - template<> std::shared_ptr copyIfNeeded(Water & var) = delete; - bool isFermentableSugar(Fermentable * fermy) { // TODO: This probably doesn't work in languages other than English! if (fermy->type() == Fermentable::Type::Sugar && fermy->name() == "Milk Sugar (Lactose)") { @@ -224,11 +173,17 @@ class Recipe::impl { public: /** - * Constructor + * Constructors */ impl(Recipe & self) : m_self {self}, -/// instructionIds {} , + m_fermentableAdditions {self}, + m_hopAdditions {self}, + m_miscAdditions {self}, + m_yeastAdditions {self}, + m_saltAdjustments {self}, + m_waterUses {self}, + m_brewNotes {self}, m_ABV_pct {0.0}, m_color_srm {0.0}, m_boilGrav {0.0}, @@ -248,72 +203,49 @@ class Recipe::impl { return; } - /** - * Destructor - */ - ~impl() = default; - - /** - * \brief Make copies of the additions of a particular type (\c RecipeAdditionHop, \c RecipeAdditionFermentable, - * etc) from one \c Recipe and add them to another - typically because we are copying the \c Recipe. - * - * This also works for \c RecipeAdjustmentSalt and \c RecipeUseOfWater. - */ - template void copyAdditions(Recipe const & other) { - for (RA * otherAddition : other.pimpl->allMyRaw()) { - std::shared_ptr ourAddition = std::make_shared(*otherAddition); - this->m_self.addAddition(ourAddition); - } + impl(Recipe & self, [[maybe_unused]] NamedParameterBundle const & namedParameterBundle) : + // For the moment at least, we don't need any info from namedParameterBundle in this constuctor. + // Eg see comments in model/OwnedSet.h for more info on why owned sets do not need only minimal construction. + impl(self) { return; } -/// /** -/// * \brief Make copies of the Instructions from one Recipe and add them to another - typically -/// * because we are copying the Recipe. -/// */ -/// void copyInstructions(Recipe & us, Recipe const & other) { -/// qDebug() << Q_FUNC_INFO; -/// for (int otherInstructionId : other.pimpl->instructionIds) { -/// // Make and store a copy of the current Hop/Fermentable/etc object we're looking at in the other Recipe -/// auto otherInstruction = ObjectStoreWrapper::getById(otherInstructionId); -/// auto ourInstruction = copyIfNeeded(*otherInstruction); -/// // Store the ID of the copy in our recipe -/// this->instructionIds.append(ourInstruction->key()); -/// -/// qDebug() << -/// Q_FUNC_INFO << "After adding Instruction #" << ourInstruction->key() << -/// ", Recipe" << us.name() << "has" << this->instructionIds.size() << "Instructions"; -/// -/// // Connect signals so that we are notified when there are changes to the Hop/Fermentable/etc we just added to -/// // our recipe. -/// connect(ourInstruction.get(), &NamedEntity::changed, &us, &Recipe::acceptChangeToContainedObject); -/// } -/// return; -/// } + impl(Recipe & self, Recipe const & other) : + // Our internal "self" reference, and the equivalents in our owned sets, obviously need to point to the new Recipe + // from whose constructor we are being called. Where relevant, each owned set will also do a deep copy of the + // contents of the corresponding owned set in the Recipe we are copying. + m_self {self}, + m_fermentableAdditions {self, other.pimpl->m_fermentableAdditions}, + m_hopAdditions {self, other.pimpl->m_hopAdditions }, + m_miscAdditions {self, other.pimpl->m_miscAdditions }, + m_yeastAdditions {self, other.pimpl->m_yeastAdditions }, + m_saltAdjustments {self, other.pimpl->m_saltAdjustments }, + m_waterUses {self, other.pimpl->m_waterUses }, + m_brewNotes {self, other.pimpl->m_brewNotes }, + // Other fields are just a simple copy + m_ABV_pct {other.pimpl->m_ABV_pct }, + m_color_srm {other.pimpl->m_color_srm }, + m_boilGrav {other.pimpl->m_boilGrav }, + m_IBU {other.pimpl->m_IBU }, + m_ibus {other.pimpl->m_ibus }, + m_wortFromMash_l {other.pimpl->m_wortFromMash_l }, + m_boilVolume_l {other.pimpl->m_boilVolume_l }, + m_postBoilVolume_l {other.pimpl->m_postBoilVolume_l }, + m_finalVolume_l {other.pimpl->m_finalVolume_l }, + m_finalVolumeNoLosses_l{other.pimpl->m_finalVolumeNoLosses_l}, + m_caloriesPerLiter {other.pimpl->m_caloriesPerLiter }, + m_grainsInMash_kg {other.pimpl->m_grainsInMash_kg }, + m_grains_kg {other.pimpl->m_grains_kg }, + m_SRMColor {other.pimpl->m_SRMColor }, + m_og_fermentable {other.pimpl->m_og_fermentable }, + m_fg_fermentable {other.pimpl->m_fg_fermentable } { + return; + } /** - * \brief If the Recipe is about to be deleted, we delete all the things that belong to it. + * Destructor */ - template void hardDeleteAdditions() { - qDebug() << Q_FUNC_INFO; - for (int id : this->allMyIds()) { - qDebug() << Q_FUNC_INFO << "Hard deleting" << NE::staticMetaObject.className() << "#" << id; - ObjectStoreWrapper::hardDelete(id); - } - } - -/// /** -/// * \brief If the Recipe is about to be deleted, we delete all the things that belong to it. Note that, with the -/// * exception of Instruction, what we are actually deleting here is not the Hops/Fermentables/etc but the "use -/// * of" Hops/Fermentables/etc records (which are distinguished by having a parent ID. -/// */ -/// template void hardDeleteAllMy() { -/// qDebug() << Q_FUNC_INFO; -/// for (auto id : this->accessIds()) { -/// ObjectStoreWrapper::hardDelete(id); -/// } -/// return; -/// } + ~impl() = default; template T getCalculated(T & memberVariable) { @@ -323,69 +255,6 @@ class Recipe::impl { return memberVariable; } - /** - * \brief Get shared pointers to all this Recipe's BrewNotes or RecipeAdditions of a particular type - * (RecipeAdditionHop, RecipeAdditionFermentable, etc). - */ - template - QList> allMy() const { - int const recipeId = this->m_self.key(); - return ObjectStoreWrapper::findAllMatching([recipeId](std::shared_ptr ne) { - return ne->recipeId() == recipeId; - }); - } - - /** - * \brief Get raw pointers to all this Recipe's BrewNotes or RecipeAdditions of a particular type (RecipeAdditionHop, - * RecipeAdditionFermentable, etc). - */ - template - QList allMyRaw() const { - int const recipeId = this->m_self.key(); - return ObjectStoreWrapper::findAllMatching([recipeId](NE const * ne) { - return ne->recipeId() == recipeId; - }); - } - - /** - * \brief Get IDs of all this Recipe's BrewNotes or RecipeAdditions of a particular type (RecipeAdditionHop, - * RecipeAdditionFermentable, etc). - */ - template - QVector allMyIds() const { - int const recipeId = this->m_self.key(); - return ObjectStoreWrapper::idsOfAllMatching([recipeId](NE const * ne) { - return ne->recipeId() == recipeId; - }); - } - - // - // .:TODO:. This will go away once we lose junction tables etc for Fermentables, Miscs and replace them with proper - // RecipeAddition objects. - // - // Inside the class implementation, it's useful to be able to access fermentableIds, hopIds, etc in templated - // functions. This allows us to write this->accessIds() in such a function and have it resolve to - // this->accessIds(), this->accessIds(), etc, which in turn returns this->fermentableIds, - // this->hopIds. - // - // Note that the specialisations need to be defined outside the class - // - template QVector & accessIds(); - - /** - * \brief Get shared pointers to all ingredients etc of a particular type (Hop, Fermentable, etc) in this Recipe - */ - template QList< std::shared_ptr > getAllMy() { - return ObjectStoreTyped::getInstance().getByIds(this->accessIds()); - } - - /** - * \brief Get raw pointers to all ingredients etc of a particular type (Hop, Fermentable, etc) in this Recipe - */ - template QList getAllMyRaw() { - return ObjectStoreTyped::getInstance().getByIdsRaw(this->accessIds()); - } - /** * \brief Called from Recipe::hardDeleteOrphanedEntities */ @@ -423,29 +292,30 @@ class Recipe::impl { connect(equipment.get(), &NamedEntity::changed, &this->m_self, &Recipe::acceptChangeToContainedObject); } - auto fermentableAdditions = this->m_self.fermentableAdditions(); - for (auto fermentableAddition : fermentableAdditions) { - connect(fermentableAddition->fermentable(), - &NamedEntity::changed, - &this->m_self, - &Recipe::acceptChangeToContainedObject); - } - - auto hopAdditions = this->m_self.hopAdditions(); - for (auto hopAddition : hopAdditions) { - connect(hopAddition->hop(), - &NamedEntity::changed, - &this->m_self, - &Recipe::acceptChangeToContainedObject); - } - - auto yeastAdditions = this->m_self.yeastAdditions(); - for (auto yeastAddition : yeastAdditions) { - connect(yeastAddition->yeast(), - &NamedEntity::changed, - &this->m_self, - &Recipe::acceptChangeToContainedObject); - } + // Below should not be handled via OwnedSet connecting to acceptChangeToRecipeAdditionFermentable etc +/// auto fermentableAdditions = this->m_self.fermentableAdditions(); +/// for (auto fermentableAddition : fermentableAdditions) { +/// connect(fermentableAddition->fermentable(), +/// &NamedEntity::changed, +/// &this->m_self, +/// &Recipe::acceptChangeToContainedObject); +/// } +/// +/// auto hopAdditions = this->m_self.hopAdditions(); +/// for (auto hopAddition : hopAdditions) { +/// connect(hopAddition->hop(), +/// &NamedEntity::changed, +/// &this->m_self, +/// &Recipe::acceptChangeToContainedObject); +/// } +/// +/// auto yeastAdditions = this->m_self.yeastAdditions(); +/// for (auto yeastAddition : yeastAdditions) { +/// connect(yeastAddition->yeast(), +/// &NamedEntity::changed, +/// &this->m_self, +/// &Recipe::acceptChangeToContainedObject); +/// } auto mash = this->m_self.mash(); if (mash) { @@ -481,8 +351,8 @@ class Recipe::impl { if (val->key() < 0) { ourId = ObjectStoreWrapper::insert(val); } else { - // TBD: Would be nice to get rid of this call to copyIfNeeded - val = copyIfNeeded(*val); +/// // TBD: Would be nice to get rid of this call to copyIfNeeded +/// val = copyIfNeeded(*val); ourId = val->key(); } @@ -1505,7 +1375,28 @@ class Recipe::impl { //================================================ Member variables ================================================= Recipe & m_self; -/// QVector instructionIds; + + OwnedSet m_fermentableAdditions; + OwnedSet m_hopAdditions ; + OwnedSet m_miscAdditions ; + OwnedSet m_yeastAdditions ; + OwnedSet m_saltAdjustments ; + OwnedSet m_waterUses ; + + /** + * \brief Each \c BrewNote is a record of a brew day. We assume that if you're copying a \c Recipe, it is in order + * to modify it into a new \c Recipe, in which case it does not make sense to bring the original brew notes + * across to the copy. + */ + OwnedSet m_brewNotes ; + + /** + * \brief It's sometimes useful to be able to access the above member variables by type. This templated member + * function allows that. See below for specialisations (which have to be outside the class declaration, + * otherwise the compiler complains about being unable to deduce the return type. + */ + template auto & ownedSetFor(); // Calculated properties. double m_ABV_pct ; @@ -1528,7 +1419,13 @@ class Recipe::impl { }; -///template<> QVector & Recipe::impl::accessIds() { return this->instructionIds; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_fermentableAdditions; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_hopAdditions ; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_miscAdditions ; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_yeastAdditions ; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_saltAdjustments ; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_waterUses ; } +template<> auto & Recipe::impl::ownedSetFor() { return this->m_brewNotes ; } QString Recipe::localisedName() { return tr("Recipe"); } @@ -1740,7 +1637,7 @@ Recipe::Recipe(NamedParameterBundle const & namedParameterBundle) : NamedEntity {namedParameterBundle}, FolderBase {namedParameterBundle}, SteppedOwnerBase{namedParameterBundle}, - pimpl {std::make_unique(*this)}, + pimpl {std::make_unique(*this, namedParameterBundle)}, SET_REGULAR_FROM_NPB (m_type , namedParameterBundle, PropertyNames::Recipe::type ), SET_REGULAR_FROM_NPB (m_brewer , namedParameterBundle, PropertyNames::Recipe::brewer , ""), SET_REGULAR_FROM_NPB (m_asstBrewer , namedParameterBundle, PropertyNames::Recipe::asstBrewer , ""), @@ -1789,8 +1686,10 @@ Recipe::Recipe(NamedParameterBundle const & namedParameterBundle) : Recipe::Recipe(Recipe const & other) : NamedEntity{other}, FolderBase{other}, - SteppedOwnerBase{other}, - pimpl{std::make_unique(*this)}, + SteppedOwnerBase{other}, // This handles copying instructions + // The impl copy constructor calls the OwnedSet copy constructor for each type of recipe addition etc which, in turn + // does a deep copy of the corresonding additions + pimpl{std::make_unique(*this, other)}, m_type {other.m_type }, m_brewer {other.m_brewer }, m_asstBrewer {other.m_asstBrewer }, @@ -1834,33 +1733,12 @@ Recipe::Recipe(Recipe const & other) : NamedEntityModifyingMarker modifyingMarker(*this); // - // When we make a copy of a Recipe, it needs to be a deep(ish) copy. In particular, we need to make copies of the - // Hops, Fermentables etc as some attributes of the recipe (eg how much and when to add) are stored inside these - // ingredients. - // - // We _don't_ want to copy BrewNotes (an instance of brewing the Recipe). (This is easy not to do as we don't - // currently store BrewNote IDs in Recipe.) - // - this->pimpl->copyAdditions(other); - this->pimpl->copyAdditions(other); - this->pimpl->copyAdditions(other); - this->pimpl->copyAdditions(other); - this->pimpl->copyAdditions(other); - this->pimpl->copyAdditions(other); -/// this->pimpl->copyInstructions(*this, other); - - // - // You might think that Style, Mash and Equipment could safely be shared between Recipes. However, AFAICT, none of - // them is. Presumably this is because users expect to be able to edit them in one Recipe without changing the - // settings for any other Recipe. TODO: We should change this so we can retire copyIfNeeded. - // - // We also need to be careful here as one or more of these may not be set to a valid value. + // TBD: For the moment we make a copy of the Mash, Boil, Fermentation of the other recipe. Once we have made changes + // to the UI to make it easier to see how these can be shared amongst different recipes, we will revisit this. // - if (other.m_styleId > 0) { auto item = copyIfNeeded(*ObjectStoreWrapper::getById