refactor: use new tr_variant API in rpcimpl (#6456)

* refactor: tr_rpc_request_exec_json takes a std::function callback

* refactor: tr_rpc_request_exec_json callback takes a tr_variant&&

* refactor: use new tr_variant API in rpcimpl.cc

* refactor: tr_rpc_request_exec_json() now takes a const& to the request

* fixup! refactor: use new tr_variant API in rpcimpl.cc

* chore: rename function to tr_rpc_request_exec()

* chore: remove unused DetailsDialog::Impl::build_torrent_ids_variant_list()

* refactor: minor copyediting in rpcimpl.cc getTorrents()

* refactor: split handler methods between sync, async

* refactor: remove unused args_out param from AsyncHandlers

* chore: fix new readability-inconsistent-declaration-parameter-name warning

gtk/Application.cc

@@ -1451,7 +1451,7 @@ bool Application::Impl::call_rpc_for_selected_torrents(std::string const& method
 
     if (tr_variantListSize(ids) != 0)
     {
-        tr_rpc_request_exec_json(session, &top, nullptr, nullptr);
+        tr_rpc_request_exec(session, top, {});
         invoked = true;
     }
 
@@ -1473,7 +1473,7 @@ void Application::Impl::start_all_torrents()
 
     tr_variantInitDict(&request, 1);
     tr_variantDictAddStrView(&request, TR_KEY_method, "torrent-start"sv);
-    tr_rpc_request_exec_json(session, &request, nullptr, nullptr);
+    tr_rpc_request_exec(session, request, {});
 }
 
 void Application::Impl::pause_all_torrents()
@@ -1483,7 +1483,7 @@ void Application::Impl::pause_all_torrents()
 
     tr_variantInitDict(&request, 1);
     tr_variantDictAddStrView(&request, TR_KEY_method, "torrent-stop"sv);
-    tr_rpc_request_exec_json(session, &request, nullptr, nullptr);
+    tr_rpc_request_exec(session, request, {});
 }
 
 void Application::Impl::copy_magnet_link_to_clipboard(Glib::RefPtr<Torrent> const& torrent) const

gtk/DetailsDialog.cc

@@ -53,6 +53,7 @@
 #include <array>
 #include <cstddef>
 #include <cstdlib> // abort()
+#include <iterator>
 #include <limits>
 #include <memory>
 #include <numeric>
@@ -446,7 +447,7 @@ void DetailsDialog::Impl::torrent_set_bool(tr_quark key, bool value)
         tr_variantListAddInt(ids, id);
     }
 
-    core_->exec(&top);
+    core_->exec(top);
 }
 
 void DetailsDialog::Impl::torrent_set_int(tr_quark key, int value)
@@ -464,7 +465,7 @@ void DetailsDialog::Impl::torrent_set_int(tr_quark key, int value)
         tr_variantListAddInt(ids, id);
     }
 
-    core_->exec(&top);
+    core_->exec(top);
 }
 
 void DetailsDialog::Impl::torrent_set_real(tr_quark key, double value)
@@ -482,7 +483,7 @@ void DetailsDialog::Impl::torrent_set_real(tr_quark key, double value)
         tr_variantListAddInt(ids, id);
     }
 
-    core_->exec(&top);
+    core_->exec(top);
 }
 
 void DetailsDialog::Impl::options_page_init(Glib::RefPtr<Gtk::Builder> const& /*builder*/)
@@ -2373,7 +2374,7 @@ void AddTrackerDialog::on_response(int response)
                 auto* const trackers = tr_variantDictAddList(args, TR_KEY_trackerAdd, 1);
                 tr_variantListAddStr(trackers, url.raw());
 
-                core_->exec(&top);
+                core_->exec(top);
                 parent_.refresh();
             }
             else
@@ -2420,7 +2421,7 @@ void DetailsDialog::Impl::on_tracker_list_remove_button_clicked()
         auto* const trackers = tr_variantDictAddList(args, TR_KEY_trackerRemove, 1);
         tr_variantListAddInt(trackers, tracker_id);
 
-        core_->exec(&top);
+        core_->exec(top);
         refresh();
     }
 }

gtk/Session.cc

@@ -83,7 +83,7 @@ public:
     void add_torrent(Glib::RefPtr<Torrent> const& torrent, bool do_notify);
     bool add_from_url(Glib::ustring const& url);
 
