From b0aed198231568dccf781a4f77a117a6396a1cdf Mon Sep 17 00:00:00 2001 From: fearlessTobi Date: Mon, 17 Sep 2018 17:16:01 +0200 Subject: [PATCH 1/7] Address a bunch of review comments --- src/citra_qt/compatdb.cpp | 6 +++++- src/citra_qt/compatdb.h | 1 - src/citra_qt/configuration/configure_web.cpp | 2 +- src/citra_qt/discord_impl.h | 2 +- src/citra_qt/main.cpp | 2 +- src/core/telemetry_session.cpp | 2 +- src/core/telemetry_session.h | 2 +- src/web_service/telemetry_json.cpp | 5 +++++ src/web_service/telemetry_json.h | 5 ++--- src/web_service/web_backend.cpp | 8 ++++---- 10 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/citra_qt/compatdb.cpp b/src/citra_qt/compatdb.cpp index fb0042b3c..d6712338c 100644 --- a/src/citra_qt/compatdb.cpp +++ b/src/citra_qt/compatdb.cpp @@ -25,7 +25,11 @@ CompatDB::CompatDB(QWidget* parent) CompatDB::~CompatDB() = default; -enum class CompatDBPage { Intro = 0, Selection = 1, Final = 2 }; +enum class CompatDBPage { + Intro = 0, + Selection = 1, + Final = 2, +}; void CompatDB::Submit() { QButtonGroup* compatibility = new QButtonGroup(this); diff --git a/src/citra_qt/compatdb.h b/src/citra_qt/compatdb.h index 0a0f27cca..ca0dd11d6 100644 --- a/src/citra_qt/compatdb.h +++ b/src/citra_qt/compatdb.h @@ -21,7 +21,6 @@ public: private: std::unique_ptr ui; -private slots: void Submit(); void EnableNext(); }; diff --git a/src/citra_qt/configuration/configure_web.cpp b/src/citra_qt/configuration/configure_web.cpp index cf1daf76b..b0d45527c 100644 --- a/src/citra_qt/configuration/configure_web.cpp +++ b/src/citra_qt/configuration/configure_web.cpp @@ -25,7 +25,7 @@ ConfigureWeb::ConfigureWeb(QWidget* parent) this->setConfiguration(); } -ConfigureWeb::~ConfigureWeb() {} +ConfigureWeb::~ConfigureWeb() = default; void ConfigureWeb::setConfiguration() { ui->web_credentials_disclaimer->setWordWrap(true); diff --git a/src/citra_qt/discord_impl.h b/src/citra_qt/discord_impl.h index e714ae6d9..067cf2e3d 100644 --- a/src/citra_qt/discord_impl.h +++ b/src/citra_qt/discord_impl.h @@ -11,7 +11,7 @@ namespace DiscordRPC { class DiscordImpl : public DiscordInterface { public: DiscordImpl(); - ~DiscordImpl(); + ~DiscordImpl() override; void Pause() override; void Update() override; diff --git a/src/citra_qt/main.cpp b/src/citra_qt/main.cpp index 9bdcc06a8..ffc838ea3 100644 --- a/src/citra_qt/main.cpp +++ b/src/citra_qt/main.cpp @@ -92,7 +92,7 @@ void GMainWindow::ShowTelemetryCallout() { } UISettings::values.callout_flags |= static_cast(CalloutFlag::Telemetry); - static const QString telemetry_message = + const QString telemetry_message = tr("Anonymous " "data is collected to help improve Citra. " "

Would you like to share your usage data with us?"); diff --git a/src/core/telemetry_session.cpp b/src/core/telemetry_session.cpp index 015a837f7..94270dbfe 100644 --- a/src/core/telemetry_session.cpp +++ b/src/core/telemetry_session.cpp @@ -82,7 +82,7 @@ u64 RegenerateTelemetryId() { return new_telemetry_id; } -bool VerifyLogin(std::string username, std::string token) { +bool VerifyLogin(const std::string& username, const std::string& token) { #ifdef ENABLE_WEB_SERVICE return WebService::VerifyLogin(Settings::values.web_api_url, username, token); #else diff --git a/src/core/telemetry_session.h b/src/core/telemetry_session.h index 127b6fe5e..bcf206b1b 100644 --- a/src/core/telemetry_session.h +++ b/src/core/telemetry_session.h @@ -56,6 +56,6 @@ u64 RegenerateTelemetryId(); * @param func A function that gets exectued when the verification is finished * @returns Future with bool indicating whether the verification succeeded */ -bool VerifyLogin(std::string username, std::string token); +bool VerifyLogin(const std::string& username, const std::string& token); } // namespace Core diff --git a/src/web_service/telemetry_json.cpp b/src/web_service/telemetry_json.cpp index a0b7f9c4e..033ea1ea4 100644 --- a/src/web_service/telemetry_json.cpp +++ b/src/web_service/telemetry_json.cpp @@ -10,6 +10,11 @@ namespace WebService { +TelemetryJson::TelemetryJson(const std::string& host, const std::string& username, + const std::string& token) + : host(std::move(host)), username(std::move(username)), token(std::move(token)) {} +TelemetryJson::~TelemetryJson() = default; + template void TelemetryJson::Serialize(Telemetry::FieldType type, const std::string& name, T value) { sections[static_cast(type)][name] = value; diff --git a/src/web_service/telemetry_json.h b/src/web_service/telemetry_json.h index 4335ade59..4efef86a4 100644 --- a/src/web_service/telemetry_json.h +++ b/src/web_service/telemetry_json.h @@ -18,9 +18,8 @@ namespace WebService { */ class TelemetryJson : public Telemetry::VisitorInterface { public: - TelemetryJson(const std::string& host, const std::string& username, const std::string& token) - : host(host), username(username), token(token) {} - ~TelemetryJson() = default; + TelemetryJson(const std::string& host, const std::string& username, const std::string& token); + ~TelemetryJson(); void Visit(const Telemetry::Field& field) override; void Visit(const Telemetry::Field& field) override; diff --git a/src/web_service/web_backend.cpp b/src/web_service/web_backend.cpp index 3c5d278e8..289896165 100644 --- a/src/web_service/web_backend.cpp +++ b/src/web_service/web_backend.cpp @@ -13,12 +13,12 @@ namespace WebService { -static constexpr char API_VERSION[]{"1"}; +constexpr char API_VERSION[]{"1"}; -constexpr int HTTP_PORT = 80; -constexpr int HTTPS_PORT = 443; +constexpr u32 HTTP_PORT = 80; +constexpr u32 HTTPS_PORT = 443; -constexpr int TIMEOUT_SECONDS = 30; +constexpr u32 TIMEOUT_SECONDS = 30; Client::JWTCache Client::jwt_cache{}; From 8a87a6a72f568ce0b3e6bdafb5d5de9376f49e37 Mon Sep 17 00:00:00 2001 From: fearlessTobi Date: Mon, 17 Sep 2018 20:58:24 +0200 Subject: [PATCH 2/7] Address more review comments --- externals/CMakeLists.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/externals/CMakeLists.txt b/externals/CMakeLists.txt index aa8b0d6f2..a351ff366 100644 --- a/externals/CMakeLists.txt +++ b/externals/CMakeLists.txt @@ -71,7 +71,7 @@ endif() # DiscordRPC if (USE_DISCORD_PRESENCE) - add_subdirectory(discord-rpc) + add_subdirectory(discord-rpc EXCLUDE_FROM_ALL) target_include_directories(discord-rpc INTERFACE ./discord-rpc/include) endif() @@ -79,11 +79,11 @@ if (ENABLE_WEB_SERVICE) # LibreSSL set(LIBRESSL_SKIP_INSTALL ON CACHE BOOL "") add_definitions(-DHAVE_INET_NTOP) - add_subdirectory(libressl) + add_subdirectory(libressl EXCLUDE_FROM_ALL) target_include_directories(ssl INTERFACE ./libressl/include) # lurlparser - add_subdirectory(lurlparser) + add_subdirectory(lurlparser EXCLUDE_FROM_ALL) # httplib add_library(httplib INTERFACE) From 08793a6dae236e26c9504b2bcdbf7b96ff9e72c0 Mon Sep 17 00:00:00 2001 From: fearlessTobi Date: Wed, 19 Sep 2018 20:04:45 +0200 Subject: [PATCH 3/7] Review comments - part 3 --- externals/CMakeLists.txt | 2 +- src/citra/default_ini.h | 2 +- src/citra_qt/configuration/configure_web.cpp | 2 +- src/web_service/json.h | 18 ------------------ src/web_service/telemetry_json.h | 2 +- src/web_service/verify_login.cpp | 2 +- 6 files changed, 5 insertions(+), 23 deletions(-) delete mode 100644 src/web_service/json.h diff --git a/externals/CMakeLists.txt b/externals/CMakeLists.txt index a351ff366..641884a3f 100644 --- a/externals/CMakeLists.txt +++ b/externals/CMakeLists.txt @@ -78,9 +78,9 @@ endif() if (ENABLE_WEB_SERVICE) # LibreSSL set(LIBRESSL_SKIP_INSTALL ON CACHE BOOL "") - add_definitions(-DHAVE_INET_NTOP) add_subdirectory(libressl EXCLUDE_FROM_ALL) target_include_directories(ssl INTERFACE ./libressl/include) + target_compile_definitions(ssl PRIVATE -DHAVE_INET_NTOP) # lurlparser add_subdirectory(lurlparser EXCLUDE_FROM_ALL) diff --git a/src/citra/default_ini.h b/src/citra/default_ini.h index d05fc6bbc..730809413 100644 --- a/src/citra/default_ini.h +++ b/src/citra/default_ini.h @@ -248,7 +248,7 @@ enable_telemetry = # URL for Web API web_api_url = https://api.citra-emu.org # Username and token for Citra Web Service -# See https://services.citra-emu.org/ for more info +# See https://profile.citra-emu.org/ for more info citra_username = citra_token = )"; diff --git a/src/citra_qt/configuration/configure_web.cpp b/src/citra_qt/configuration/configure_web.cpp index b0d45527c..d74a5bafa 100644 --- a/src/citra_qt/configuration/configure_web.cpp +++ b/src/citra_qt/configuration/configure_web.cpp @@ -38,7 +38,7 @@ void ConfigureWeb::setConfiguration() { ui->web_signup_link->setOpenExternalLinks(true); ui->web_signup_link->setText( - tr("Sign up")); ui->web_token_info_link->setOpenExternalLinks(true); ui->web_token_info_link->setText( diff --git a/src/web_service/json.h b/src/web_service/json.h deleted file mode 100644 index 88b31501e..000000000 --- a/src/web_service/json.h +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2018 Citra Emulator Project -// Licensed under GPLv2 or any later version -// Refer to the license.txt file included. - -#pragma once - -// This hack is needed to support json.hpp on platforms where the C++17 stdlib -// lacks std::string_view. See https://github.com/nlohmann/json/issues/735. -// clang-format off -#if !__has_include() && __has_include() -# include -# define string_view experimental::string_view -# include -# undef string_view -#else -# include -#endif -// clang-format on diff --git a/src/web_service/telemetry_json.h b/src/web_service/telemetry_json.h index 4efef86a4..42a15b6a4 100644 --- a/src/web_service/telemetry_json.h +++ b/src/web_service/telemetry_json.h @@ -6,9 +6,9 @@ #include #include +#include #include "common/announce_multiplayer_room.h" #include "common/telemetry.h" -#include "web_service/json.h" namespace WebService { diff --git a/src/web_service/verify_login.cpp b/src/web_service/verify_login.cpp index 02e1b74f3..124aa3863 100644 --- a/src/web_service/verify_login.cpp +++ b/src/web_service/verify_login.cpp @@ -2,7 +2,7 @@ // Licensed under GPLv2 or any later version // Refer to the license.txt file included. -#include "web_service/json.h" +#include #include "web_service/verify_login.h" #include "web_service/web_backend.h" From 9901b289b689feef7c77918714137de3412340c1 Mon Sep 17 00:00:00 2001 From: fearlessTobi Date: Sat, 29 Sep 2018 02:51:28 +0200 Subject: [PATCH 4/7] Review comments -part 4 --- CMakeLists.txt | 7 ------- src/citra_qt/CMakeLists.txt | 4 ++++ src/core/CMakeLists.txt | 1 + src/web_service/web_backend.cpp | 4 ++-- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9353a5155..b91e996f9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -255,13 +255,6 @@ if (ENABLE_QT) endif() endif() -if (ENABLE_WEB_SERVICE) - add_definitions(-DENABLE_WEB_SERVICE) -endif() -if (CITRA_ENABLE_COMPATIBILITY_REPORTING) - add_definitions(-DCITRA_ENABLE_COMPATIBILITY_REPORTING) -endif() - if (ENABLE_SCRIPTING) add_definitions(-DENABLE_SCRIPTING) endif() diff --git a/src/citra_qt/CMakeLists.txt b/src/citra_qt/CMakeLists.txt index 3a61f9aa0..ea275f3d2 100644 --- a/src/citra_qt/CMakeLists.txt +++ b/src/citra_qt/CMakeLists.txt @@ -208,6 +208,10 @@ target_link_libraries(citra-qt PRIVATE audio_core common core input_common netwo target_link_libraries(citra-qt PRIVATE Boost::boost glad nihstro-headers Qt5::OpenGL Qt5::Widgets Qt5::Multimedia) target_link_libraries(citra-qt PRIVATE ${PLATFORM_LIBRARIES} Threads::Threads) +if (CITRA_ENABLE_COMPATIBILITY_REPORTING) + add_definitions(-DCITRA_ENABLE_COMPATIBILITY_REPORTING) +endif() + if (USE_DISCORD_PRESENCE) target_sources(citra-qt PUBLIC discord_impl.cpp diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt index 1498ee343..e99a6ac79 100644 --- a/src/core/CMakeLists.txt +++ b/src/core/CMakeLists.txt @@ -445,6 +445,7 @@ create_target_directory_groups(core) target_link_libraries(core PUBLIC common PRIVATE audio_core network video_core) target_link_libraries(core PUBLIC Boost::boost PRIVATE cryptopp fmt open_source_archives) if (ENABLE_WEB_SERVICE) + add_definitions(-DENABLE_WEB_SERVICE) target_link_libraries(core PUBLIC json-headers web_service) endif() diff --git a/src/web_service/web_backend.cpp b/src/web_service/web_backend.cpp index 289896165..c1f320738 100644 --- a/src/web_service/web_backend.cpp +++ b/src/web_service/web_backend.cpp @@ -13,7 +13,7 @@ namespace WebService { -constexpr char API_VERSION[]{"1"}; +constexpr std::array API_VERSION{'1'}; constexpr u32 HTTP_PORT = 80; constexpr u32 HTTPS_PORT = 443; @@ -70,7 +70,7 @@ Common::WebResult Client::GenericJson(const std::string& method, const std::stri }; } - params.emplace(std::string("api-version"), std::string(API_VERSION)); + params.emplace(std::string("api-version"), std::string(API_VERSION.begin(), API_VERSION.end())); if (method != "GET") { params.emplace(std::string("Content-Type"), std::string("application/json")); }; From 7daac9686296dfd58b3b94318480cbcf3be03fba Mon Sep 17 00:00:00 2001 From: fearlessTobi Date: Tue, 2 Oct 2018 16:04:10 +0200 Subject: [PATCH 5/7] Review comments - part 5 --- src/common/detached_tasks.h | 1 + src/web_service/CMakeLists.txt | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/common/detached_tasks.h b/src/common/detached_tasks.h index eae27788d..5dd8fc27b 100644 --- a/src/common/detached_tasks.h +++ b/src/common/detached_tasks.h @@ -3,6 +3,7 @@ // Refer to the license.txt file included. #pragma once + #include #include diff --git a/src/web_service/CMakeLists.txt b/src/web_service/CMakeLists.txt index fc311cb81..088509883 100644 --- a/src/web_service/CMakeLists.txt +++ b/src/web_service/CMakeLists.txt @@ -14,5 +14,5 @@ create_target_directory_groups(web_service) get_directory_property(OPENSSL_LIBS DIRECTORY ${CMAKE_SOURCE_DIR}/externals/libressl DEFINITION OPENSSL_LIBS) -add_definitions(-DCPPHTTPLIB_OPENSSL_SUPPORT) -target_link_libraries(web_service PUBLIC common json-headers ${OPENSSL_LIBS} httplib lurlparser) +target_compile_definitions(web_service PUBLIC -DCPPHTTPLIB_OPENSSL_SUPPORT) +target_link_libraries(web_service PRIVATE common json-headers ${OPENSSL_LIBS} httplib lurlparser) From 57d68bb541a57e6940d020fdde6c8009c190bd20 Mon Sep 17 00:00:00 2001 From: fearlessTobi Date: Sat, 13 Oct 2018 13:33:04 +0200 Subject: [PATCH 6/7] Address review comments --- externals/CMakeLists.txt | 2 +- src/web_service/announce_room_json.cpp | 2 +- src/web_service/web_backend.cpp | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/externals/CMakeLists.txt b/externals/CMakeLists.txt index 641884a3f..13b347d89 100644 --- a/externals/CMakeLists.txt +++ b/externals/CMakeLists.txt @@ -80,7 +80,7 @@ if (ENABLE_WEB_SERVICE) set(LIBRESSL_SKIP_INSTALL ON CACHE BOOL "") add_subdirectory(libressl EXCLUDE_FROM_ALL) target_include_directories(ssl INTERFACE ./libressl/include) - target_compile_definitions(ssl PRIVATE -DHAVE_INET_NTOP) + target_compile_definitions(ssl PRIVATE -DHAVE_INET_NTOP) # lurlparser add_subdirectory(lurlparser EXCLUDE_FROM_ALL) diff --git a/src/web_service/announce_room_json.cpp b/src/web_service/announce_room_json.cpp index 6a6512491..c0d543e73 100644 --- a/src/web_service/announce_room_json.cpp +++ b/src/web_service/announce_room_json.cpp @@ -3,10 +3,10 @@ // Refer to the license.txt file included. #include +#include #include "common/detached_tasks.h" #include "common/logging/log.h" #include "web_service/announce_room_json.h" -#include "web_service/json.h" #include "web_service/web_backend.h" namespace AnnounceMultiplayerRoom { diff --git a/src/web_service/web_backend.cpp b/src/web_service/web_backend.cpp index c1f320738..fc702a378 100644 --- a/src/web_service/web_backend.cpp +++ b/src/web_service/web_backend.cpp @@ -15,10 +15,10 @@ namespace WebService { constexpr std::array API_VERSION{'1'}; -constexpr u32 HTTP_PORT = 80; -constexpr u32 HTTPS_PORT = 443; +constexpr int HTTP_PORT = 80; +constexpr int HTTPS_PORT = 443; -constexpr u32 TIMEOUT_SECONDS = 30; +constexpr std::size_t TIMEOUT_SECONDS = 30; Client::JWTCache Client::jwt_cache{}; From 65ec8de31e9b0fd1d87024b9e8fbf8cdc50cf0b8 Mon Sep 17 00:00:00 2001 From: Weiyi Wang Date: Sat, 20 Oct 2018 10:47:17 -0400 Subject: [PATCH 7/7] web_service: hide dependencies to private --- src/core/CMakeLists.txt | 4 ++-- src/web_service/CMakeLists.txt | 2 +- src/web_service/web_backend.cpp | 4 +++- src/web_service/web_backend.h | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt index e99a6ac79..2b9838a84 100644 --- a/src/core/CMakeLists.txt +++ b/src/core/CMakeLists.txt @@ -445,8 +445,8 @@ create_target_directory_groups(core) target_link_libraries(core PUBLIC common PRIVATE audio_core network video_core) target_link_libraries(core PUBLIC Boost::boost PRIVATE cryptopp fmt open_source_archives) if (ENABLE_WEB_SERVICE) - add_definitions(-DENABLE_WEB_SERVICE) - target_link_libraries(core PUBLIC json-headers web_service) + target_compile_definitions(core PRIVATE -DENABLE_WEB_SERVICE) + target_link_libraries(core PRIVATE json-headers web_service) endif() if (ARCHITECTURE_x86_64) diff --git a/src/web_service/CMakeLists.txt b/src/web_service/CMakeLists.txt index 088509883..993ba9ac6 100644 --- a/src/web_service/CMakeLists.txt +++ b/src/web_service/CMakeLists.txt @@ -14,5 +14,5 @@ create_target_directory_groups(web_service) get_directory_property(OPENSSL_LIBS DIRECTORY ${CMAKE_SOURCE_DIR}/externals/libressl DEFINITION OPENSSL_LIBS) -target_compile_definitions(web_service PUBLIC -DCPPHTTPLIB_OPENSSL_SUPPORT) +target_compile_definitions(web_service PRIVATE -DCPPHTTPLIB_OPENSSL_SUPPORT) target_link_libraries(web_service PRIVATE common json-headers ${OPENSSL_LIBS} httplib lurlparser) diff --git a/src/web_service/web_backend.cpp b/src/web_service/web_backend.cpp index fc702a378..9b824e47d 100644 --- a/src/web_service/web_backend.cpp +++ b/src/web_service/web_backend.cpp @@ -6,9 +6,9 @@ #include #include #include +#include #include "common/announce_multiplayer_room.h" #include "common/logging/log.h" -#include "core/settings.h" #include "web_service/web_backend.h" namespace WebService { @@ -30,6 +30,8 @@ Client::Client(const std::string& host, const std::string& username, const std:: } } +Client::~Client() = default; + Common::WebResult Client::GenericJson(const std::string& method, const std::string& path, const std::string& data, const std::string& jwt, const std::string& username, const std::string& token) { diff --git a/src/web_service/web_backend.h b/src/web_service/web_backend.h index 955b91c7a..6cdf35b48 100644 --- a/src/web_service/web_backend.h +++ b/src/web_service/web_backend.h @@ -8,7 +8,6 @@ #include #include #include -#include #include "common/announce_multiplayer_room.h" #include "common/common_types.h" @@ -21,6 +20,7 @@ namespace WebService { class Client { public: Client(const std::string& host, const std::string& username, const std::string& token); + ~Client(); /** * Posts JSON to the specified path.