Merge pull request #876 from matty0ung/usability

Error-handling fixes
Brewtarget · Nov 15, 2024 · bf4b637 · bf4b637
2 parents a1d0870 + d423c23
commit bf4b637
Show file tree

Hide file tree

Showing 7 changed files with 130 additions and 86 deletions.
diff --git a/CHANGES.markdown b/CHANGES.markdown
@@ -17,15 +17,15 @@ happens, so I'm now setting it to a slightly arbitrary time early in the morning
 Bug fixes and minor enhancements.
 
 ### New Features
-* Danish translation
+* Danish translation, courtesy of Orla Valbjørn Møller
 
 ### Bug Fixes
 * Crash when copying recipe, then on adding new steps in mash, ferment, boil, others [868](https://github.com/Brewtarget/brewtarget/issues/868)
 * Changing Yeast or Annetuation does't change ABV [872](https://github.com/Brewtarget/brewtarget/issues/872)
 * Brewtarget 4.0.X doesn't work on macOS [809](https://github.com/Brewtarget/brewtarget/issues/809)
 
 ### Release Timestamp
-Sun, 3 Nov 2024 04:00:10 +0100
+Fri, 15 Nov 2024 04:00:10 +0100
 
 ## v4.0.9
 Bug fixes and minor enhancements.

diff --git a/src/Application.cpp b/src/Application.cpp
@@ -448,7 +448,6 @@ int Application::run() {
       Application::cleanup();
       return 1;
    }
-   Database::instance().checkForNewDefaultData();
 
    //
    // Make sure the MainWindow singleton exists, but don't initialise it just yet.  We're going to use the end of the

diff --git a/src/LatestReleaseFinder.cpp b/src/LatestReleaseFinder.cpp
@@ -63,8 +63,8 @@ void LatestReleaseFinder::checkMainRespository() {
    // well use the newer standard.
    //
    boost::beast::http::request<boost::beast::http::string_body> httpRequest{boost::beast::http::verb::get,
-                                                                              path,
-                                                                              30};
+                                                                            path,
+                                                                            30};
    httpRequest.set(boost::beast::http::field::host, host);
    //
    // GitHub will respond with an error if the user agent field is not present, but it doesn't care what it's set to
@@ -78,73 +78,84 @@ void LatestReleaseFinder::checkMainRespository() {
       Q_FUNC_INFO << "Sending request to " << QString::fromStdString(requestAsString.str()) <<
       "to check for latest release";
 
-   boost::asio::io_service ioService;
-   //
-   // A lot of old example code for Boost still uses sslv23_client.  However, TLS 1.3 has been out since 2018, and we
-   // know GitHub (along with most other web sites) supports it.  So there's no reason not to use that.
-   //
-   boost::asio::ssl::context securityContext(boost::asio::ssl::context::tlsv13_client);
-   boost::asio::ssl::stream<boost::asio::ip::tcp::socket> secureSocket{ioService, securityContext};
-   // The resolver essentially does the DNS requests to look up the host address etc
-   boost::asio::ip::tcp::resolver tcpIpResolver{ioService};
-   auto endpoint = tcpIpResolver.resolve(host, port);
-
-   // Once we have the address, we can connect, do the SSL handshake, and then send the request
-   boost::asio::connect(secureSocket.lowest_layer(), endpoint);
-   secureSocket.handshake(boost::asio::ssl::stream_base::handshake_type::client);
-   boost::beast::http::write(secureSocket, httpRequest);
-
-   // Now wait for the response
-   boost::beast::http::response<boost::beast::http::string_body> httpResponse;
-   boost::beast::flat_buffer buffer;
-   boost::beast::http::read(secureSocket, buffer, httpResponse);
-
-   if (httpResponse.result() != boost::beast::http::status::ok) {
+   try {
+      boost::asio::io_service ioService;
       //
-      // It's not the end of the world if we couldn't check for an update, but we should record the fact.  With some
-      // things in Boost.Beast, the easiest way to convert them to a string is via a standard library output stream,
-      // so we construct the whole error message like that rather then try to mix-and-match with Qt logging output
-      // streams.
+      // A lot of old example code for Boost still uses sslv23_client.  However, TLS 1.3 has been out since 2018, and we
+      // know GitHub (along with most other web sites) supports it.  So there's no reason not to use that.
       //
-      std::ostringstream errorMessage;
-      errorMessage <<
-         "Error checking for update: " << httpResponse.result_int() << ".\nResponse headers:" << httpResponse.base();
-      qInfo().noquote() << Q_FUNC_INFO << QString::fromStdString(errorMessage.str());
-      return;
-   }
+      boost::asio::ssl::context securityContext(boost::asio::ssl::context::tlsv13_client);
+      boost::asio::ssl::stream<boost::asio::ip::tcp::socket> secureSocket{ioService, securityContext};
+      // The resolver essentially does the DNS requests to look up the host address etc
+      boost::asio::ip::tcp::resolver tcpIpResolver{ioService};
+      auto endpoint = tcpIpResolver.resolve(host, port);
+
+      // Once we have the address, we can connect, do the SSL handshake, and then send the request
+      boost::asio::connect(secureSocket.lowest_layer(), endpoint);
+      secureSocket.handshake(boost::asio::ssl::stream_base::handshake_type::client);
+      boost::beast::http::write(secureSocket, httpRequest);
+
+      // Now wait for the response
+      boost::beast::http::response<boost::beast::http::string_body> httpResponse;
+      boost::beast::flat_buffer buffer;
+      boost::beast::http::read(secureSocket, buffer, httpResponse);
+
+      if (httpResponse.result() != boost::beast::http::status::ok) {
+         //
+         // It's not the end of the world if we couldn't check for an update, but we should record the fact.  With some
+         // things in Boost.Beast, the easiest way to convert them to a string is via a standard library output stream,
+         // so we construct the whole error message like that rather then try to mix-and-match with Qt logging output
+         // streams.
+         //
+         std::ostringstream errorMessage;
+         errorMessage <<
+            "Error checking for update: " << httpResponse.result_int() << ".\nResponse headers:" << httpResponse.base();
+         qInfo().noquote() << Q_FUNC_INFO << QString::fromStdString(errorMessage.str());
+         return;
+      }
 
-   //
-   // Checking a version number on Sourceforge is easy, eg a GET request to
-   // https://brewtarget.sourceforge.net/version just returns the last version of Brewtarget that was hosted on
-   // Sourceforge (quite an old one).
-   //
-   // On GitHub, it's a bit harder as there's a REST API that gives back loads of info in JSON format.  We don't want
-   // to do anything clever with the JSON response, just extract one field, so the Qt JSON support suffices here.
-   // (See comments elsewhere for why we don't use it for BeerJSON.)
-   //
-   QByteArray rawContent = QByteArray::fromStdString(httpResponse.body());
-   QJsonParseError jsonParseError{};
-
-   QJsonDocument jsonDocument = QJsonDocument::fromJson(rawContent, &jsonParseError);
-   if (QJsonParseError::ParseError::NoError != jsonParseError.error) {
+      //
+      // Checking a version number on Sourceforge is easy, eg a GET request to
+      // https://brewtarget.sourceforge.net/version just returns the last version of Brewtarget that was hosted on
+      // Sourceforge (quite an old one).
+      //
+      // On GitHub, it's a bit harder as there's a REST API that gives back loads of info in JSON format.  We don't want
+      // to do anything clever with the JSON response, just extract one field, so the Qt JSON support suffices here.
+      // (See comments elsewhere for why we don't use it for BeerJSON.)
+      //
+      QByteArray rawContent = QByteArray::fromStdString(httpResponse.body());
+      QJsonParseError jsonParseError{};
+
+      QJsonDocument jsonDocument = QJsonDocument::fromJson(rawContent, &jsonParseError);
+      if (QJsonParseError::ParseError::NoError != jsonParseError.error) {
+         qWarning() <<
+            Q_FUNC_INFO << "Error parsing JSON from version check response:" << jsonParseError.error << "at offset" <<
+            jsonParseError.offset;
+         return;
+
+      }
+
+      QJsonObject jsonObject = jsonDocument.object();
+
+      QString remoteVersion = jsonObject.value("tag_name").toString();
+      // Version names are usually "v3.0.2" etc, so we want to strip the 'v' off the front
+      if (remoteVersion.startsWith("v", Qt::CaseInsensitive)) {
+         remoteVersion.remove(0, 1);
+      }
+      QVersionNumber const latestRelease {QVersionNumber::fromString(remoteVersion)};
+      qInfo() <<
+         Q_FUNC_INFO << "Latest release is" << remoteVersion << "(parsed as" << latestRelease << ")";
+      emit foundLatestRelease(latestRelease);
+
+   } catch (std::exception & e) {
+      //
+      // Typically we might get an exception if there are network problems.  It's not fatal.  We'll do the check again
+      // next time the program is run.
+      //
       qWarning() <<
-         Q_FUNC_INFO << "Error parsing JSON from version check response:" << jsonParseError.error << "at offset" <<
-         jsonParseError.offset;
-      return;
-
+         Q_FUNC_INFO << "Problem while trying to contact GitHub to check for newer version of the software:" <<
+         e.what();
    }
 
-   QJsonObject jsonObject = jsonDocument.object();
-
-   QString remoteVersion = jsonObject.value("tag_name").toString();
-   // Version names are usually "v3.0.2" etc, so we want to strip the 'v' off the front
-   if (remoteVersion.startsWith("v", Qt::CaseInsensitive)) {
-      remoteVersion.remove(0, 1);
-   }
-   QVersionNumber const latestRelease {QVersionNumber::fromString(remoteVersion)};
-   qInfo() <<
-      Q_FUNC_INFO << "Latest release is" << remoteVersion << "(parsed as" << latestRelease << ")";
-   emit foundLatestRelease(latestRelease);
-
    return;
 }
diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
@@ -1087,11 +1087,6 @@ void MainWindow::initialiseAndMakeVisible() {
    // set up the drag/drop parts
    this->setupDrops();
 
-   // This sets up things that might have been 'remembered' (ie stored in the config file) from a previous run of the
-   // program - eg window size, which is stored in MainWindow::closeEvent().
-   // Breaks the naming convention, doesn't it?
-   this->restoreSavedState();
-
    // Moved from Database class
    Recipe::connectSignalsForAllRecipes();
    qDebug() << Q_FUNC_INFO << "Recipe signals connected";
@@ -1112,9 +1107,23 @@ void MainWindow::initialiseAndMakeVisible() {
    // .:TODO:. Change this so we use the newer deleted signal!
    connect(&ObjectStoreTyped<BrewNote>::getInstance(), &ObjectStoreTyped<BrewNote>::signalObjectDeleted, this, &MainWindow::closeBrewNote);
 
+   //
+   // Read in any new ingredients, styles, example recipes etc
+   //
+   // (In the past this was done in Application::run() because we were reading raw DB files.  Now that default
+   // ingredients etc are stored in BeerXML and BeerJSON, we need to read them in a bit later, after the MainWindow
+   // object exists (ie after its constructor finishes running!) and after the call InitialiseAllObjectStores.)
+   //
+   Database::instance().checkForNewDefaultData();
+
+   // This sets up things that might have been 'remembered' (ie stored in the config file) from a previous run of the
+   // program - eg window size, which is stored in MainWindow::closeEvent().
+   // Breaks the naming convention, doesn't it?
+   this->restoreSavedState();
+
    // Set up the pretty tool tip. It doesn't really belong anywhere, so here it is
    // .:TODO:. When we allow users to change databases without restarting, we'll need to make sure to call this whenever
-   // the databae is changed (as setToolTip() just takes static text as its parameter).
+   // the database is changed (as setToolTip() just takes static text as its parameter).
    label_Brewtarget->setToolTip(getLabelToolTip());
 
    this->setVisible(true);
@@ -1287,15 +1296,18 @@ void MainWindow::restoreSavedState() {
          key = firstRecipeWeFind->key();
       }
    }
-   if ( key > -1 ) {
-      this->pimpl->m_recipeObs = ObjectStoreWrapper::getByIdRaw<Recipe>(key);
-      QModelIndex rIdx = treeView_recipe->findElement(this->pimpl->m_recipeObs);
-
-      setRecipe(this->pimpl->m_recipeObs);
-      setTreeSelection(rIdx);
+   if (key > -1) {
+      // We can't assume that the "remembered" recipe exists.  The user might have restored to an older DB file since
+      // the last time the program was run.
+      Recipe * recipe = ObjectStoreWrapper::getByIdRaw<Recipe>(key);
+      qDebug() << Q_FUNC_INFO << "Recipe #" << key << (recipe ? "found" : "not found");
+      if (recipe) {
+         // We trust setRecipe to do any necessary checks and UI updates
+         this->setRecipe(recipe);
+      }
    }
 
-   //UI restore state
+   // UI restore state
    if (PersistentSettings::contains(PersistentSettings::Names::splitter_vertical_State,
                                     PersistentSettings::Sections::MainWindow)) {
       splitter_vertical->restoreState(PersistentSettings::value(PersistentSettings::Names::splitter_vertical_State,
@@ -1786,6 +1798,11 @@ void MainWindow::setRecipe(Recipe* recipe) {
    if (boil) {
       connect(boil.get(), &NamedEntity::changed, this, &MainWindow::changed);
    }
+
+   // TBD: Had some problems with this that we should come back to once rework of TreeView etc is complete
+//   QModelIndex rIdx = treeView_recipe->findElement(this->pimpl->m_recipeObs);
+//   setTreeSelection(rIdx);
+
    this->showChanges();
    return;
 }

diff --git a/src/database/Database.h b/src/database/Database.h
@@ -71,7 +71,8 @@ class Database {
    static Database& instance(Database::DbType dbType = Database::DbType::NODB);
 
    /**
-    * \brief Check for new default ingredients etc
+    * \brief Check for new default ingredients etc.  NB: This should be called \b after calling
+    *        \c InitialiseAllObjectStores, and after the \c MainWindow is constructed.
     */
    void checkForNewDefaultData();
 

diff --git a/src/main.cpp b/src/main.cpp
@@ -20,6 +20,8 @@
  ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌*/
 #include <boost/json/src.hpp> // Needs to be included exactly once in the code to use header-only version of Boost.JSON
 
+#include <iostream>
+
 #include <xercesc/util/PlatformUtils.hpp>
 #include <xalanc/Include/PlatformDefinitions.hpp>
 
@@ -80,8 +82,10 @@ namespace {
          try {
             return QApplication::notify(receiver, event);
          } catch (std::exception & e) {
-            qCritical() << Q_FUNC_INFO << "Uncaught exception: " << e.what();
-            qCritical().noquote() << Q_FUNC_INFO << "Stacktrace:" << Logging::getStackTrace();
+            // Obviously "uncaught" here means "uncaught elsewhere in the code"
+            std::cerr << "Uncaught exception: " << e.what() << std::endl;
+            qCritical().noquote() <<
+               Q_FUNC_INFO << "Uncaught exception: " << e.what() << "\nStacktrace:" << Logging::getStackTrace();
 
             // If we wanted the application to keep running here, we could just drop through to `return false`.  But we
             // don't know whether continuing is a good idea.  So the safest thing is to exit now that we've logged some

diff --git a/src/model/Recipe.cpp b/src/model/Recipe.cpp
@@ -2225,11 +2225,23 @@ QList<Recipe *> Recipe::ancestors() const {
    if (this->m_ancestor_id > 0 && this->m_ancestor_id != this->key() && this->m_ancestors.size() == 0) {
       // NB: In previous versions of the code, we included the Recipe in the list along with its ancestors, but it's
       //     now just the ancestors in the list.
-      Recipe * ancestor = const_cast<Recipe *>(this);
-      while (ancestor->m_ancestor_id > 0 && ancestor->m_ancestor_id != ancestor->key()) {
-         ancestor = ObjectStoreWrapper::getByIdRaw<Recipe>(ancestor->m_ancestor_id);
+      Recipe * recipe = const_cast<Recipe *>(this);
+      while (recipe && recipe->m_ancestor_id > 0 && recipe->m_ancestor_id != recipe->key()) {
+         qDebug() <<
+            Q_FUNC_INFO << "Search ancestors for Recipe #" << recipe->key() << "with m_ancestor_id" <<
+            recipe->m_ancestor_id;
+         Recipe * ancestor = ObjectStoreWrapper::getByIdRaw<Recipe>(recipe->m_ancestor_id);
+         if (!ancestor || ancestor->key() < 0) {
+            qWarning() <<
+               Q_FUNC_INFO << "Recipe #" << recipe->key() << "(" << recipe->name() << ")" <<
+               "has non-existent ancestor #" << recipe->m_ancestor_id;
+            break;
+         }
+
+         qDebug() << Q_FUNC_INFO << "Found ancestor Recipe #" << ancestor->key();
          ancestor->m_hasDescendants = true;
          this->m_ancestors.append(ancestor);
+         recipe = ancestor;
       }
    }