-    void send_rpc_request(tr_variant const* request, int64_t tag, std::function<void(tr_variant&)> const& response_func);
+    void send_rpc_request(tr_variant const& request, int64_t tag, std::function<void(tr_variant&)> const& response_func);
 
     void commit_prefs_change(tr_quark key);
 
@@ -967,7 +967,7 @@ void Session::start_now(tr_torrent_id_t id)
     auto* args = tr_variantDictAddDict(&top, TR_KEY_arguments, 1);
     auto* ids = tr_variantDictAddList(args, TR_KEY_ids, 1);
     tr_variantListAddInt(ids, id);
-    exec(&top);
+    exec(top);
 }
 
 void Session::Impl::update()
@@ -1186,19 +1186,16 @@ bool core_read_rpc_response_idle(tr_variant& response)
     return false;
 }
 
-void core_read_rpc_response(tr_session* /*session*/, tr_variant* response, gpointer /*user_data*/)
+void core_read_rpc_response(tr_session* /*session*/, tr_variant&& response)
 {
-    auto owned_response = std::make_shared<tr_variant>();
+    auto owned_response = std::make_shared<tr_variant>(std::move(response));
-    *owned_response.get() = false;
-    std::swap(*owned_response, *response);
-
     Glib::signal_idle().connect([owned_response]() mutable { return core_read_rpc_response_idle(*owned_response); });
 }
 
 } // namespace
 
 void Session::Impl::send_rpc_request(
-    tr_variant const* request,
+    tr_variant const& request,
     int64_t tag,
     std::function<void(tr_variant&)> const& response_func)
 {
@@ -1216,7 +1213,7 @@ void Session::Impl::send_rpc_request(
         gtr_message(fmt::format("request: [{}]", tr_variantToStr(request, TR_VARIANT_FMT_JSON_LEAN)));
 #endif
 
-        tr_rpc_request_exec_json(session_, request, core_read_rpc_response, nullptr);
+        tr_rpc_request_exec(session_, request, core_read_rpc_response);
     }
 }
 
