Skip to content

Commit

Permalink
Merge pull request #869 from matty0ung/usability
Browse files Browse the repository at this point in the history
Better way of doing owned sets in Recipe, which also fixes Recipe copy problems
  • Loading branch information
matty0ung authored Nov 3, 2024
2 parents 44bdd03 + 777ed63 commit d64d620
Show file tree
Hide file tree
Showing 9 changed files with 870 additions and 522 deletions.
3 changes: 2 additions & 1 deletion CHANGES.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 6 additions & 3 deletions src/model/NamedEntity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
4 changes: 4 additions & 0 deletions src/model/OwnedByRecipe.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ class OwnedByRecipe : public NamedEntity {
int recipeId() const;
std::shared_ptr<Recipe> 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;

Expand Down
607 changes: 607 additions & 0 deletions src/model/OwnedSet.h

Large diffs are not rendered by default.

638 changes: 180 additions & 458 deletions src/model/Recipe.cpp

Large diffs are not rendered by default.

61 changes: 18 additions & 43 deletions src/model/Recipe.h
Original file line number Diff line number Diff line change
Expand Up @@ -415,38 +415,6 @@ class Recipe : public NamedEntity,
*/
static void connectSignalsForAllRecipes();

/// /*!
/// * \brief Add (a copy if necessary of) a Hop/Fermentable/Instruction etc (that may or may not already be in an
/// * ObjectStore).
/// *
/// * TODO: Need to finish killing off this member function!
/// *
/// * When we add a Hop/Fermentable/Yeast/etc to a Recipe, we make a copy of thing we're adding to serve as an "instance
/// * of use of" record. Amongst other things, this allows the same Hop/Fermentable/Yeast/etc to be added multiple
/// * times to a recipe - eg the same type of hops might well be added at multiple points in the recipe. It also allows
/// * an ingredient in a recipe to be modified without those modifications affecting the use of the ingredient in other
/// * recipes (eg if you want to modify the % alpha acid on a hop). An "instance of use of" instance of a
/// * Hop/Fermentable/Yeast/etc will always have parent record which is the actual Hop/Fermentable/Yeast/etc to which it
/// * relates.
/// *
/// * Calling the templated \c Recipe::add function returns the copy "instance of use of" object for whatever
/// * Hop/Fermentable/Yeast/etc was added. This returned object is what needs to be passed to \c Recipe::remove to
/// * remove that instance of use of the Hop/Fermentable/Yeast/etc from the Recipe. When you call \c Recipe::remove it
/// * returns the "instance of use of" object that was removed, which you as caller now own (because it will no longer
/// * be in the ObjectStore). If you want to undo the remove (or redo an add that the remove itself was undoing), you
/// * can call \c Recipe::add with the "instance of use of" object returned from \c Recipe::remove, in which case
/// * \c Recipe::add will determine that the "instance of use of" object can be used directly without needing to be
/// * copied. (The \c Recipe::add method will also recognise when an object has been removed from the ObjectStore and
/// * will reinsert it, so the caller doesn't need to worry about this.) Thus, eg, the following sequence of calls is
/// * valid:
/// *
/// * std::shared_ptr<Hop> copyOfFooHop = myRecipe->add<Hop>(fooHop); // DO
/// * std::shared_ptr<Hop> sameCopyOfFooHop = myRecipe->remove<Hop>(*copyOfFooHop); // UNDO
/// * std::shared_ptr<Hop> stillSameCopyOfFooHop = myRecipe->add<Hop>(*sameCopyOfFooHop); // REDO
/// *
/// */
/// template<class NE> std::shared_ptr<NE> add(std::shared_ptr<NE> var);

/**
* \brief Use this for adding \c RecipeAdditionHop, etc.
*/
Expand Down Expand Up @@ -479,15 +447,6 @@ class Recipe : public NamedEntity,
return ObjectStoreWrapper::findFirstMatching<Recipe>( [var](Recipe * rec) {return rec->uses(var);} );
}

/// int instructionNumber(Instruction const & ins) const;
/// /*!
/// * \brief Swap instructions \c ins1 and \c ins2
/// */
/// void swapInstructions(Instruction & ins1, Instruction & ins2);
/// //! \brief Remove all instructions.
/// void clearInstructions();
/// //! \brief Insert instruction ins into slot pos.
/// void insertInstruction(Instruction const & ins, int pos);
//! \brief Automagically generate a list of instructions.
void generateInstructions();
/*!
Expand Down Expand Up @@ -716,17 +675,33 @@ class Recipe : public NamedEntity,
*/
virtual void hardDeleteOrphanedEntities() override;

