From e09a141676fbb989df5b9eee86079a6dcc98a5ba Mon Sep 17 00:00:00 2001 From: Matt Young Date: Sun, 27 Oct 2024 09:19:41 +0100 Subject: [PATCH 1/7] Add small diagnostics for 866 --- src/Application.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/Application.cpp b/src/Application.cpp index b7f441fc5..598e98666 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -300,17 +300,24 @@ namespace { return; } + // // Nobody else needs to access this QNetworkAccessManager object, but it needs to carry on existing after this // function returns (otherwise the HTTP GET request will get cancelled), hence why we make it static. - static QNetworkAccessManager * manager = new QNetworkAccessManager(); + // + // As with a lot of things in Qt, passing in a "parent" QObject means Qt handles ownership of the object so + // having this as a raw pointer is not the resource leak it might appear. + // + static QNetworkAccessManager * manager = new QNetworkAccessManager(mw); static QString const releasesLatest = QString{"https://api.github.com/repos/%1/%2/releases/latest"}.arg(CONFIG_APPLICATION_NAME_UC, CONFIG_APPLICATION_NAME_LC); - QUrl url(releasesLatest); - responseToCheckForNewVersion = manager->get(QNetworkRequest(url)); + QUrl url{releasesLatest}; + QNetworkRequest networkRequest{url}; + qInfo() << + Q_FUNC_INFO << "Sending request to" << url << "to check for new version. (Timeout" << + manager->transferTimeout() << "ms, Redirect Policy" << manager->redirectPolicy() << ")"; + responseToCheckForNewVersion = manager->get(networkRequest); + qDebug() << Q_FUNC_INFO << "Request running =" << responseToCheckForNewVersion->isRunning(); // Since Qt5, you can connect signals to simple functions (see https://wiki.qt.io/New_Signal_Slot_Syntax) QObject::connect(responseToCheckForNewVersion, &QNetworkReply::finished, mw, &finishCheckForNewVersion); - qDebug() << - Q_FUNC_INFO << "Sending request to" << url << "to check for new version (request running =" << - responseToCheckForNewVersion->isRunning() << ")"; return; } From af03aedaff2a21b00f734d61a18691692dca4ba8 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 29 Oct 2024 02:22:37 +0100 Subject: [PATCH 2/7] Use Boost to do version check HTTP request. Still need timeouts etc. --- CMakeLists.txt | 8 +++ meson.build | 16 ++++- scripts/buildTool.py | 58 +++++++++--------- src/Application.cpp | 136 +++++++++++++++++++++++++++++-------------- src/main.cpp | 5 ++ 5 files changed, 148 insertions(+), 75 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index fc3bbe36d..32347961e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -766,6 +766,13 @@ include_directories(${XalanC_INCLUDE_DIRS}) message("Xalan-C++ include directories: ${XalanC_INCLUDE_DIRS}") message("Xalan-C++ libraries: ${XalanC_LIBRARIES}") +#===================================================== Find OpenSSL ==================================================== +# This is needed for us to use https with Boost.Asio +find_package(OpenSSL REQUIRED) +include_directories(${OPENSSL_INCLUDE_DIR}) +message("OpenSSL include directories: ${OPENSSL_INCLUDE_DIR}") +message("OpenSSL libraries: ${OPENSSL_LIBRARIES}") + if(APPLE) # TBD: Is this also needed when static linking Xerces on MacOS? find_package(CURL REQUIRED) @@ -1137,6 +1144,7 @@ set(appAndTestCommonLibraries ${DL_LIBRARY} ${XalanC_LIBRARIES} ${XercesC_LIBRARIES} + ${OPENSSL_LIBRARIES} ) if(APPLE) # Static linking Xerces and Xalan on MacOS means we have to explicitly say what libraries and frameworks they in turn diff --git a/meson.build b/meson.build index 1b02edc39..cbe180a46 100644 --- a/meson.build +++ b/meson.build @@ -198,7 +198,7 @@ project('brewtarget', 'cpp', 'buildtype=plain']) # -# Although Meson itself is written in Python, Meson build files uses a slihgtly different syntax and have less +# Although Meson itself is written in Python, Meson build files uses a slightly different syntax and have less # functionality than Python. See # https://mesonbuild.com/FAQ.html#why-is-meson-not-just-a-python-module-so-i-could-code-my-build-setup-in-python and # links therefrom for the rationale for avoiding being a full programming language. @@ -516,6 +516,17 @@ message('Xalan Library:', xalanDependency.name(), 'found =', xalanDependency.fou 'version =', xalanDependency.version(), 'path(s)=', xalanLibPaths) sharedLibraryPaths += xalanLibPaths +#======================================================= OpenSSL ======================================================= +# This is needed for us to use https with Boost.Asio +openSslDependency = dependency('OpenSSL', + version : '>=3.0.13', + required : true, + static : true) +#openSslLibPaths = openSslDependency.get_variable(cmake : 'PACKAGE_LIBRARIES') +message('OpenSSL Library:', openSslDependency.name(), 'found =', openSslDependency.found(), + 'version =', openSslDependency.version()) +#sharedLibraryPaths += openSslLibPaths + #====================================================== Valijson ======================================================= # Don't need to do anything special, other than set include directories below, as it's header-only and we pull it in as # a Git submodule. @@ -1278,6 +1289,7 @@ commonDependencies = [qtCommonDependencies, xalanDependency, boostDependency, dlDependency, + openSslDependency, # This isn't strictly needed for the testRunner, but no harm comes from including it backtraceDependency] mainExeDependencies = commonDependencies + qtMainExeDependencies testRunnerDependencies = commonDependencies + qtTestRunnerDependencies @@ -1345,7 +1357,7 @@ exportedVariables = { 'CONFIG_DESCRIPTION_STRING' : 'Open source brewing software', # Some installers, eg NSIS on Windows, want a brief copyright string - 'CONFIG_COPYRIGHT_STRING' : 'Copyright 2009-2023. Distributed under the terms of the GNU General Public License (version 3).', + 'CONFIG_COPYRIGHT_STRING' : 'Copyright 2009-2024. Distributed under the terms of the GNU General Public License (version 3).', # Installers often want the name of the organisation supplying the product, so we need something to put there 'CONFIG_ORGANIZATION_NAME' : 'The ' + capitalisedProjectName + ' Team', diff --git a/scripts/buildTool.py b/scripts/buildTool.py index 992236821..a8c9bc747 100755 --- a/scripts/buildTool.py +++ b/scripts/buildTool.py @@ -434,6 +434,7 @@ def installDependencies(): 'libqt6sql6-psql', 'libqt6sql6-sqlite', 'libqt6svg6-dev', + 'libssl-dev', # For OpenSSL headers 'libxalan-c-dev', 'libxerces-c-dev', 'lintian', @@ -826,6 +827,7 @@ def installDependencies(): 'mingw-w64-' + arch + '-nsis', 'mingw-w64-' + arch + '-freetype', 'mingw-w64-' + arch + '-harfbuzz', + 'mingw-w64-' + arch + '-openssl', # Needed for OpenSSL headers 'mingw-w64-' + arch + '-qt6-base', 'mingw-w64-' + arch + '-qt6-declarative', # Also needed for lupdate? 'mingw-w64-' + arch + '-qt6-static', @@ -837,7 +839,6 @@ def installDependencies(): 'mingw-w64-' + arch + '-xerces-c', 'mingw-w64-' + arch + '-angleproject', # See comment above 'mingw-w64-' + arch + '-ntldd', # Dependency tool useful for running manually -- see below - ] for packageToInstall in installList: log.debug('Installing ' + packageToInstall) @@ -934,6 +935,7 @@ def installDependencies(): 'pandoc', 'tree', 'qt@6', + 'openssl@3', # OpenSSL headers # 'xalan-c', # 'xerces-c' ] @@ -1989,41 +1991,41 @@ def doPackage(): #'Qt6PrintSupport', #'Qt6Sql' , #'Qt6Widgets' , - 'libb2', # BLAKE hash functions -- https://en.wikipedia.org/wiki/BLAKE_(hash_function) - 'libbrotlicommon', # Brotli compression -- see https://en.wikipedia.org/wiki/Brotli - 'libbrotlidec', # Brotli compression - 'libbrotlienc', # Brotli compression - 'libbz2', # BZip2 compression -- see https://en.wikipedia.org/wiki/Bzip2 + 'libb2' , # BLAKE hash functions -- https://en.wikipedia.org/wiki/BLAKE_(hash_function) + 'libbrotlicommon' , # Brotli compression -- see https://en.wikipedia.org/wiki/Brotli + 'libbrotlidec' , # Brotli compression + 'libbrotlienc' , # Brotli compression + 'libbz2' , # BZip2 compression -- see https://en.wikipedia.org/wiki/Bzip2 'libdouble-conversion', # Binary-decimal & decimal-binary routines for IEEE doubles -- see https://github.com/google/double-conversion - 'libfreetype', # Font rendering -- see https://freetype.org/ + 'libfreetype' , # Font rendering -- see https://freetype.org/ # # 32-bit and 64-bit MinGW use different exception handling (see # https://sourceforge.net/p/mingw-w64/wiki2/Exception%20Handling/) hence the different naming of libgcc in # the 32-bit and 64-bit environments. # -# 'libgcc_s_dw2', # 32-bit GCC library - 'libgcc_s_seh', # 64-bit GCC library - 'libglib-2.0', - 'libgraphite', - 'libharfbuzz', # HarfBuzz text shaping engine -- see https://github.com/harfbuzz/harfbuzz - 'libiconv', # See https://www.gnu.org/software/libiconv/ - 'libicudt', # Part of International Components for Unicode - 'libicuin', # Part of International Components for Unicode - 'libicuuc', # Part of International Components for Unicode - 'libintl', # See https://www.gnu.org/software/gettext/ - 'libmd4c', # Markdown for C -- see https://github.com/mity/md4c - 'libpcre2-8', # Perl Compatible Regular Expressions - 'libpcre2-16', # Perl Compatible Regular Expressions - 'libpcre2-32', # Perl Compatible Regular Expressions - 'libpng16', # Official PNG reference library -- see http://www.libpng.org/pub/png/libpng.html - 'libsqlite3', # Need this IN ADDITION to bin/sqldrivers/qsqlite.dll, which gets installed by windeployqt - 'libstdc++', +# 'libgcc_s_dw2' , # 32-bit GCC library + 'libgcc_s_seh' , # 64-bit GCC library + 'libglib-2.0' , + 'libgraphite' , + 'libharfbuzz' , # HarfBuzz text shaping engine -- see https://github.com/harfbuzz/harfbuzz + 'libiconv' , # See https://www.gnu.org/software/libiconv/ + 'libicudt' , # Part of International Components for Unicode + 'libicuin' , # Part of International Components for Unicode + 'libicuuc' , # Part of International Components for Unicode + 'libintl' , # See https://www.gnu.org/software/gettext/ + 'libmd4c' , # Markdown for C -- see https://github.com/mity/md4c + 'libpcre2-8' , # Perl Compatible Regular Expressions + 'libpcre2-16' , # Perl Compatible Regular Expressions + 'libpcre2-32' , # Perl Compatible Regular Expressions + 'libpng16' , # Official PNG reference library -- see http://www.libpng.org/pub/png/libpng.html + 'libsqlite3' , # Need this IN ADDITION to bin/sqldrivers/qsqlite.dll, which gets installed by windeployqt + 'libstdc++' , 'libwinpthread', - 'libxalan-c', - 'libxalanMsg', + 'libxalan-c' , + 'libxalanMsg' , 'libxerces-c-3', - 'libzstd', # ZStandard (aka zstd) = fast lossless compression algorithm - 'zlib', # ZLib compression library + 'libzstd' , # ZStandard (aka zstd) = fast lossless compression algorithm + 'zlib' , # ZLib compression library ]: found = False for searchDir in pathsToSearch: diff --git a/src/Application.cpp b/src/Application.cpp index 598e98666..e4e401dab 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -36,6 +36,14 @@ #include #include // For std::call_once etc +//¥¥vv +#include + +#include +#include +#include +//¥¥^^ + #include #include #include @@ -196,22 +204,92 @@ namespace { return; } - QNetworkReply * responseToCheckForNewVersion = nullptr; - /** - * \brief Once the response is received to the web request to get latest version info, this parses it and, if - * necessary, prompts the user to upgrade. - * See \c initiateCheckForNewVersion. + * \brief Check whether there is a newer version of the software available to download from GitHub. If so, ask the + * user whether they want to upgrade. (We don't actually DO the upgrade. We just take them to the place they + * can download it.) */ - void finishCheckForNewVersion() { - if (!responseToCheckForNewVersion) { - qDebug() << Q_FUNC_INFO << "Invalid sender"; + void checkForNewVersion() { + // + // Users can turn off the check for new versions + // + if (!checkVersion) { + qDebug() << Q_FUNC_INFO << "Check for new version is disabled"; return; } - // If there is an error, just return. - if (responseToCheckForNewVersion->error() != QNetworkReply::NoError) { - qDebug() << Q_FUNC_INFO << "Error checking for update:" << responseToCheckForNewVersion->error(); + // + // Checking for the latest version involves requesting a JSON object from the GitHub API over HTTPS. + // + // Previously we used the Qt framework (QNetworkAccessManager / QNetworkRequest / QNetworkReply) to do the HTTP + // request/response. The problem with this is that, when something goes wrong it can be rather hard to diagnose. + // Eg we had a bug that triggered a stack overflow in the Qt internals but there was only a limited amount of + // logging we could add to try to determine what was going on. + // + // So now, instead, we use Boost.Beast (which sits on top of Boost.Asio) and OpenSSL. This is very slightly + // lower-level -- in that fewer things are magically defaulted for you -- and requires us to use std::string + // rather than QString. But at least it does not require a callback function. And, should we have future + // problems, it should be easier to delve into. + // + // Although it's a bit long-winded, we're not really doing anything clever here. The example code at + // https://www.boost.org/doc/libs/1_86_0/libs/beast/doc/html/beast/quick_start/http_client.html explains a lot of + // what's going on. We're just doing a bit extra to do HTTPS rather than HTTP. + // + std::string const host{"api.github.com"}; + // It would be neat to construct this string at compile-time, but I haven't yet worked out how! + std::string const path = QString{"/repos/%1/%2/releases/latest"}.arg(CONFIG_APPLICATION_NAME_UC, CONFIG_APPLICATION_NAME_LC).toStdString(); + std::string const port{"443"}; + // + // Here 11 means HTTP/1.1, 20 means HTTP/2.0, 30 means HTTP/3.0. (See + // https://www.boost.org/doc/libs/1_86_0/libs/beast/doc/html/beast/ref/boost__beast__http__message/version/overload1.html.) + // If we were doing something generic then we'd stick with HTTP/1.1 since that has 100% support. But, since we're + // only making one request, and it's to GitHub, and we know they support they newer version of HTTP, we might as + // well use the newer standard. + // + boost::beast::http::request httpRequest{boost::beast::http::verb::get, + 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 + // and will even accept empty string. + // + httpRequest.set(boost::beast::http::field::user_agent, ""); + + 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); + // TODO: Need to add time-outs here. + + // 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); + + // We're expecting a response back pretty quickly, so we'll just wait for it here. If we find response times are + // too + 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 could'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; } @@ -224,7 +302,7 @@ namespace { // 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 = responseToCheckForNewVersion->readAll(); + QByteArray rawContent = QByteArray::fromStdString(httpResponse.body()); QJsonParseError jsonParseError{}; QJsonDocument jsonDocument = QJsonDocument::fromJson(rawContent, &jsonParseError); @@ -289,38 +367,6 @@ namespace { } - /** - * \brief Sends a web request to check whether there is a newer version of the software available - */ - void initiateCheckForNewVersion(MainWindow* mw) { - - // Don't do anything if the checkVersion flag was set false - if (!checkVersion) { - qDebug() << Q_FUNC_INFO << "Check for new version is disabled"; - return; - } - - // - // Nobody else needs to access this QNetworkAccessManager object, but it needs to carry on existing after this - // function returns (otherwise the HTTP GET request will get cancelled), hence why we make it static. - // - // As with a lot of things in Qt, passing in a "parent" QObject means Qt handles ownership of the object so - // having this as a raw pointer is not the resource leak it might appear. - // - static QNetworkAccessManager * manager = new QNetworkAccessManager(mw); - static QString const releasesLatest = QString{"https://api.github.com/repos/%1/%2/releases/latest"}.arg(CONFIG_APPLICATION_NAME_UC, CONFIG_APPLICATION_NAME_LC); - QUrl url{releasesLatest}; - QNetworkRequest networkRequest{url}; - qInfo() << - Q_FUNC_INFO << "Sending request to" << url << "to check for new version. (Timeout" << - manager->transferTimeout() << "ms, Redirect Policy" << manager->redirectPolicy() << ")"; - responseToCheckForNewVersion = manager->get(networkRequest); - qDebug() << Q_FUNC_INFO << "Request running =" << responseToCheckForNewVersion->isRunning(); - // Since Qt5, you can connect signals to simple functions (see https://wiki.qt.io/New_Signal_Slot_Syntax) - QObject::connect(responseToCheckForNewVersion, &QNetworkReply::finished, mw, &finishCheckForNewVersion); - return; - } - /** * \brief This is only called from \c Application::getResourceDir to initialise the variable it returns * @@ -496,7 +542,7 @@ int Application::run() { mainWindow.setVisible(true); splashScreen.finish(&mainWindow); - initiateCheckForNewVersion(&mainWindow); + checkForNewVersion(); do { ret = qApp->exec(); } while (ret == 1000); diff --git a/src/main.cpp b/src/main.cpp index e1f3844f5..692dab01f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -109,6 +109,11 @@ int main(int argc, char **argv) { // always enabled. This attribute no longer has any effect." // + // + // This ensures the resources in "resources.qrc" are initialised at startup. + // + Q_INIT_RESOURCE(resources); + // // Various bits of Qt initialisation need to be done straight away for other Qt functionality to work correctly // From 70c3f17acbec5b689e6420fa2388cb8560760cc2 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 29 Oct 2024 02:43:53 +0100 Subject: [PATCH 3/7] Build fix --- meson.build | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index cbe180a46..f9d5fcf71 100644 --- a/meson.build +++ b/meson.build @@ -518,8 +518,12 @@ sharedLibraryPaths += xalanLibPaths #======================================================= OpenSSL ======================================================= # This is needed for us to use https with Boost.Asio +# +# As of 2024-10-29, 3.0.2 is the most recent version of OpenSSL available on Ubuntu 22.04, so that's the minimum we +# specify here. +# openSslDependency = dependency('OpenSSL', - version : '>=3.0.13', + version : '>=3.0.2', required : true, static : true) #openSslLibPaths = openSslDependency.get_variable(cmake : 'PACKAGE_LIBRARIES') From 2ddeaf175e35fa1e1078a40f3b5cfe9837b28507 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 29 Oct 2024 11:25:21 +0100 Subject: [PATCH 4/7] Move check for latest release onto a background thread --- meson.build | 2 + src/Application.cpp | 247 +++++++++++++----------------------- src/CMakeLists.txt | 1 + src/LatestReleaseFinder.cpp | 150 ++++++++++++++++++++++ src/LatestReleaseFinder.h | 51 ++++++++ src/MainWindow.cpp | 6 +- src/MainWindow.h | 14 +- 7 files changed, 310 insertions(+), 161 deletions(-) create mode 100644 src/LatestReleaseFinder.cpp create mode 100644 src/LatestReleaseFinder.h diff --git a/meson.build b/meson.build index f9d5fcf71..64c4c1849 100644 --- a/meson.build +++ b/meson.build @@ -630,6 +630,7 @@ commonSourceFiles = files([ 'src/HydrometerTool.cpp', 'src/IbuGuSlider.cpp', 'src/InventoryFormatter.cpp', + 'src/LatestReleaseFinder.cpp', 'src/Localization.cpp', 'src/Logging.cpp', 'src/MainWindow.cpp', @@ -865,6 +866,7 @@ mocHeaders = files([ 'src/HelpDialog.h', 'src/HydrometerTool.h', 'src/IbuGuSlider.h', + 'src/LatestReleaseFinder.h', 'src/MainWindow.h', 'src/MashDesigner.h', 'src/MashWizard.h', diff --git a/src/Application.cpp b/src/Application.cpp index e4e401dab..2e5f72f52 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -36,25 +36,13 @@ #include #include // For std::call_once etc -//¥¥vv -#include - -#include -#include -#include -//¥¥^^ - #include #include #include -#include -#include #include #include #include -#include -#include -#include +#include #include #include @@ -62,6 +50,7 @@ #include "BtSplashScreen.h" #include "config.h" #include "database/Database.h" +#include "LatestReleaseFinder.h" #include "Localization.h" #include "MainWindow.h" #include "measurement/ColorMethods.h" @@ -91,11 +80,10 @@ namespace { bool interactive = true; //! \brief If this option is false, do not bother the user about new versions. - bool checkVersion = true; + bool tellUserAboutNewRelease = true; - void setCheckVersion(bool value) { - checkVersion = value; - } + //! \brief Worker thread for finding the latest released version of the program + QThread latestReleaseFinderThread; /** * \brief Create a directory if it doesn't exist, popping a error dialog if creation fails @@ -205,122 +193,16 @@ namespace { } /** - * \brief Check whether there is a newer version of the software available to download from GitHub. If so, ask the - * user whether they want to upgrade. (We don't actually DO the upgrade. We just take them to the place they - * can download it.) + * \brief This is called to tell us the version number of the latest release of the program in its main GitHub + * repository. + * + * NB: AFAICT, because this is a freestanding function, rather than a member function of a QObject subclass, + * this gets run on the caller's thread. In particular this means it could run at the same time as other + * code running inside the program's main Qt event loop. If we wanted to do anything that might risk + * clashing with other code then we would either want to add locking here or move this function to be, + * say, a slot on one of our objects (eg MainWindow). */ - void checkForNewVersion() { - // - // Users can turn off the check for new versions - // - if (!checkVersion) { - qDebug() << Q_FUNC_INFO << "Check for new version is disabled"; - return; - } - - // - // Checking for the latest version involves requesting a JSON object from the GitHub API over HTTPS. - // - // Previously we used the Qt framework (QNetworkAccessManager / QNetworkRequest / QNetworkReply) to do the HTTP - // request/response. The problem with this is that, when something goes wrong it can be rather hard to diagnose. - // Eg we had a bug that triggered a stack overflow in the Qt internals but there was only a limited amount of - // logging we could add to try to determine what was going on. - // - // So now, instead, we use Boost.Beast (which sits on top of Boost.Asio) and OpenSSL. This is very slightly - // lower-level -- in that fewer things are magically defaulted for you -- and requires us to use std::string - // rather than QString. But at least it does not require a callback function. And, should we have future - // problems, it should be easier to delve into. - // - // Although it's a bit long-winded, we're not really doing anything clever here. The example code at - // https://www.boost.org/doc/libs/1_86_0/libs/beast/doc/html/beast/quick_start/http_client.html explains a lot of - // what's going on. We're just doing a bit extra to do HTTPS rather than HTTP. - // - std::string const host{"api.github.com"}; - // It would be neat to construct this string at compile-time, but I haven't yet worked out how! - std::string const path = QString{"/repos/%1/%2/releases/latest"}.arg(CONFIG_APPLICATION_NAME_UC, CONFIG_APPLICATION_NAME_LC).toStdString(); - std::string const port{"443"}; - // - // Here 11 means HTTP/1.1, 20 means HTTP/2.0, 30 means HTTP/3.0. (See - // https://www.boost.org/doc/libs/1_86_0/libs/beast/doc/html/beast/ref/boost__beast__http__message/version/overload1.html.) - // If we were doing something generic then we'd stick with HTTP/1.1 since that has 100% support. But, since we're - // only making one request, and it's to GitHub, and we know they support they newer version of HTTP, we might as - // well use the newer standard. - // - boost::beast::http::request httpRequest{boost::beast::http::verb::get, - 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 - // and will even accept empty string. - // - httpRequest.set(boost::beast::http::field::user_agent, ""); - - 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); - // TODO: Need to add time-outs here. - - // 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); - - // We're expecting a response back pretty quickly, so we'll just wait for it here. If we find response times are - // too - 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 could'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) { - 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); - } + void checkAgainstLatestRelease(QVersionNumber const latestRelease) { // // We used to just compare if the remote version is the same as the current one, but it then gets annoying if you @@ -328,41 +210,65 @@ namespace { // running 4.0.1. So now we do it properly, letting QVersionNumber do the heavy lifting for us. // QVersionNumber const currentlyRunning{QVersionNumber::fromString(CONFIG_VERSION_STRING)}; - QVersionNumber const latestRelease {QVersionNumber::fromString(remoteVersion)}; qInfo() << - Q_FUNC_INFO << "Latest release is" << remoteVersion << "(parsed as" << latestRelease << ") ; " - "currently running" << CONFIG_VERSION_STRING << "(parsed as" << currentlyRunning << ")"; + Q_FUNC_INFO << "Latest release is" << latestRelease.toString() << ") ; currently running" << + CONFIG_VERSION_STRING << "(parsed as" << currentlyRunning.toString() << ")"; - // If the remote version is newer... if (latestRelease > currentlyRunning) { - // ...and the user wants to download the new version... - if(QMessageBox::information(&MainWindow::instance(), - QObject::tr("New Version"), - QObject::tr("Version %1 is now available. Download it?").arg(remoteVersion), - QMessageBox::Yes | QMessageBox::No, - QMessageBox::Yes) == QMessageBox::Yes) { - // ...take them to the website. + // + // Users can turn off the notification about new versions, though note below that this gets reset once they are + // running the latest version again. + // + if (!tellUserAboutNewRelease) { + qInfo() << Q_FUNC_INFO << "Check for new version is disabled"; + return; + } + + // + // The latest release is newer than what we are currently running. + // See if the user wants to download the newer version. + // + bool const wantsToDownload{ + QMessageBox::Yes == QMessageBox::information( + &MainWindow::instance(), + QObject::tr("New Version"), + QObject::tr("Version %1 is now available. Download it?").arg(latestRelease.toString()), + QMessageBox::Yes | QMessageBox::No, + QMessageBox::Yes + ) + }; + if (wantsToDownload) { + // + // It would be a bit of a tall order for the program to upgrade itself in place. We just take the user to + // the release download page. + // static QString const releasesPage = QString{"%1/releases"}.arg(CONFIG_GITHUB_URL); QDesktopServices::openUrl(QUrl(releasesPage)); - } else { - // ... and the user does NOT want to download the new version... - // ... and they want us to stop bothering them... + } else { + // + // If the user doesn't want to be taken to the download page, give them the option to stop being reminded + // about the new release. + // if(QMessageBox::question(&MainWindow::instance(), QObject::tr("New Version"), QObject::tr("Stop bothering you about new versions?"), QMessageBox::Yes | QMessageBox::No, QMessageBox::Yes) == QMessageBox::Yes) { // ... make a note to stop bothering the user about the new version. - setCheckVersion(false); + tellUserAboutNewRelease = false; } } return; } - // The current version is newest so make a note to bother users about future new versions. - // This means that when a user downloads the new version, this variable will always get reset to true. - setCheckVersion(true); + // + // The user is already running the latest release. Make sure the user gets notified when a newer release is + // available. (They can then turn off that notification if they want.) Essentially, this means that, every time + // the user installs whatever the latest version of the software is, we ensure that we will tell them at least + // once when there is a new release. + // + tellUserAboutNewRelease = true; return; } @@ -480,6 +386,9 @@ bool Application::initialize() { // Make sure all the necessary directories and files we need exist before starting. ensureDirectoriesExist(); + // TODO: Seems a bit ugly that we call readSystemOptions here but saveSystemOptions from MainWindow::closeEvent. + // In the long run, it would be a lot more elegant to use RAII to automatically store everything we read from + // PersistentSettings. Application::readSystemOptions(); Localization::loadTranslations(); // Do internationalization. @@ -512,6 +421,13 @@ void Application::cleanup() { MainWindow::DeleteMainWindow(); Database::instance().unload(); + + // + // Ensure the thread we spawned to check for new versions is properly terminated + // + latestReleaseFinderThread.quit(); + latestReleaseFinderThread.wait(); + return; } @@ -536,13 +452,28 @@ int Application::run() { } Database::instance().checkForNewDefaultData(); - // .:TBD:. Could maybe move the calls to init and setVisible inside createMainWindowInstance() in MainWindow.cpp + // + // Make sure the MainWindow singleton exists, but don't initialise it just yet. We're going to use the end of the + // initialisation to trigger the background thread that checks to see whether a new version of the program is + // available, so we want to set that background thread up first. (But we need the MainWindow object to exist, so we + // can connect signals and slots.) + // MainWindow & mainWindow = MainWindow::instance(); - mainWindow.init(); - mainWindow.setVisible(true); - splashScreen.finish(&mainWindow); - checkForNewVersion(); + // + // It feels a bit wrong these days to be calling `new` directly, but it is the way to do things in Qt. We tell the + // framework about the object we've created, and it handles clean-up for us. + // + LatestReleaseFinder * latestReleaseFinder = new LatestReleaseFinder{}; + latestReleaseFinder->moveToThread(&latestReleaseFinderThread); + mainWindow.connect(&latestReleaseFinderThread, &QThread::finished, latestReleaseFinder, &QObject::deleteLater); + + mainWindow.connect(&mainWindow, &MainWindow::initialisedAndVisible, latestReleaseFinder, &LatestReleaseFinder::checkMainRespository); + mainWindow.connect(latestReleaseFinder, &LatestReleaseFinder::foundLatestRelease, &checkAgainstLatestRelease); + latestReleaseFinderThread.start(); + + mainWindow.initialiseAndMakeVisible(); + splashScreen.finish(&mainWindow); do { ret = qApp->exec(); } while (ret == 1000); @@ -559,8 +490,8 @@ void Application::readSystemOptions() { updateConfig(); //================Version Checking======================== - checkVersion = PersistentSettings::value(PersistentSettings::Names::check_version, QVariant(true)).toBool(); - qDebug() << Q_FUNC_INFO << "checkVersion=" << checkVersion; + tellUserAboutNewRelease = PersistentSettings::value(PersistentSettings::Names::check_version, QVariant(true)).toBool(); + qDebug() << Q_FUNC_INFO << "tellUserAboutNewRelease=" << tellUserAboutNewRelease; Measurement::loadDisplayScales(); @@ -578,7 +509,7 @@ void Application::readSystemOptions() { } void Application::saveSystemOptions() { - PersistentSettings::insert(PersistentSettings::Names::check_version, checkVersion); + PersistentSettings::insert(PersistentSettings::Names::check_version, tellUserAboutNewRelease); //setOption("user_data_dir", userDataDir); Localization::saveSettings(); diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d51da5a58..79fc0e23e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -61,6 +61,7 @@ set(filesToCompile_cpp ${repoDir}/src/HydrometerTool.cpp ${repoDir}/src/IbuGuSlider.cpp ${repoDir}/src/InventoryFormatter.cpp + ${repoDir}/src/LatestReleaseFinder ${repoDir}/src/Localization.cpp ${repoDir}/src/Logging.cpp ${repoDir}/src/MainWindow.cpp diff --git a/src/LatestReleaseFinder.cpp b/src/LatestReleaseFinder.cpp new file mode 100644 index 000000000..895a87110 --- /dev/null +++ b/src/LatestReleaseFinder.cpp @@ -0,0 +1,150 @@ +/*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ + * LatestReleaseFinder.h is part of Brewtarget, and is copyright the following authors 2024: + * • Matt Young + * + * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later + * version. + * + * Brewtarget is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with this program. If not, see + * . + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌*/ +#include "LatestReleaseFinder.h" + +#include + +#include +#include +#include + +#include +#include +#include +#include + +#include "config.h" + +void LatestReleaseFinder::checkMainRespository() { + + // + // Checking for the latest version involves requesting a JSON object from the GitHub API over HTTPS. + // + // Previously we used the Qt framework (QNetworkAccessManager / QNetworkRequest / QNetworkReply) to do the HTTP + // request/response. The problem with this is that, when something goes wrong it can be rather hard to diagnose. + // Eg we had a bug that triggered a stack overflow in the Qt internals but there was only a limited amount of + // logging we could add to try to determine what was going on. + // + // So now, instead, we use Boost.Beast (which sits on top of Boost.Asio) and OpenSSL. This is very slightly + // lower-level -- in that fewer things are magically defaulted for you -- and requires us to use std::string + // rather than QString. But at least it does not require a callback function. And, should we have future + // problems, it should be easier to delve into. + // + // Although it's a bit long-winded, we're not really doing anything clever here. The example code at + // https://www.boost.org/doc/libs/1_86_0/libs/beast/doc/html/beast/quick_start/http_client.html explains a lot of + // what's going on. We're just doing a bit extra to do HTTPS rather than HTTP. + // + // Because we're running on a separate thread, we don't worry about doing timeouts etc for the individual parts of + // the request/response below. If something takes a long time or we don't get a response at all, no harm is done. + // The main program carries on running on the main thread and just never receives the foundLatestRelease signal. + // + std::string const host{"api.github.com"}; + // It would be neat to construct this string at compile-time, but I haven't yet worked out how! + std::string const path = QString{"/repos/%1/%2/releases/latest"}.arg(CONFIG_APPLICATION_NAME_UC, CONFIG_APPLICATION_NAME_LC).toStdString(); + std::string const port{"443"}; + // + // Here 11 means HTTP/1.1, 20 means HTTP/2.0, 30 means HTTP/3.0. (See + // https://www.boost.org/doc/libs/1_86_0/libs/beast/doc/html/beast/ref/boost__beast__http__message/version/overload1.html.) + // If we were doing something generic then we'd stick with HTTP/1.1 since that has 100% support. But, since we're + // only making one request, and it's to GitHub, and we know they support they newer version of HTTP, we might as + // well use the newer standard. + // + boost::beast::http::request httpRequest{boost::beast::http::verb::get, + 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 + // and will even accept empty string. + // + httpRequest.set(boost::beast::http::field::user_agent, ""); + + std::ostringstream requestAsString; + requestAsString << "https://" << host << ":" << port << path; + qInfo() << + 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) { + // + // 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) { + 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); + + return; +} diff --git a/src/LatestReleaseFinder.h b/src/LatestReleaseFinder.h new file mode 100644 index 000000000..bd43b9dc0 --- /dev/null +++ b/src/LatestReleaseFinder.h @@ -0,0 +1,51 @@ +/*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ + * LatestReleaseFinder.h is part of Brewtarget, and is copyright the following authors 2024: + * • Matt Young + * + * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later + * version. + * + * Brewtarget is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with this program. If not, see + * . + ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌*/ +#ifndef LATESTRELEASEFINDER_H +#define LATESTRELEASEFINDER_H +#pragma once + +#include +#include + +/** + * \class LatestReleaseFinder + * + * This class runs on a background thread, just after application start-up. On receiving the + * \c checkForNewVersion signal, it makes a simple HTTP request to the latest version of the program available at + * its main GitHub repository. If this succeeds, it then sends a \c newVersionFound signal to the main code so + * that it can tell the user. + * + * It would be possible to have done this in slightly less code by inheriting from \c QThread and overriding + * \c QThread::run (as suggested as an alternate approach at https://doc.qt.io/qt-6/qthread.html#details). + * However, that feels like a "wrong" design decision in that it tightly couples "code we want to run on a + * thread" with "mechanism for instantiating and running a thread". So, instead, we keep \c QThread separate, + * and use signals and slots to kick off some work and to communicate its results. It's very slightly more code, + * but it feels less clunky. + * + * For the moment at least, this is the only bit of HTTP that the program does, so we haven't made things more + * generic -- eg by writing an HttpRequester object or some such. + */ +class LatestReleaseFinder : public QObject { + Q_OBJECT + +public slots: + void checkMainRespository(); + +signals: + void foundLatestRelease(QVersionNumber const latestRelease); +}; + +#endif diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp index 12eb27e94..c04a26704 100644 --- a/src/MainWindow.cpp +++ b/src/MainWindow.cpp @@ -1055,7 +1055,7 @@ MainWindow::MainWindow(QWidget* parent) : QMainWindow(parent), pimpl{std::make_u return; } -void MainWindow::init() { +void MainWindow::initialiseAndMakeVisible() { qDebug() << Q_FUNC_INFO; this->setupCSS(); @@ -1117,6 +1117,10 @@ void MainWindow::init() { // the databae is changed (as setToolTip() just takes static text as its parameter). label_Brewtarget->setToolTip(getLabelToolTip()); + this->setVisible(true); + + emit initialisedAndVisible(); + qDebug() << Q_FUNC_INFO << "MainWindow initialisation complete"; return; } diff --git a/src/MainWindow.h b/src/MainWindow.h index a13c48a89..8da3fad60 100644 --- a/src/MainWindow.h +++ b/src/MainWindow.h @@ -86,7 +86,7 @@ class MainWindow : public QMainWindow, public Ui::mainWindow { * code that calls Application::mainWindow() which returns a pointer to the MainWindow and therefore needs the * MainWindow constructor to have returned! */ - void init(); + void initialiseAndMakeVisible(); //! \brief Get the currently observed recipe. Recipe* currentRecipe(); @@ -309,12 +309,22 @@ public slots: void showChanges(QMetaProperty* prop = nullptr); protected: - virtual void closeEvent(QCloseEvent* event); + //! \brief Overrides \c QWidget::closeEvent + virtual void closeEvent(QCloseEvent* event) override; private slots: //! \brief Set whether undo / redo commands are enabled void setUndoRedoEnable(); +signals: + /** + * \brief Emitted when \c MainWindow object is initialised (at end of \c MainWindow::initialiseAndMakeVisible). + * + * For the moment, we use this to trigger the background thread that checks to see whether a new version of + * the software is available. + */ + void initialisedAndVisible(); + private: // Private implementation details - see https://herbsutter.com/gotw/_100/ class impl; From 07a350f26bf17615dd67fe661207bc73d7ec7cf8 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 29 Oct 2024 16:59:21 +0100 Subject: [PATCH 5/7] Tidy up and update change log --- CHANGES.markdown | 5 +-- bt | 7 ++--- scripts/buildTool.py | 63 ++++++++++++++++++++++--------------- src/Application.cpp | 4 ++- src/Localization.cpp | 66 ++++++++++++++++++++++----------------- src/Localization.h | 8 ++--- src/OptionDialog.cpp | 49 +++++++++++++++-------------- src/model/NamedEntity.cpp | 4 ++- 8 files changed, 117 insertions(+), 89 deletions(-) diff --git a/CHANGES.markdown b/CHANGES.markdown index 88e1fddc0..791c4aad2 100644 --- a/CHANGES.markdown +++ b/CHANGES.markdown @@ -20,9 +20,10 @@ Bug fixes and minor enhancements. * None ### Bug Fixes -* None +* Crash (stack overflow in Qt) on some Windows builds during "check for new version" [866](https://github.com/Brewtarget/brewtarget/issues/866) + ### Release Timestamp -Sat, 26 Oct 2024 04:00:09 +0100 +Tue, 29 Oct 2024 04:00:09 +0100 ## v4.0.8 Bug fixes and minor enhancements. diff --git a/bt b/bt index 9c29af18c..d217090ac 100755 --- a/bt +++ b/bt @@ -31,14 +31,13 @@ # need to be for a local install). Then creates a distributable package, making use # of various build variables passed back from Meson. # -# NOTE: This tool, used in conjunction with Meson, is now the official way to build and package the software. We -# continue to support CMake for local compiles and installs, but not for packaging. +# NOTE: This tool, used in conjunction with Meson, is now the official way to build and package the software. We will +# continue to support CMake indefinitely for local compiles and installs, since it is such a widespread and +# familiar build system for C++ projects. However, we no longer attempt to support packaging via CPack. # # .:TODO:. At some point we should be able to retire: # configure # setupgit.sh -# CMakeLists.txt -# src/CMakeLists.txt # # ********************************************************************************************************************** # * diff --git a/scripts/buildTool.py b/scripts/buildTool.py index a8c9bc747..96691eafc 100755 --- a/scripts/buildTool.py +++ b/scripts/buildTool.py @@ -827,7 +827,7 @@ def installDependencies(): 'mingw-w64-' + arch + '-nsis', 'mingw-w64-' + arch + '-freetype', 'mingw-w64-' + arch + '-harfbuzz', - 'mingw-w64-' + arch + '-openssl', # Needed for OpenSSL headers + 'mingw-w64-' + arch + '-openssl', # OpenSSL headers and library 'mingw-w64-' + arch + '-qt6-base', 'mingw-w64-' + arch + '-qt6-declarative', # Also needed for lupdate? 'mingw-w64-' + arch + '-qt6-static', @@ -935,7 +935,7 @@ def installDependencies(): 'pandoc', 'tree', 'qt@6', - 'openssl@3', # OpenSSL headers + 'openssl@3', # OpenSSL headers and library # 'xalan-c', # 'xerces-c' ] @@ -2084,12 +2084,18 @@ def doPackage(): ) # - # Make the checksum file TODO + # Make the checksum file # winInstallerName = capitalisedProjectName + ' ' + buildConfig['CONFIG_VERSION_STRING'] + ' Installer.exe' log.info('Generating checksum file for ' + winInstallerName) writeSha256sum(dir_packages_platform, winInstallerName) + #-------------------------------------------------------------------------------------------------------------- + # Signing Windows binaries is a separate step. For Brewtarget, it is possible, with the help of SignPath, to + # do via GitHub Actions. (For Brewken, we do not yet have enough standing/users to qualify for the SignPath + # Open Source Software sponsorship.) + #-------------------------------------------------------------------------------------------------------------- + #----------------------------------------------------------------------------------------------------------------- #------------------------------------------------- Mac Packaging ------------------------------------------------- #----------------------------------------------------------------------------------------------------------------- @@ -2228,7 +2234,6 @@ def doPackage(): # Rather than create dir_packages_mac_rsc directly, it's simplest to copy the whole Resources tree from # mbuild/mackages/darwin/usr/local/Contents/Resources, as we want everything that's inside it log.debug('Copying Resources') -# shutil.copytree(dir_packages_platform.joinpath('usr/local/Contents/Resources'), dir_packages_mac_rsc) shutil.copytree(dir_packages_platform.joinpath('opt/homebrew/Contents/Resources'), dir_packages_mac_rsc) # Copy the Information Property List file to where it belongs @@ -2238,8 +2243,6 @@ def doPackage(): # Because Meson is geared towards local installs, in the mbuild/mackages/darwin directory, it is going to have # placed the executable in the usr/local/bin or opt/homebrew/bin subdirectory. Copy it to the right place. log.debug('Copying executable') -# shutil.copy2(dir_packages_platform.joinpath('usr/local/bin').joinpath(capitalisedProjectName).as_posix(), -# dir_packages_mac_bin) shutil.copy2(dir_packages_platform.joinpath('opt/homebrew/bin').joinpath(capitalisedProjectName).as_posix(), dir_packages_mac_bin) @@ -2372,24 +2375,6 @@ def doPackage(): log.debug('Copying ' + xalanDir + xalanMsgLibName + ' to ' + dir_packages_mac_frm.as_posix()) shutil.copy2(xalanDir + xalanMsgLibName, dir_packages_mac_frm) -### # -### # Let's copy libxalan-c.112.dylib to where it needs to go and hope that macdeployqt does not overwrite it -### # -### shutil.copy2(xalanDir + xalanLibName, dir_packages_mac_frm) -### -### # -### # We need to fix up libxalan-c.112.dylib so that it looks for libxalanMsg.112.dylib in the right place. Long -### # story short, this means changing '@rpath/libxalanMsg.112.dylib' to '@loader_path/libxalanMsg.112.dylib' -### # -### os.chdir(dir_packages_mac_frm) -### btUtils.abortOnRunFail( -### subprocess.run(['install_name_tool', -### '-change', -### xalanMsgLibName, -### '@loader_path/' + xalanMsgLibName], -### capture_output=False) -### ) - # # Now let macdeployqt do most of the heavy lifting # @@ -2407,11 +2392,39 @@ def doPackage(): # # .:TBD:. Ideally we would sign our application here using the `-codesign=` command line option to # macdeployqt. For the GitHub builds, we would have to import a code signing certificate using - # https://github.com/Apple-Actions/import-codesign-certs. + # https://github.com/Apple-Actions/import-codesign-certs. (Note that we would need to sign both the + # app and the disk image.) # # However, getting an identity and certificate with which to sign is a bit complicated. For a start, # Apple pretty much require you to sign up to their $99/year developer program. # + # As explained at https://wiki.lazarus.freepascal.org/Code_Signing_for_macOS, Apple do not want you to + # run unsigned MacOS applications, and are making it every harder to do so. As of 2024, if you try to + # launch an unsigned executable on MacOS that you didn't download from an Apple-approved source, you'll + # get two layers of errors: + # - First you'll be told that the application "is damaged and can’t be opened. You should move it to + # the Trash". You can fix this by running the xattr command (as suggested at + # https://discussions.apple.com/thread/253714860) + # - If you now try to run the application, you'll get a different error: that the application "quit + # unexpectedly". When you click on "Report...", you'll see, buried in amongst a huge amount of + # other information, the message "Exception Type: EXC_BAD_ACCESS (SIGKILL (Code Signature + # Invalid))". This can apparently be fixed by doing an "ad hoc" signing with the codesign command + # (as explained at the aforementioned https://wiki.lazarus.freepascal.org/Code_Signing_for_macOS). + # + # ❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄ + # ❄ + # ❄ TLDR for Mac users, once you've built or downloaded the app, you still need to do the following + # ❄ "Simon says run this app" incantations to get it to work. (This is because Apple knows best. + # ❄ Do not question the Apple. Do not step outside the reality distortion field. Do not pass Go. + # ❄ Etc.) Make sure you have Xcode installed from the Mac App Store (see + # ❄ https://developer.apple.com/support/xcode/). Open the console and run the following (with the + # ❄ appropriate substitution for ): + # ❄ + # ❄ $ xattr -c + # ❄ $ codesign --force --deep -s - + # ❄ + # ❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄❄ + # log.debug('Running macdeployqt') os.chdir(dir_packages_platform) btUtils.abortOnRunFail( diff --git a/src/Application.cpp b/src/Application.cpp index 2e5f72f52..28473cd8c 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -389,9 +389,11 @@ bool Application::initialize() { // TODO: Seems a bit ugly that we call readSystemOptions here but saveSystemOptions from MainWindow::closeEvent. // In the long run, it would be a lot more elegant to use RAII to automatically store everything we read from // PersistentSettings. + // Application::readSystemOptions(); - Localization::loadTranslations(); // Do internationalization. +/// // Load in the default / remembered translations +/// Localization::loadTranslations(); QLocale const & locale = Localization::getLocale(); qInfo() << diff --git a/src/Localization.cpp b/src/Localization.cpp index 92a97299c..fdedc78fd 100644 --- a/src/Localization.cpp +++ b/src/Localization.cpp @@ -42,8 +42,8 @@ namespace { QString currentLanguage = "en"; - QTranslator defaultTrans; - QTranslator btTrans; +/// QTranslator defaultTranslator; + QTranslator btTranslator; QLocale initSystemLocale() { // @@ -128,13 +128,16 @@ QString Localization::displayDateUserFormated(QDate const & date) { void Localization::setLanguage(QString twoLetterLanguage) { currentLanguage = twoLetterLanguage; - qApp->removeTranslator(&btTrans); + QCoreApplication::removeTranslator(&btTranslator); QString filename = QString("bt_%1").arg(twoLetterLanguage); QDir translations = QDir(Application::getResourceDir().canonicalPath() + "/translations_qm"); - - if (btTrans.load(filename, translations.canonicalPath())) { - qApp->installTranslator(&btTrans); + bool const succeeded = btTranslator.load(filename, translations.canonicalPath()); + qDebug() << + Q_FUNC_INFO << "Loading" << filename << "from" << translations.canonicalPath() << + (succeeded ? "succeeded" : "failed"); + if (succeeded) { + QCoreApplication::installTranslator(&btTranslator); } return; } @@ -216,32 +219,37 @@ double Localization::toDouble(QString text, char const * const caller) { } -void Localization::loadTranslations() { - if (qApp == nullptr) { - return; - } - - // Load translators. - bool succeeded = defaultTrans.load("qt_" + Localization::getLocale().name(), - QLibraryInfo::path(QLibraryInfo::TranslationsPath)); - if (!succeeded) { - qWarning() << Q_FUNC_INFO << "Error loading translations for" << Localization::getLocale().name(); - } - if (getCurrentLanguage().isEmpty()) { - setLanguage(getSystemLanguage()); - } - //btTrans.load("bt_" + getSystemLanguage()); - - // Install translators - qApp->installTranslator(&defaultTrans); - //qApp->installTranslator(btTrans); - - return; -} +///void Localization::loadTranslations() { +/// if (!qApp) { +/// // It's probably a coding error if we're called before the qApp global variable is set +/// return; +/// } +/// +/// // Load translators. +/// bool succeeded = defaultTranslator.load("qt_" + Localization::getLocale().name(), +/// QLibraryInfo::path(QLibraryInfo::TranslationsPath)); +/// if (!succeeded) { +/// qWarning() << +/// Q_FUNC_INFO << "Error loading translations for" << Localization::getLocale().name() << "from" << +/// QLibraryInfo::path(QLibraryInfo::TranslationsPath); +/// } +/// if (getCurrentLanguage().isEmpty()) { +/// setLanguage(getSystemLanguage()); +/// } +/// //btTranslator.load("bt_" + getSystemLanguage()); +/// +/// // Install translators +/// qApp->installTranslator(&defaultTranslator); +/// //qApp->installTranslator(btTranslator); +/// +/// return; +///} void Localization::loadSettings() { if (PersistentSettings::contains(PersistentSettings::Names::language)) { - Localization::setLanguage(PersistentSettings::value(PersistentSettings::Names::language,"").toString()); + Localization::setLanguage(PersistentSettings::value(PersistentSettings::Names::language, "").toString()); + } else { + Localization::setLanguage(getSystemLanguage()); } dateFormat = static_cast(PersistentSettings::value(PersistentSettings::Names::date_format, Localization::YearMonthDay).toInt()); diff --git a/src/Localization.h b/src/Localization.h index e491b3d1a..8e1be247c 100644 --- a/src/Localization.h +++ b/src/Localization.h @@ -148,10 +148,10 @@ namespace Localization { */ bool hasUnits(QString qstr); - /** - * \brief Load translation files. - */ - void loadTranslations(); +/// /** +/// * \brief Load translation files. +/// */ +/// void loadTranslations(); /** * \brief Load localization settings from persistent storage diff --git a/src/OptionDialog.cpp b/src/OptionDialog.cpp index 40352ea70..ad6117d8d 100644 --- a/src/OptionDialog.cpp +++ b/src/OptionDialog.cpp @@ -141,29 +141,32 @@ class OptionDialog::impl { // // See also CmakeLists.txt for list of translation source files (in ../translations directory) // - {"ca", QIcon(":images/flagCatalonia.svg"), "Catalan", tr("Catalan") }, - {"cs", QIcon(":images/flagCzech.svg"), "Czech", tr("Czech") }, - {"da", QIcon(":images/flagDenmark.svg"), "Danish", tr("Danish") }, - {"de", QIcon(":images/flagGermany.svg"), "German", tr("German") }, - {"el", QIcon(":images/flagGreece.svg"), "Greek", tr("Greek") }, - {"en", QIcon(":images/flagUK.svg"), "English", tr("English") }, - {"es", QIcon(":images/flagSpain.svg"), "Spanish", tr("Spanish") }, - {"et", QIcon(":images/flagEstonia.svg"), "Estonian", tr("Estonian") }, - {"eu", QIcon(":images/flagBasque.svg"), "Basque", tr("Basque") }, - {"fr", QIcon(":images/flagFrance.svg"), "French", tr("French") }, - {"gl", QIcon(":images/flagGalicia.svg"), "Galician", tr("Galician") }, - {"hu", QIcon(":images/flagHungary.svg"), "Hungarian", tr("Hungarian") }, - {"it", QIcon(":images/flagItaly.svg"), "Italian", tr("Italian") }, - {"lv", QIcon(":images/flagLatvia.svg"), "Latvian", tr("Latvian") }, - {"nb", QIcon(":images/flagNorway.svg"), "Norwegian Bokmål", tr("Norwegian Bokmål") }, - {"nl", QIcon(":images/flagNetherlands.svg"), "Dutch", tr("Dutch") }, - {"pl", QIcon(":images/flagPoland.svg"), "Polish", tr("Polish") }, - {"pt", QIcon(":images/flagBrazil.svg"), "Portuguese", tr("Portuguese") }, - {"ru", QIcon(":images/flagRussia.svg"), "Russian", tr("Russian") }, - {"sr", QIcon(":images/flagSerbia.svg"), "Serbian", tr("Serbian") }, - {"sv", QIcon(":images/flagSweden.svg"), "Swedish", tr("Swedish") }, - {"tr", QIcon(":images/flagTurkey.svg"), "Turkish", tr("Turkish") }, - {"zh", QIcon(":images/flagChina.svg"), "Chinese", tr("Chinese") } + // The order here is alphabetical by language name in English (eg "German" rather then "Deutsch"). Of course, + // one day it would be nice to sort this at run-time by the name in whatever the currently-set language is. + // + {"eu", QIcon(":images/flagBasque.svg" ), "Basque" , tr("Basque" )}, + {"ca", QIcon(":images/flagCatalonia.svg" ), "Catalan" , tr("Catalan" )}, + {"zh", QIcon(":images/flagChina.svg" ), "Chinese" , tr("Chinese" )}, + {"cs", QIcon(":images/flagCzech.svg" ), "Czech" , tr("Czech" )}, + {"da", QIcon(":images/flagDenmark.svg" ), "Danish" , tr("Danish" )}, + {"nl", QIcon(":images/flagNetherlands.svg"), "Dutch" , tr("Dutch" )}, + {"de", QIcon(":images/flagGermany.svg" ), "German" , tr("German" )}, + {"el", QIcon(":images/flagGreece.svg" ), "Greek" , tr("Greek" )}, + {"en", QIcon(":images/flagUK.svg" ), "English" , tr("English" )}, + {"et", QIcon(":images/flagEstonia.svg" ), "Estonian" , tr("Estonian" )}, + {"fr", QIcon(":images/flagFrance.svg" ), "French" , tr("French" )}, + {"gl", QIcon(":images/flagGalicia.svg" ), "Galician" , tr("Galician" )}, + {"hu", QIcon(":images/flagHungary.svg" ), "Hungarian" , tr("Hungarian" )}, + {"it", QIcon(":images/flagItaly.svg" ), "Italian" , tr("Italian" )}, + {"lv", QIcon(":images/flagLatvia.svg" ), "Latvian" , tr("Latvian" )}, + {"nb", QIcon(":images/flagNorway.svg" ), "Norwegian Bokmål", tr("Norwegian Bokmål")}, + {"pl", QIcon(":images/flagPoland.svg" ), "Polish" , tr("Polish" )}, + {"pt", QIcon(":images/flagBrazil.svg" ), "Portuguese" , tr("Portuguese" )}, + {"ru", QIcon(":images/flagRussia.svg" ), "Russian" , tr("Russian" )}, + {"sr", QIcon(":images/flagSerbia.svg" ), "Serbian" , tr("Serbian" )}, + {"es", QIcon(":images/flagSpain.svg" ), "Spanish" , tr("Spanish" )}, + {"sv", QIcon(":images/flagSweden.svg" ), "Swedish" , tr("Swedish" )}, + {"tr", QIcon(":images/flagTurkey.svg" ), "Turkish" , tr("Turkish" )}, } { // // Optimise the select file dialog to select directories diff --git a/src/model/NamedEntity.cpp b/src/model/NamedEntity.cpp index 228aa22d2..d7cdfb842 100644 --- a/src/model/NamedEntity.cpp +++ b/src/model/NamedEntity.cpp @@ -461,7 +461,9 @@ void NamedEntity::prepareForPropertyChange(BtStringConst const & propertyName) { void NamedEntity::propagatePropertyChange(BtStringConst const & propertyName, bool notify) const { if (!this->m_propagationAndSignalsEnabled) { - qDebug() << Q_FUNC_INFO << "m_propagationAndSignalsEnabled unset on" << this->metaObject()->className(); + qDebug() << + Q_FUNC_INFO << "Not propagating" << *propertyName << "change on" << this->metaObject()->className() << + "as m_propagationAndSignalsEnabled unset"; return; } From 5fa405a07a0c076197731db64315ed3608d93ef5 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 29 Oct 2024 18:47:01 +0100 Subject: [PATCH 6/7] Fix for failing unit tests, plus a bit more tidy up --- CMakeLists.txt | 75 ++++++++++++++++++++++---------------------- src/Application.cpp | 27 ++++++++++------ src/Localization.cpp | 39 ++++++----------------- src/Localization.h | 12 +++---- 4 files changed, 70 insertions(+), 83 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 32347961e..2da853c63 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -580,47 +580,48 @@ if(WIN32) get_filename_component(QT_BIN_DIR "${_qmake_executable}" DIRECTORY) message("QT_BIN_DIR = ${QT_BIN_DIR}") - # - # Per https://doc.qt.io/qt-6/windows-deployment.html, the windeployqt executable creates all the necessary folder - # tree "containing the Qt-related dependencies (libraries, QML imports, plugins, and translations) required to run - # the application from that folder". - # - # On some systems at least, looks like Qt6::windeployqt is already available in CMake (when Qt5::windeployqt) was - # not. If it is, then we can't try to set it up again as we'll get an error ("add_executable cannot create imported - # target "Qt6::windeployqt" because another target with the same name already exists"). - # - if (NOT TARGET Qt6::windeployqt) - find_program(WINDEPLOYQT_EXECUTABLE windeployqt HINTS "${QT_BIN_DIR}") - if(EXISTS ${WINDEPLOYQT_EXECUTABLE}) - # Per https://cmake.org/cmake/help/latest/command/add_executable.html, "IMPORTED executables are useful for - # convenient reference from commands like add_custom_command()". - add_executable(Qt6::windeployqt IMPORTED) - set_target_properties(Qt6::windeployqt PROPERTIES IMPORTED_LOCATION ${WINDEPLOYQT_EXECUTABLE}) - endif() - endif() +### # +### # Per https://doc.qt.io/qt-6/windows-deployment.html, the windeployqt executable creates all the necessary folder +### # tree "containing the Qt-related dependencies (libraries, QML imports, plugins, and translations) required to run +### # the application from that folder". +### # +### # On some systems at least, looks like Qt6::windeployqt is already available in CMake (when Qt5::windeployqt) was +### # not. If it is, then we can't try to set it up again as we'll get an error ("add_executable cannot create imported +### # target "Qt6::windeployqt" because another target with the same name already exists"). +### # +### if (NOT TARGET Qt6::windeployqt) +### find_program(WINDEPLOYQT_EXECUTABLE windeployqt HINTS "${QT_BIN_DIR}") +### if(EXISTS ${WINDEPLOYQT_EXECUTABLE}) +### # Per https://cmake.org/cmake/help/latest/command/add_executable.html, "IMPORTED executables are useful for +### # convenient reference from commands like add_custom_command()". +### add_executable(Qt6::windeployqt IMPORTED) +### set_target_properties(Qt6::windeployqt PROPERTIES IMPORTED_LOCATION ${WINDEPLOYQT_EXECUTABLE}) +### endif() +### endif() # International Components for Unicode file(GLOB IcuDlls "${QT_BIN_DIR}/libicu*.dll") -elseif(APPLE) - #==================================================================================================================== - #=================================================== Mac Qt Stuff =================================================== - #==================================================================================================================== - # The macdeployqt executable shipped with Qt does for Mac what windeployqt does for Windows -- see - # https://doc.qt.io/qt-6/macos-deployment.html#the-mac-deployment-tool - # - # At first glance, you might thanks that, with a few name changes, we might share all the CMake code for macdeployqt - # and windeployqt. However, as you will see below, the two programs share _only_ a top-level goal ("automate the - # process of creating a deployable [folder / applicaiton bundle] that contains [the necessary Qt dependencies]" - ie - # so that the end user does not have to install Qt to run our software). They have completely different - # implementations and command line options, so it would be unhelpful to try to treat them identically. - find_program(MACDEPLOYQT_EXECUTABLE macdeployqt HINTS "${QT_BIN_DIR}") - if(EXISTS ${MACDEPLOYQT_EXECUTABLE}) - # Per https://cmake.org/cmake/help/latest/command/add_executable.html, "IMPORTED executables are useful for - # convenient reference from commands like add_custom_command()". - add_executable(Qt6::macdeployqt IMPORTED) - set_target_properties(Qt6::macdeployqt PROPERTIES IMPORTED_LOCATION ${MACDEPLOYQT_EXECUTABLE}) - endif() +###elseif(APPLE) +### #==================================================================================================================== +### #=================================================== Mac Qt Stuff =================================================== +### #==================================================================================================================== +### +### # The macdeployqt executable shipped with Qt does for Mac what windeployqt does for Windows -- see +### # https://doc.qt.io/qt-6/macos-deployment.html#the-mac-deployment-tool +### # +### # At first glance, you might thanks that, with a few name changes, we might share all the CMake code for macdeployqt +### # and windeployqt. However, as you will see below, the two programs share _only_ a top-level goal ("automate the +### # process of creating a deployable [folder / applicaiton bundle] that contains [the necessary Qt dependencies]" - ie +### # so that the end user does not have to install Qt to run our software). They have completely different +### # implementations and command line options, so it would be unhelpful to try to treat them identically. +### find_program(MACDEPLOYQT_EXECUTABLE macdeployqt HINTS "${QT_BIN_DIR}") +### if(EXISTS ${MACDEPLOYQT_EXECUTABLE}) +### # Per https://cmake.org/cmake/help/latest/command/add_executable.html, "IMPORTED executables are useful for +### # convenient reference from commands like add_custom_command()". +### add_executable(Qt6::macdeployqt IMPORTED) +### set_target_properties(Qt6::macdeployqt PROPERTIES IMPORTED_LOCATION ${MACDEPLOYQT_EXECUTABLE}) +### endif() endif() diff --git a/src/Application.cpp b/src/Application.cpp index 28473cd8c..e25db568b 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -392,8 +392,8 @@ bool Application::initialize() { // Application::readSystemOptions(); -/// // Load in the default / remembered translations -/// Localization::loadTranslations(); + // Load in the default / remembered translations + Localization::loadTranslations(); QLocale const & locale = Localization::getLocale(); qInfo() << @@ -422,14 +422,10 @@ void Application::cleanup() { // Should I do qApp->removeTranslator() first? MainWindow::DeleteMainWindow(); + qDebug() << Q_FUNC_INFO << "Unloading database"; Database::instance().unload(); - // - // Ensure the thread we spawned to check for new versions is properly terminated - // - latestReleaseFinderThread.quit(); - latestReleaseFinderThread.wait(); - + qDebug() << Q_FUNC_INFO << "Done cleaning up"; return; } @@ -449,7 +445,7 @@ int Application::run() { splashScreen.show(); qApp->processEvents(); if (!Application::initialize()) { - cleanup(); + Application::cleanup(); return 1; } Database::instance().checkForNewDefaultData(); @@ -476,11 +472,22 @@ int Application::run() { mainWindow.initialiseAndMakeVisible(); splashScreen.finish(&mainWindow); + + // TODO: According to https://doc.qt.io/qt-6/qapplication.html#exec, there are circumstances where exec() does not + // return, so best practice is to connect clean-up code to the aboutToQuit() signal, instead of putting it in + // the application's main() function. do { ret = qApp->exec(); } while (ret == 1000); - cleanup(); + // + // Ensure the thread we spawned to check for new versions is properly terminated + // + qDebug() << Q_FUNC_INFO << "Stopping extra thread"; + latestReleaseFinderThread.quit(); + latestReleaseFinderThread.wait(); + + Application::cleanup(); qDebug() << Q_FUNC_INFO << "Cleaned up. Returning " << ret; diff --git a/src/Localization.cpp b/src/Localization.cpp index fdedc78fd..a555b7d98 100644 --- a/src/Localization.cpp +++ b/src/Localization.cpp @@ -40,9 +40,8 @@ namespace { Localization::NumericDateFormat dateFormat = Localization::YearMonthDay; - QString currentLanguage = "en"; + QString currentLanguage = ""; -/// QTranslator defaultTranslator; QTranslator btTranslator; QLocale initSystemLocale() { @@ -218,38 +217,18 @@ double Localization::toDouble(QString text, char const * const caller) { return ret; } - -///void Localization::loadTranslations() { -/// if (!qApp) { -/// // It's probably a coding error if we're called before the qApp global variable is set -/// return; -/// } -/// -/// // Load translators. -/// bool succeeded = defaultTranslator.load("qt_" + Localization::getLocale().name(), -/// QLibraryInfo::path(QLibraryInfo::TranslationsPath)); -/// if (!succeeded) { -/// qWarning() << -/// Q_FUNC_INFO << "Error loading translations for" << Localization::getLocale().name() << "from" << -/// QLibraryInfo::path(QLibraryInfo::TranslationsPath); -/// } -/// if (getCurrentLanguage().isEmpty()) { -/// setLanguage(getSystemLanguage()); -/// } -/// //btTranslator.load("bt_" + getSystemLanguage()); -/// -/// // Install translators -/// qApp->installTranslator(&defaultTranslator); -/// //qApp->installTranslator(btTranslator); -/// -/// return; -///} +void Localization::loadTranslations() { + auto const systemLanguage = getSystemLanguage(); + qDebug() << Q_FUNC_INFO << "Current language:" << currentLanguage << "; System language:" << systemLanguage; + if (currentLanguage.isEmpty()) { + Localization::setLanguage(systemLanguage); + } + return; +} void Localization::loadSettings() { if (PersistentSettings::contains(PersistentSettings::Names::language)) { Localization::setLanguage(PersistentSettings::value(PersistentSettings::Names::language, "").toString()); - } else { - Localization::setLanguage(getSystemLanguage()); } dateFormat = static_cast(PersistentSettings::value(PersistentSettings::Names::date_format, Localization::YearMonthDay).toInt()); diff --git a/src/Localization.h b/src/Localization.h index 8e1be247c..d8e0d103c 100644 --- a/src/Localization.h +++ b/src/Localization.h @@ -148,18 +148,18 @@ namespace Localization { */ bool hasUnits(QString qstr); -/// /** -/// * \brief Load translation files. -/// */ -/// void loadTranslations(); + /** + * \brief Load default translation file if necessary (usually only the first time ever the software is run). + */ + void loadTranslations(); /** - * \brief Load localization settings from persistent storage + * \brief Load localization settings from persistent storage, including the language setting from the previous run. */ void loadSettings(); /** - * \brief Save localization settings to persistent storage + * \brief Save localization settings to persistent storage, including current language setting. */ void saveSettings(); } From 307a502ae68b828d6345d9f75ce6f1c21edbafd7 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Wed, 30 Oct 2024 07:52:04 +0100 Subject: [PATCH 7/7] Unit test crash fix 2 --- CMakeLists.txt | 6 +++--- meson.build | 6 +++--- src/Localization.cpp | 41 ++++++++++++++++++++++++------------ src/database/ObjectStore.cpp | 7 +++++- src/unitTests/Testing.cpp | 4 ++-- 5 files changed, 41 insertions(+), 23 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2da853c63..23bdb5dfb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -241,9 +241,9 @@ set(RUNTIME_INSTALL_COMPONENT "Runtime") # We use different compilers on different platforms. Where possible, we want to let CMake handle the actual compiler # settings set(CMAKE_EXPORT_COMPILE_COMMANDS ON) -# We need C++20 for std::map::contains() and concepts, C++17 or later for nested namespaces and structured bindings, and -# C++11 or later for lambdas. -set(CMAKE_CXX_STANDARD 20) +# We need C++23 for std::ranges::zip_view, C++20 for std::map::contains() and concepts, C++17 or later for nested +# namespaces and structured bindings, and C++11 or later for lambdas. +set(CMAKE_CXX_STANDARD 23) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) diff --git a/meson.build b/meson.build index 64c4c1849..8d38cfe4a 100644 --- a/meson.build +++ b/meson.build @@ -163,8 +163,8 @@ # # Default options (mostly ultimately controlling compiler settings): # -# - cpp_std We need C++20 for std::map::contains(), C++17 or later for nested namespaces and structured -# bindings, and C++11 or later for lambdas. +# - cpp_std We need C++23 for std::ranges::zip_view, C++20 for std::map::contains(), C++17 or later for nested +# namespaces and structured bindings, and C++11 or later for lambdas. # # - warning_level 3 is the highest level ie warns about the most things. For gcc it translates to # -Wall -Winvalid-pch -Wextra -Wpedantic @@ -192,7 +192,7 @@ project('brewtarget', 'cpp', version: '4.0.9', license: 'GPL-3.0-or-later', meson_version: '>=0.63.0', - default_options : ['cpp_std=c++20', + default_options : ['cpp_std=c++23', 'warning_level=3', # 'prefer_static=true', See comment above for why this is commented-out for now 'buildtype=plain']) diff --git a/src/Localization.cpp b/src/Localization.cpp index a555b7d98..92a97299c 100644 --- a/src/Localization.cpp +++ b/src/Localization.cpp @@ -40,9 +40,10 @@ namespace { Localization::NumericDateFormat dateFormat = Localization::YearMonthDay; - QString currentLanguage = ""; + QString currentLanguage = "en"; - QTranslator btTranslator; + QTranslator defaultTrans; + QTranslator btTrans; QLocale initSystemLocale() { // @@ -127,16 +128,13 @@ QString Localization::displayDateUserFormated(QDate const & date) { void Localization::setLanguage(QString twoLetterLanguage) { currentLanguage = twoLetterLanguage; - QCoreApplication::removeTranslator(&btTranslator); + qApp->removeTranslator(&btTrans); QString filename = QString("bt_%1").arg(twoLetterLanguage); QDir translations = QDir(Application::getResourceDir().canonicalPath() + "/translations_qm"); - bool const succeeded = btTranslator.load(filename, translations.canonicalPath()); - qDebug() << - Q_FUNC_INFO << "Loading" << filename << "from" << translations.canonicalPath() << - (succeeded ? "succeeded" : "failed"); - if (succeeded) { - QCoreApplication::installTranslator(&btTranslator); + + if (btTrans.load(filename, translations.canonicalPath())) { + qApp->installTranslator(&btTrans); } return; } @@ -217,18 +215,33 @@ double Localization::toDouble(QString text, char const * const caller) { return ret; } + void Localization::loadTranslations() { - auto const systemLanguage = getSystemLanguage(); - qDebug() << Q_FUNC_INFO << "Current language:" << currentLanguage << "; System language:" << systemLanguage; - if (currentLanguage.isEmpty()) { - Localization::setLanguage(systemLanguage); + if (qApp == nullptr) { + return; + } + + // Load translators. + bool succeeded = defaultTrans.load("qt_" + Localization::getLocale().name(), + QLibraryInfo::path(QLibraryInfo::TranslationsPath)); + if (!succeeded) { + qWarning() << Q_FUNC_INFO << "Error loading translations for" << Localization::getLocale().name(); } + if (getCurrentLanguage().isEmpty()) { + setLanguage(getSystemLanguage()); + } + //btTrans.load("bt_" + getSystemLanguage()); + + // Install translators + qApp->installTranslator(&defaultTrans); + //qApp->installTranslator(btTrans); + return; } void Localization::loadSettings() { if (PersistentSettings::contains(PersistentSettings::Names::language)) { - Localization::setLanguage(PersistentSettings::value(PersistentSettings::Names::language, "").toString()); + Localization::setLanguage(PersistentSettings::value(PersistentSettings::Names::language,"").toString()); } dateFormat = static_cast(PersistentSettings::value(PersistentSettings::Names::date_format, Localization::YearMonthDay).toInt()); diff --git a/src/database/ObjectStore.cpp b/src/database/ObjectStore.cpp index 7a4aad0ef..2b56e3cb6 100644 --- a/src/database/ObjectStore.cpp +++ b/src/database/ObjectStore.cpp @@ -214,10 +214,15 @@ namespace { QString result; QTextStream resultAsStream{&result}; - // From Qt 6.6 we'll be able to write: + // + // In Qt5, QSqlQuery::boundValues() returned a QMap giving you the bound value names and + // values. In Qt6, QSqlQuery::boundValues() returns QVariantList of just the values. We have to wait until all + // our platforms support Qt 6.6, when QSqlQuery::boundValueNames() and QSqlQuery::boundValueName() are introduced, + // before we can write, say: // for (auto const & bvn : sqlQuery.boundValueNames()) { // resultAsStream << bvn << ": " << sqlQuery.boundValue(bvn).toString() << "\n"; // } + // for (auto const & bv : sqlQuery.boundValues()) { resultAsStream << bv.toString() << "\n"; } diff --git a/src/unitTests/Testing.cpp b/src/unitTests/Testing.cpp index 1ba434a74..f74d85b6c 100644 --- a/src/unitTests/Testing.cpp +++ b/src/unitTests/Testing.cpp @@ -468,8 +468,8 @@ void Testing::initTestCase() { // // Application::initialize() will initialise a bunch of things, including creating a default database in - // this->pimpl->m_tempDir courtesy of the call to PersistentSettings::initialise() above. If there is a problem creating the DB, - // it will return false. + // this->pimpl->m_tempDir courtesy of the call to PersistentSettings::initialise() above. If there is a problem + // creating the DB, it will return false. // QVERIFY(Application::initialize());