@@ -1234,7 +1231,7 @@ void Session::port_test()
     tr_variantDictAddStrView(&request, TR_KEY_method, "port-test");
     tr_variantDictAddInt(&request, TR_KEY_tag, tag);
     impl_->send_rpc_request(
-        &request,
+        request,
         tag,
         [this](auto& response)
         {
@@ -1265,7 +1262,7 @@ void Session::blocklist_update()
     tr_variantDictAddStrView(&request, TR_KEY_method, "blocklist-update");
     tr_variantDictAddInt(&request, TR_KEY_tag, tag);
     impl_->send_rpc_request(
-        &request,
+        request,
         tag,
         [this](auto& response)
         {
@@ -1291,7 +1288,7 @@ void Session::blocklist_update()
 ****
 ***/
 
-void Session::exec(tr_variant const* request)
+void Session::exec(tr_variant const& request)
 {
     auto const tag = nextTag;
     ++nextTag;

gtk/Session.h

@@ -131,7 +131,7 @@ public:
 
     void blocklist_update();
 
-    void exec(tr_variant const* request);
+    void exec(tr_variant const& request);
 
     void open_folder(tr_torrent_id_t torrent_id) const;
 

libtransmission/rpc-server.cc

@@ -340,29 +340,21 @@ void handle_web_client(struct evhttp_request* req, tr_rpc_server const* server)
     }
 }
 
-struct rpc_response_data
-{
-    struct evhttp_request* req;
-    tr_rpc_server* server;
-};
-
-void rpc_response_func(tr_session* /*session*/, tr_variant* content, void* user_data)
-{
-    auto* data = static_cast<struct rpc_response_data*>(user_data);
-
-    auto* const response = make_response(data->req, data->server, tr_variant_serde::json().compact().to_string(*content));
-    evhttp_add_header(data->req->output_headers, "Content-Type", "application/json; charset=UTF-8");
-    evhttp_send_reply(data->req, HTTP_OK, "OK", response);
-    evbuffer_free(response);
-
-    delete data;
-}
-
 void handle_rpc_from_json(struct evhttp_request* req, tr_rpc_server* server, std::string_view json)
 {
-    auto otop = tr_variant_serde::json().inplace().parse(json);
+    if (auto otop = tr_variant_serde::json().inplace().parse(json); otop)
-
+    {
-    tr_rpc_request_exec_json(server->session, otop ? &*otop : nullptr, rpc_response_func, new rpc_response_data{ req, server });
+        tr_rpc_request_exec(
+            server->session,
+            *otop,
+            [req, server](tr_session* /*session*/, tr_variant&& content)
+            {
+                auto* const response = make_response(req, server, tr_variant_serde::json().compact().to_string(content));
+                evhttp_add_header(req->output_headers, "Content-Type", "application/json; charset=UTF-8");
+                evhttp_send_reply(req, HTTP_OK, "OK", response);
+                evbuffer_free(response);
+            });
+    }
 }
 
 void handle_rpc(struct evhttp_request* req, tr_rpc_server* server)

libtransmission/rpcimpl.h

@@ -5,18 +5,14 @@
 
 #pragma once
 
+#include <functional>
 #include <string_view>
 
 struct tr_session;
 struct tr_variant;
 
-using tr_rpc_response_func = void (*)(tr_session* session, tr_variant* response, void* user_data);
+using tr_rpc_response_func = std::function<void(tr_session* session, tr_variant&& response)>;
 
-/* https://www.json.org/ */
+void tr_rpc_request_exec(tr_session* session, tr_variant const& request, tr_rpc_response_func&& callback = {});
-void tr_rpc_request_exec_json(
-    tr_session* session,
-    tr_variant const* request,
-    tr_rpc_response_func callback,
-    void* callback_user_data);
 
 tr_variant tr_rpc_parse_list_str(std::string_view str);

qt/RpcClient.cc

@@ -121,7 +121,7 @@ int64_t RpcClient::getNextTag()
     return next_tag_++;
 }
 
-void RpcClient::sendNetworkRequest(TrVariantPtr json, QFutureInterface<RpcResponse> const& promise)
+void RpcClient::sendNetworkRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise)
 {
     if (!request_)
     {
@@ -139,9 +139,9 @@ void RpcClient::sendNetworkRequest(TrVariantPtr json, QFutureInterface<RpcRespon
         request_ = request;
     }
 
-    auto const json_data = QByteArray::fromStdString(tr_variant_serde::json().compact().to_string(*json));
+    auto const json_data = QByteArray::fromStdString(tr_variant_serde::json().compact().to_string(*req));
     QNetworkReply* reply = networkAccessManager()->post(*request_, json_data);
-    reply->setProperty(RequestDataPropertyKey, QVariant::fromValue(json));
+    reply->setProperty(RequestDataPropertyKey, QVariant::fromValue(req));
     reply->setProperty(RequestFutureinterfacePropertyKey, QVariant::fromValue(promise));
 
     connect(reply, &QNetworkReply::downloadProgress, this, &RpcClient::dataReadProgress);
@@ -162,15 +162,31 @@ void RpcClient::sendNetworkRequest(TrVariantPtr json, QFutureInterface<RpcRespon
     }
 }
 
-void RpcClient::sendLocalRequest(TrVariantPtr json, QFutureInterface<RpcResponse> const& promise, int64_t tag)
+void RpcClient::sendLocalRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise, int64_t tag)
 {
     if (verbose_)
     {
-        fmt::print("{:s}:{:d} sending req:\n{:s}\n", __FILE__, __LINE__, tr_variant_serde::json().to_string(*json));
+        fmt::print("{:s}:{:d} sending req:\n{:s}\n", __FILE__, __LINE__, tr_variant_serde::json().to_string(*req));
     }
 
     local_requests_.try_emplace(tag, promise);
-    tr_rpc_request_exec_json(session_, json.get(), localSessionCallback, this);
+    tr_rpc_request_exec(
+        session_,
+        *req,
+        [this](tr_session* /*sesson*/, tr_variant&& response)
+        {
+            if (verbose_)
+            {
+                fmt::print("{:s}:{:d} got response:\n{:s}\n", __FILE__, __LINE__, tr_variant_serde::json().to_string(response));
+            }
+
+            TrVariantPtr const resp = createVariant();
+            *resp = std::move(response);
+
+            // this callback is invoked in the libtransmission thread, so we don't want
+            // to process the response here... let's push it over to the Qt thread.
+            QMetaObject::invokeMethod(this, "localRequestFinished", Qt::QueuedConnection, Q_ARG(TrVariantPtr, resp));
+        });
 }
 
 RpcResponseFuture RpcClient::sendRequest(TrVariantPtr json)
@@ -210,27 +226,6 @@ QNetworkAccessManager* RpcClient::networkAccessManager()
     return nam_;
 }
 