/**
* \brief This does all the work for the acceptChangeToXxxxx functions below (which can't be templated because the Qt
* MOC does not allow it).
*/
template<class Owned> void acceptChange(QMetaProperty prop, QVariant val);

signals:
//! Emitted when the number of instructions changes, or when you should call steps() again.
void stepsChanged();

public slots:
void acceptChangeToContainedObject(QMetaProperty prop, QVariant val);
// It would be neat if we could template these slots, but the Qt MOC does not allow it, and will give "error:
// Template function as signal or slot". These slot functions just call the obvious
void acceptChangeToRecipeAdditionFermentable(QMetaProperty prop, QVariant val);
void acceptChangeToRecipeAdditionHop (QMetaProperty prop, QVariant val);
void acceptChangeToRecipeAdditionMisc (QMetaProperty prop, QVariant val);
void acceptChangeToRecipeAdditionYeast (QMetaProperty prop, QVariant val);
void acceptChangeToRecipeAdjustmentSalt (QMetaProperty prop, QVariant val);
void acceptChangeToRecipeUseOfWater (QMetaProperty prop, QVariant val);
void acceptChangeToBrewNote (QMetaProperty prop, QVariant val);

void acceptStepChange(QMetaProperty, QVariant);

protected:
virtual bool isEqualTo(NamedEntity const & other) const;
virtual ObjectStore & getObjectStoreTypedInstance() const;
virtual bool isEqualTo(NamedEntity const & other) const override;
virtual ObjectStore & getObjectStoreTypedInstance() const override;

private:
// Private implementation details - see https://herbsutter.com/gotw/_100/
Expand Down
14 changes: 13 additions & 1 deletion src/model/StepOwnerBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,23 @@ class StepOwnerBase : public SteppedOwnerBase<Derived, DerivedStep> {
return;
}

StepOwnerBase(StepOwnerBase const & other) :
StepOwnerBase(Derived const & other) :
SteppedOwnerBase<Derived, DerivedStep>{other} {
return;
}

/**
* \brief We have to delete the default copy constructor because we want the constructor above (that takes \c Derived
* rather than \c SteppedOwnerBase) to be used instead of a compiler-generated copy constructor which wouldn't
* do the deep copy we need.
*/
StepOwnerBase(StepOwnerBase const & other) = delete;

/**
* \brief Similarly, we don't want copy assignment happening.
*/
StepOwnerBase & operator=(StepOwnerBase const & other) = delete;

~StepOwnerBase() = default;

};
Expand Down
19 changes: 16 additions & 3 deletions src/model/SteppedBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ AddPropertyName(stepNumber)
* NOTE that the "stepped" concept does not apply to other things "owned" by Recipe, such as \c RecipeAdditionHop
* or \c BrewNote, because they do not have the same need of pure ordering. There is an implicit ordering by
* addition time or date, but it is not necessary or desirable to be more precise than this. You will often have
* two hop additions that happen at the same time, for instance.
* two hop additions that happen at the same time, for instance. See \c OwnedSet for the logic for dealing with
* these.
*
* Directly derived classes need to include STEPPED_COMMON_DECL and STEPPED_COMMON_CODE in the obvious places
* (either their own macros for \c StepBase or in the header and implementation file respectively for
Expand Down Expand Up @@ -112,8 +113,20 @@ class SteppedBase : public CuriouslyRecurringTemplateBase<SteppedPhantom, Derive
this->derived().propagatePropertyChange(PropertyNames::SteppedBase::ownerId, false);
return;
}
void setStepNumber(int const val) {
this->derived().setAndNotify(PropertyNames::SteppedBase::stepNumber, this->m_stepNumber, val);

/**
* \brief Does what it says on the tin
*
* \param notify Needs to be set to \c false when part-way through swapping two steps, because we don't want
* observers to re-read all the steps until we've finished.
*/
void setStepNumber(int const val, bool const notify = true) {
if (notify) {
this->derived().setAndNotify(PropertyNames::SteppedBase::stepNumber, this->m_stepNumber, val);
} else {
this->m_stepNumber = val;
this->derived().propagatePropertyChange(PropertyNames::SteppedBase::stepNumber, false);
}
return;
}

Expand Down
37 changes: 24 additions & 13 deletions src/model/SteppedOwnerBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ AddPropertyName(steps )
* etc.
*
* Concrete classes deriving from this one need to declare a `void stepsChanged()` Qt signal in their header.
*
* TODO: We could probably find a way to share more code between this class and \c OwnedSet. Plus, I also now
* think we'd be better using \c OwnedSet for Recipe's ownership of \c Instructions, so we can merge this
* back into \c StepOwnerBase etc.
*/
template<class Derived> class SteppedOwnerPhantom;
template<class Derived, class DerivedStep>
Expand Down Expand Up @@ -96,6 +100,18 @@ class SteppedOwnerBase : public CuriouslyRecurringTemplateBase<SteppedOwnerPhant
return;
}

