From 0c0d495f48e57c050ae386c390f1725dffc44122 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Fri, 15 Nov 2024 14:04:58 +0100 Subject: [PATCH 1/2] Error-handling fixes --- src/Application.cpp | 1 - src/LatestReleaseFinder.cpp | 139 +++++++++++++++++++----------------- src/MainWindow.cpp | 43 +++++++---- src/database/Database.h | 3 +- src/main.cpp | 8 ++- src/model/Recipe.cpp | 18 ++++- 6 files changed, 128 insertions(+), 84 deletions(-) diff --git a/src/Application.cpp b/src/Application.cpp index e25db568b..bd1d47112 100644 --- 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 index 895a87110..d8b60b28a 100644 --- a/src/LatestReleaseFinder.cpp +++ b/src/LatestReleaseFinder.cpp @@ -63,8 +63,8 @@ void LatestReleaseFinder::checkMainRespository() { // well use the newer standard. // boost::beast::http::request 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 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 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 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 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 index e2fe54083..af1f6e8d6 100644 --- 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::getInstance(), &ObjectStoreTyped::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(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(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 index 0ee83cb78..5589f64fc 100644 --- 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 index 692dab01f..9c6c3dd18 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -20,6 +20,8 @@ ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌*/ #include // Needs to be included exactly once in the code to use header-only version of Boost.JSON +#include + #include #include @@ -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 index b799c7d15..e11817bcb 100644 --- a/src/model/Recipe.cpp +++ b/src/model/Recipe.cpp @@ -2225,11 +2225,23 @@ QList 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(this); - while (ancestor->m_ancestor_id > 0 && ancestor->m_ancestor_id != ancestor->key()) { - ancestor = ObjectStoreWrapper::getByIdRaw(ancestor->m_ancestor_id); + Recipe * recipe = const_cast(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->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; } } From d423c237381f56e74e00556df1c882f5b2e56458 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Fri, 15 Nov 2024 18:08:07 +0100 Subject: [PATCH 2/2] Update changelog --- CHANGES.markdown | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.markdown b/CHANGES.markdown index 4d8f860ef..019caa774 100644 --- a/CHANGES.markdown +++ b/CHANGES.markdown @@ -17,7 +17,7 @@ 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) @@ -25,7 +25,7 @@ Bug fixes and minor enhancements. * 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.