-void RpcClient::localSessionCallback(tr_session* s, tr_variant* response, void* vself) noexcept
-{
-    Q_UNUSED(s)
-
-    auto* self = static_cast<RpcClient*>(vself);
-
-    if (self->verbose_)
-    {
-        fmt::print("{:s}:{:d} got response:\n{:s}\n", __FILE__, __LINE__, tr_variant_serde::json().to_string(*response));
-    }
-
-    TrVariantPtr const json = createVariant();
-    std::swap(*json, *response);
-
-    variantInit(response, false);
-
-    // this callback is invoked in the libtransmission thread, so we don't want
-    // to process the response here... let's push it over to the Qt thread.
-    QMetaObject::invokeMethod(self, "localRequestFinished", Qt::QueuedConnection, Q_ARG(TrVariantPtr, json));
-}
-
 void RpcClient::networkRequestFinished(QNetworkReply* reply)
 {
     reply->deleteLater();

qt/RpcClient.h

@@ -80,13 +80,11 @@ private:
     QNetworkAccessManager* networkAccessManager();
     int64_t getNextTag();
 
-    void sendNetworkRequest(TrVariantPtr json, QFutureInterface<RpcResponse> const& promise);
+    void sendNetworkRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise);
-    void sendLocalRequest(TrVariantPtr json, QFutureInterface<RpcResponse> const& promise, int64_t tag);
+    void sendLocalRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise, int64_t tag);
     [[nodiscard]] int64_t parseResponseTag(tr_variant& response) const;
     [[nodiscard]] RpcResponse parseResponseData(tr_variant& response) const;
 
-    static void localSessionCallback(tr_session* s, tr_variant* response, void* vself) noexcept;
-
     std::optional<QNetworkRequest> request_;
 
     tr_session* session_ = {};

tests/libtransmission/rpc-test.cc

@@ -71,19 +71,17 @@ TEST_F(RpcTest, list)
 
 TEST_F(RpcTest, sessionGet)
 {
-    auto const rpc_response_func = [](tr_session* /*session*/, tr_variant* response, void* setme) noexcept
-    {
-        std::swap(*static_cast<tr_variant*>(setme), *response);
-    };
-
     auto* tor = zeroTorrentInit(ZeroTorrentState::NoFiles);
     EXPECT_NE(nullptr, tor);
 
-    tr_variant request;
+    auto request = tr_variant{};
     tr_variantInitDict(&request, 1);
     tr_variantDictAddStrView(&request, TR_KEY_method, "session-get");
-    tr_variant response;
+    auto response = tr_variant{};
-    tr_rpc_request_exec_json(session_, &request, rpc_response_func, &response);
+    tr_rpc_request_exec(
+        session_,
+        request,
+        [&response](tr_session* /*session*/, tr_variant&& resp) { response = std::move(resp); });
 
     EXPECT_TRUE(response.holds_alternative<tr_variant::Map>());
     tr_variant* args = nullptr;
@@ -186,11 +184,6 @@ TEST_F(RpcTest, sessionGet)
 
 TEST_F(RpcTest, torrentGet)
 {
-    auto const rpc_response_func = [](tr_session* /*session*/, tr_variant* response, void* setme) noexcept
-    {
-        std::swap(*static_cast<tr_variant*>(setme), *response);
-    };
-
     auto* tor = zeroTorrentInit(ZeroTorrentState::NoFiles);
     EXPECT_NE(nullptr, tor);
 
@@ -203,8 +196,11 @@ TEST_F(RpcTest, torrentGet)
     tr_variant* fields = tr_variantDictAddList(args_in, TR_KEY_fields, 1);
     tr_variantListAddStrView(fields, tr_quark_get_string_view(TR_KEY_id));
 
-    tr_variant response;
+    auto response = tr_variant{};
-    tr_rpc_request_exec_json(session_, &request, rpc_response_func, &response);
+    tr_rpc_request_exec(
+        session_,
+        request,
+        [&response](tr_session* /*session*/, tr_variant&& resp) { response = std::move(resp); });
 
     EXPECT_TRUE(response.holds_alternative<tr_variant::Map>());
     tr_variant* args = nullptr;
@@ -215,7 +211,7 @@ TEST_F(RpcTest, torrentGet)
     EXPECT_EQ(1UL, tr_variantListSize(torrents));
 
     tr_variant* first_torrent = tr_variantListChild(torrents, 0);
-
+    EXPECT_TRUE(first_torrent != nullptr);
     EXPECT_TRUE(first_torrent->holds_alternative<tr_variant::Map>());
     int64_t first_torrent_id = 0;
     EXPECT_TRUE(tr_variantDictFindInt(first_torrent, TR_KEY_id, &first_torrent_id));