/**
* \brief We have to delete the default copy constructor because we want the constructor above (that takes \c Derived
* rather than \c SteppedOwnerBase) to be used instead of a compiler-generated copy constructor which wouldn't
* do the deep copy we need.
*/
SteppedOwnerBase(SteppedOwnerBase const & other) = delete;

/**
* \brief Similarly, we don't want copy assignment happening.
*/
SteppedOwnerBase & operator=(SteppedOwnerBase const & other) = delete;

~SteppedOwnerBase() = default;

public:
Expand Down Expand Up @@ -272,25 +288,20 @@ class SteppedOwnerBase : public CuriouslyRecurringTemplateBase<SteppedOwnerPhant
Q_FUNC_INFO << "Swapping steps" << step1.stepNumber() << "(#" << step1.key() << ") and " <<
step2.stepNumber() << " (#" << step2.key() << ")";

// Make sure we don't send notifications until the end (hence the false parameter on setStepNumber).
int temp = step1.stepNumber();
step1.setStepNumber(step2.stepNumber());
step2.setStepNumber(temp);
step1.setStepNumber(step2.stepNumber(), false);
step2.setStepNumber(temp, false);

int indexOf1 = this->m_stepIds.indexOf(step1.key());
int indexOf2 = this->m_stepIds.indexOf(step2.key());

// We can't swap them if we can't find both of them
// There's no point swapping them if they're the same
if (-1 == indexOf1 || -1 == indexOf2 || indexOf1 == indexOf2) {
return;
if (-1 != indexOf1 && -1 != indexOf2 && indexOf1 != indexOf2) {
this->m_stepIds.swapItemsAt(indexOf1, indexOf2);
}

// As of Qt 5.14 we could write:
// this->m_stepIds.swapItemsAt(indexOf1, indexOf2);
// However, we still need to support slightly older versions of Qt (5.12 in particular), hence the more cumbersome
// way here.
std::swap(this->m_stepIds[indexOf1], this->m_stepIds[indexOf2]);

emit this->derived().stepsChanged();
return;
}
Expand Down Expand Up @@ -420,11 +431,11 @@ class SteppedOwnerBase : public CuriouslyRecurringTemplateBase<SteppedOwnerPhant
/**
* \brief Intended to be called from \c Derived::acceptStepChange
*
* \param sender - Result of caller calling \c this->sender() (which is protected, so we can't call it here
* \param sender - Result of caller calling \c this->sender() (which is protected, so we can't call it here)
* \param prop - As received by Derived::acceptStepChange
* \param val - As received by Derived::acceptStepChange
* \param additionalProperties - Additional properties for which to emit \c changed signal if the change we are
* receiving comes from one of our steps
* receiving comes from one of our steps. TODO: Should move this to a template parameter
*/
void doAcceptStepChange(QObject * sender,
[[maybe_unused]] QMetaProperty prop,
Expand All @@ -437,7 +448,7 @@ class SteppedOwnerBase : public CuriouslyRecurringTemplateBase<SteppedOwnerPhant

// If one of our steps changed, our pseudo properties may also change, so we need to emit some signals
if (stepSender->ownerId() == this->derived().key()) {
emit this->derived().changed(this->derived().metaProperty(*PropertyNames::SteppedOwnerBase::numSteps), QVariant());
/// emit this->derived().changed(this->derived().metaProperty(*PropertyNames::SteppedOwnerBase::numSteps), QVariant());
emit this->derived().changed(this->derived().metaProperty(*PropertyNames::SteppedOwnerBase::steps ), QVariant());
for (auto property : additionalProperties) {
emit this->derived().changed(this->derived().metaProperty(**property), QVariant());
Expand Down

0 comments on commit d64d620

Please sign in to comment.