diff --git a/src/RecipeExtrasWidget.cpp b/src/RecipeExtrasWidget.cpp index 42018a85d..37eca2e7d 100644 --- a/src/RecipeExtrasWidget.cpp +++ b/src/RecipeExtrasWidget.cpp @@ -1,6 +1,6 @@ /* * RecipeExtrasWidget.cpp is part of Brewtarget, and is Copyright the following - * authors 2009-2023 + * authors 2009-2024 * - Matt Young * - Mik Firestone * - Peter Buelow @@ -117,7 +117,9 @@ void RecipeExtrasWidget::updateTasteRating() { void RecipeExtrasWidget::updatePrimaryAge() { if (!this->recipe) { return;} - MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::primaryAge_days, lineEdit_primaryAge->toCanonical().quantity(), tr("Change Primary Age")); + // See comment in model/Recipe.cpp for why age_days, primaryAge_days, secondaryAge_days, tertiaryAge_days properties + // are dimensionless + MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::primaryAge_days, lineEdit_primaryAge->getNonOptValueAs(), tr("Change Primary Age")); } void RecipeExtrasWidget::updatePrimaryTemp() { @@ -127,7 +129,7 @@ void RecipeExtrasWidget::updatePrimaryTemp() { void RecipeExtrasWidget::updateSecondaryAge() { if (!this->recipe) { return;} - MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::secondaryAge_days, lineEdit_secAge->toCanonical().quantity(), tr("Change Secondary Age")); + MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::secondaryAge_days, lineEdit_secAge->getNonOptValueAs(), tr("Change Secondary Age")); } void RecipeExtrasWidget::updateSecondaryTemp() { @@ -137,7 +139,7 @@ void RecipeExtrasWidget::updateSecondaryTemp() { void RecipeExtrasWidget::updateTertiaryAge() { if (!this->recipe) { return;} - MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::tertiaryAge_days, lineEdit_tertAge->toCanonical().quantity(), tr("Change Tertiary Age")); + MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::tertiaryAge_days, lineEdit_tertAge->getNonOptValueAs(), tr("Change Tertiary Age")); } void RecipeExtrasWidget::updateTertiaryTemp() { @@ -147,7 +149,7 @@ void RecipeExtrasWidget::updateTertiaryTemp() { void RecipeExtrasWidget::updateAge() { if (!this->recipe) { return;} - MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::age_days, lineEdit_age->toCanonical().quantity(), tr("Change Age")); + MainWindow::instance().doOrRedoUpdate(*recipe, PropertyNames::Recipe::age_days, lineEdit_age->getNonOptValueAs(), tr("Change Age")); } void RecipeExtrasWidget::updateAgeTemp() { diff --git a/src/database/ObjectStore.cpp b/src/database/ObjectStore.cpp index be708c8e2..fa5324e99 100644 --- a/src/database/ObjectStore.cpp +++ b/src/database/ObjectStore.cpp @@ -1,6 +1,6 @@ /* * database/ObjectStore.cpp is part of Brewtarget, and is copyright the following - * authors 2021-2023: + * authors 2021-2024: * • Matt Young * * Brewtarget is free software: you can redistribute it and/or modify @@ -695,46 +695,58 @@ class ObjectStore::impl { /// propertyValue.typeName() << "(" << propertyType << ") in column" << fieldDefn.columnName << /// ". This is a known ugliness that we intend to fix one day."; } else { - // It's not a known exception, so it's a coding error. However, we may still be able to recover. - // If we are expecting a boolean and we get a string holding "true" or "false" etc, then we know what to - // do. + // + // It's not a known exception, so it's a coding error. However, we can recover in the following cases: + // - If we are expecting a boolean and we get a string holding "true" or "false" etc, then we know + // what to do. + // - If we are expecting an int and we get a double then we can just ignore the non-integer part of + // the number. + // - If we are expecting an double and we got an int then we can just upcast it. + // bool recovered = false; + QVariant readPropertyValue = propertyValue; if (propertyType == QMetaType::QString && fieldDefn.fieldType == ObjectStore::FieldType::Bool) { // We'll take any reasonable string representation of true/false. For the moment, at least, I'm not // worrying about optional fields here. I think it's pretty rare, if ever, that we'd want an optional // boolean. QString propertyAsLcString = propertyValue.toString().trimmed().toLower(); - bool interpretedValue = false; if (propertyAsLcString == "true" || propertyAsLcString == "t" || propertyAsLcString == "1") { recovered = true; - interpretedValue = true; + propertyValue = QVariant(true); } else if (propertyAsLcString == "false" || propertyAsLcString == "f" || propertyAsLcString == "0") { recovered = true; - interpretedValue = false; - } - if (recovered) { - qWarning() << - Q_FUNC_INFO << "Recovered from unexpected type #" << propertyType << "=" << - propertyValue.typeName() << "in QVariant for property" << fieldDefn.propertyName << - ", field type" << fieldDefn.fieldType << ", value" << propertyValue << ", table" << - primaryTable.tableName << ", column" << fieldDefn.columnName << ". Interpreted value as" << - interpretedValue; - // Now we overwrite the supplied variant with what we worked out it should be, so processing can - // continue. - propertyValue = QVariant(interpretedValue); + propertyValue = QVariant(false); } + } else if (propertyType == QMetaType::Double && fieldDefn.fieldType == ObjectStore::FieldType::Int) { + propertyValue = QVariant(propertyValue.toInt(&recovered)); + } else if (propertyType == QMetaType::Int && fieldDefn.fieldType == ObjectStore::FieldType::Double) { + propertyValue = QVariant(propertyValue.toDouble(&recovered)); } - if (!recovered) { + if (recovered) { + qWarning() << + Q_FUNC_INFO << "Recovered from unexpected type #" << propertyType << "=" << + readPropertyValue.typeName() << "in QVariant for property" << fieldDefn.propertyName << + ", field type" << fieldDefn.fieldType << ", value" << readPropertyValue << ", table" << + primaryTable.tableName << ", column" << fieldDefn.columnName << ". Interpreted value as" << + propertyValue; + } else { + // + // Even in the case where we do not have a reasonable way to interpret the data in this column, we + // should probably NOT terminate the program. We are still discovering lots of cases where folks + // using the software have old Brewtarget databases with quirky values in some columns. It's better + // from a user point of view that the software carries on working even if some (hopefully) obscure + // field could not be read from the DB. + // qCritical() << Q_FUNC_INFO << "Unexpected type #" << propertyType << "=" << propertyValue.typeName() << "in QVariant for property" << fieldDefn.propertyName << ", field type" << fieldDefn.fieldType << ", value" << propertyValue << ", table" << primaryTable.tableName << ", column" << fieldDefn.columnName; - qCritical().noquote() << Q_FUNC_INFO << "Call stack is:" << Logging::getStackTrace(); - // Stop here on debug build - Q_ASSERT(false); + // If, during development or debugging, you want to have the program stop when it cannot interpret + // one of the DB fields, then uncomment the following two lines. +/// qCritical().noquote() << Q_FUNC_INFO << "Call stack is:" << Logging::getStackTrace(); +/// Q_ASSERT(false); } - } } } diff --git a/src/widgets/SmartField.cpp b/src/widgets/SmartField.cpp index ddfcc15c7..c019889b0 100644 --- a/src/widgets/SmartField.cpp +++ b/src/widgets/SmartField.cpp @@ -158,7 +158,8 @@ class SmartField::impl { Measurement::Amount toCanonical(QString const & enteredText, SmartAmounts::ScaleInfo previousScaleInfo) { Q_ASSERT(this->m_initialised); - // It's a coding error to call this for a NonPhysicalQuantity + // It's a coding error to call this for a NonPhysicalQuantity. (Instead call getNonOptValueAs or + // similar.) Q_ASSERT(!std::holds_alternative(*this->m_typeInfo->fieldType)); Q_ASSERT(this->m_currentPhysicalQuantity); @@ -401,6 +402,12 @@ SmartAmounts::ScaleInfo SmartField::getScaleInfo() const { return SmartAmounts::ScaleInfo{this->pimpl->m_fixedDisplayUnit->getUnitSystem().systemOfMeasurement, std::nullopt}; } + // Only need next bit for debugging! +// if (std::holds_alternative(*this->pimpl->m_typeInfo->fieldType)) { +// qCritical() << Q_FUNC_INFO << this->pimpl->m_typeInfo; +// qCritical().noquote() << Q_FUNC_INFO << "Stack trace:" << Logging::getStackTrace(); +// } + Q_ASSERT(!std::holds_alternative(*this->pimpl->m_typeInfo->fieldType)); return SmartAmounts::getScaleInfo(this->pimpl->m_editorName, this->pimpl->m_fieldName, diff --git a/src/widgets/SmartField.h b/src/widgets/SmartField.h index 5537481f3..e87de94a8 100644 --- a/src/widgets/SmartField.h +++ b/src/widgets/SmartField.h @@ -1,5 +1,5 @@ /* - * widgets/SmartField.h is part of Brewtarget, and is copyright the following authors 2009-2023: + * widgets/SmartField.h is part of Brewtarget, and is copyright the following authors 2009-2024: * • Brian Rower * • Mark de Wever * • Matt Young @@ -62,7 +62,7 @@ class TypeInfo; * / SmartField \ * QLabel / \ \ * / \ / \ \ - * SmartLabel SmartDigitWidget SmartField + * SmartLabel SmartDigitWidget SmartLineEdit * * A number of helper functions exist in the \c SmartAmounts namespace. *