More initialisation order fixes, plus filename fix for Windows

src/database/Database.cpp

@@ -36,6 +36,7 @@
  =====================================================================================================================*/
 #include "database/Database.h"
 
+#include <filesystem>
 #include <iostream> // For writing to std::cerr in destructor
 #include <mutex>    // For std::once_flag etc
 
@@ -62,6 +63,7 @@
 #include "PersistentSettings.h"
 #include "utils/BtStringConst.h"
 #include "utils/EnumStringMapping.h"
+#include "utils/ErrorCodeToStream.h"
 
 namespace {
    EnumStringMapping const dbTypeToName {
@@ -414,13 +416,16 @@ class Database::impl {
          ).toString();
          //
          // It's probably enough for most users to put the date on the backup file name to make it unique.  But we put
-         // the time too just in case.
+         // the time too just in case.  Note that we cannot format the time as hh:mm:ss because Windows does not accept
+         // colons in filenames.  We can however use the ratio symbol (∶) which looks almost the same.  There is a
+         // precendent for doing this:
+         // https://web.archive.org/web/20190108033419/https://blogs.msdn.microsoft.com/oldnewthing/20180913-00/?p=99725
          //
          // NOTE: We do not currently check whether the file we are creating already exists...
          //
          QString backupName = QString(
             "%1 database.sqlite backup (before upgrade from v%2 to v%3)"
-         ).arg(QDateTime::currentDateTime().toString("yyyy-MM-dd hh:mm:ss")).arg(currentVersion).arg(newVersion);
+         ).arg(QDateTime::currentDateTime().toString("yyyy-MM-dd hh∶mm∶ss")).arg(currentVersion).arg(newVersion);
          bool succeeded = database.backupToDir(backupDir, backupName);
          if (!succeeded) {
             qCritical() << Q_FUNC_INFO << "Unable to create DB backup";
@@ -939,16 +944,60 @@ char const * Database::getDefaultBackupFileName() {
     return "database.sqlite";
 }
 
-bool Database::backupToFile(QString newDbFileName) {
-   // Remove the files if they already exist so that
-   // the copy() operation will succeed.
-   QFile::remove(newDbFileName);
+bool Database::backupToFile(QString const & newDbFileName) {
+   QString const curDbFileName = this->pimpl->dbFile.fileName();
 
-   bool success = this->pimpl->dbFile.copy(newDbFileName);
+   qDebug() << Q_FUNC_INFO << "Database backup from" << curDbFileName << "to" << newDbFileName;
 
-   qDebug() << QString("Database backup to \"%1\" %2").arg(newDbFileName, success ? "succeeded" : "failed");
+   //
+   // In earlier versions of the code, we just used the copy() member function of QFile.  When this works it is fine,
+   // but when there is an error, the diagnostics are not always very helpful.  Eg getting QFileDevice::CopyError back
+   // from the error() member function doesn't really tell us why the file could not be copied.
+   //
+   // Using the Filesystem library from the C++ standard library gives us better (albeit not perfect) diagnostics when
+   // things go wrong.
+   //
+   // The std::filesystem functions typically come in two versions: one using an error code and one throwing an
+   // exception.  Since we're going to abort on the first error, the exception-throwing versions make the code a bit
+   // simpler.  The `operation` variable keeps track of what we were doing when the exception occurred so we can give a
+   // clue about what caused the error.
+   //
+   this->pimpl->dbFile.close();
+   char const * operation = "Allocate";
+   try {
+      std::filesystem::path source{curDbFileName.toStdString()};
+      std::filesystem::path target{newDbFileName.toStdString()};
+      // Note that std::filesystem::canonical needs its parameter to exist, whereas std::filesystem::weakly_canonical
+      // does not.
+      source = std::filesystem::canonical(source);
+      target = std::filesystem::weakly_canonical(target);
+      operation = "See if target already exists";
+      if (std::filesystem::exists(target)) {
+         // AFAICT std::filesystem::copy should overwrite its target if it exists, but it's helpful for diagnostics to
+         // pull that case out as a separate step.
+         operation = "Remove existing target";
+         qInfo() <<
+            Q_FUNC_INFO << "Removing existing file" << newDbFileName << "before copying" << curDbFileName;
+         std::filesystem::remove(target);
+      }
+      operation = "Copy";
+      std::filesystem::copy(source, target);
+   } catch (std::filesystem::filesystem_error & fsError) {
+      std::error_code errorCode = fsError.code();
+      qWarning() <<
+         Q_FUNC_INFO << "Error backing up database file " << curDbFileName << "to" << newDbFileName << ":" <<
+         operation << "failed with" << errorCode << ".  Error message:" << fsError.what();
+      return false;
+   } catch (std::exception & exception) {
+      // Most probably this would be std::bad_alloc, in which case we'd probably even have difficulty logging, but we
+      // might as well try!
+      qCritical() <<
+         Q_FUNC_INFO << "Unexpected error backing up database file " << curDbFileName << "to" << newDbFileName << ":" <<
+         operation << "failed:" << exception.what();
+      return false;
+   }
 
-   return success;
+   return true;
 }
 
 bool Database::backupToDir(QString dir, QString filename) {

src/database/Database.h

@@ -1,5 +1,5 @@
 /*======================================================================================================================
- * database/Database.h is part of Brewken, and is copyright the following authors 2009-2022:
+ * database/Database.h is part of Brewken, and is copyright the following authors 2009-2024:
  *   • Aidan Roberts <[email protected]>
  *   • A.J. Drobnich <[email protected]>
  *   • Brian Rower <[email protected]>
@@ -120,7 +120,7 @@ class Database {
    static char const * getDefaultBackupFileName();
 
    //! backs up database to chosen file
-   bool backupToFile(QString newDbFileName);
+   bool backupToFile(QString const & newDbFileName);
 
    //! backs up database to 'dir' in chosen directory
    bool backupToDir(QString dir, QString filename="");

src/model/Recipe.cpp

@@ -68,8 +68,6 @@
 #include "PhysicalConstants.h"
 
 namespace {
-   double const us12ozInLiters = Measurement::Units::us_fluidOunces.toCanonical(12.0).quantity;
-   double const usPintInLiters = Measurement::Units::us_fluidOunces.toCanonical(16.0).quantity;
 
    /**
     * \brief This is used to assist the creation of instructions.
@@ -2788,8 +2786,14 @@ QList<double> Recipe::IBUs            () { return this->pimpl->getCalculated(thi
 double        Recipe::boilGrav        () { return this->pimpl->getCalculated(this->pimpl->m_boilGrav        ); }
 double        Recipe::caloriesPerLiter() { return this->pimpl->getCalculated(this->pimpl->m_caloriesPerLiter); }
 double        Recipe::caloriesPer33cl  () { return this->caloriesPerLiter() * 0.33          ; }
-double        Recipe::caloriesPerUs12oz() { return this->caloriesPerLiter() * us12ozInLiters; }
-double        Recipe::caloriesPerUsPint() { return this->caloriesPerLiter() * usPintInLiters; }
+double        Recipe::caloriesPerUs12oz() {
+   static double const us12ozInLiters = Measurement::Units::us_fluidOunces.toCanonical(12.0).quantity;
+   return this->caloriesPerLiter() * us12ozInLiters;
+}
+double        Recipe::caloriesPerUsPint() {
+   static double const usPintInLiters = Measurement::Units::us_fluidOunces.toCanonical(16.0).quantity;
+   return this->caloriesPerLiter() * usPintInLiters;
+}
 double        Recipe::wortFromMash_l  () { return this->pimpl->getCalculated(this->pimpl->m_wortFromMash_l  );}
 double        Recipe::boilVolume_l    () { return this->pimpl->getCalculated(this->pimpl->m_boilVolume_l    );}
 double        Recipe::postBoilVolume_l() { return this->pimpl->getCalculated(this->pimpl->m_postBoilVolume_l);}