refactor: tr_torrentSetLocation() takes a std::function callback

gtk/FileList.cc

@@ -812,13 +812,6 @@ bool FileList::Impl::onViewButtonPressed(guint button, TrGdkModifierType state,
     return handled;
 }
 
-struct rename_data
-{
-    Glib::ustring newname;
-    Glib::ustring path_string;
-    gpointer impl = nullptr;
-};
-
 void FileList::Impl::on_rename_done(Glib::ustring const& path_string, Glib::ustring const& newname, int error)
 {
     rename_done_tags_.push(
@@ -901,22 +894,12 @@ void FileList::Impl::cell_edited_callback(Glib::ustring const& path_string, Glib
         oldpath.insert(0, 1, G_DIR_SEPARATOR);
     }
 
-    /* do the renaming */
+    // do the renaming
-    auto rename_data = std::make_unique<struct rename_data>();
-    rename_data->newname = newname;
-    rename_data->impl = this;
-    rename_data->path_string = path_string;
     tr_torrentRenamePath(
         tor,
         oldpath.raw(),
         newname.raw(),
-        static_cast<tr_torrent_rename_done_func>(
+        [this, newname, path_string](int const error) { on_rename_done(path_string, newname, error); });
-            [](tr_torrent* /*tor*/, char const* /*oldpath*/, char const* /*newname*/, int error, gpointer data)
-            {
-                auto const data_grave = std::unique_ptr<struct rename_data>(static_cast<struct rename_data*>(data));
-                static_cast<Impl*>(data_grave->impl)->on_rename_done(data_grave->path_string, data_grave->newname, error);
-            }),
-        rename_data.release());
 }
 
 FileList::FileList(

libtransmission/rpcimpl.cc

@@ -1431,26 +1431,6 @@ namespace make_torrent_field_helpers
 
 // ---
 
-void torrentRenamePathDone(tr_torrent* tor, char const* oldpath, char const* newname, int error, void* user_data)
-{
-    using namespace JsonRpc;
-
-    auto* const data = static_cast<struct tr_rpc_idle_data*>(user_data);
-
-    data->args_out.try_emplace(TR_KEY_id, tor->id());
-    data->args_out.try_emplace(TR_KEY_path, oldpath);
-    data->args_out.try_emplace(TR_KEY_name, newname);
-
-    if (error == 0)
-    {
-        tr_rpc_idle_done(data, Error::SUCCESS, {});
-    }
-    else
-    {
-        tr_rpc_idle_done(data, Error::SYSTEM_ERROR, tr_strerror(error));
-    }
-}
-
 void torrentRenamePath(tr_session* session, tr_variant::Map const& args_in, struct tr_rpc_idle_data* idle_data)
 {
     using namespace JsonRpc;
@@ -1462,14 +1442,28 @@ void torrentRenamePath(tr_session* session, tr_variant::Map const& args_in, stru
         return;
     }
 
+    auto* const tor = torrents[0];
+    idle_data->args_out.try_emplace(TR_KEY_id, tor->id());
+
     auto const oldpath = args_in.value_if<std::string_view>(TR_KEY_path).value_or(""sv);
     auto const newname = args_in.value_if<std::string_view>(TR_KEY_name).value_or(""sv);
-    torrents[0]->rename_path(
+    idle_data->args_out.try_emplace(TR_KEY_path, oldpath);
+    idle_data->args_out.try_emplace(TR_KEY_name, newname);
+
+    tor->rename_path(
         oldpath,
         newname,
-        [](tr_torrent* tor, char const* old_path, char const* new_name, int error, void* user_data)
+        [idle_data](int const error)
-        { torrentRenamePathDone(tor, old_path, new_name, error, user_data); },
+        {
-        idle_data);
+            if (error == 0)
+            {
+                tr_rpc_idle_done(idle_data, JsonRpc::Error::SUCCESS, {});
+            }
+            else
+            {
+                tr_rpc_idle_done(idle_data, JsonRpc::Error::SYSTEM_ERROR, tr_strerror(error));
+            }
+        });
 }
 
 // ---

libtransmission/torrent.cc

@@ -2471,8 +2471,7 @@ void renameTorrentFileString(tr_torrent* tor, std::string_view oldpath, std::str
 void tr_torrent::rename_path_in_session_thread(
     std::string_view const oldpath,
     std::string_view const newname,
-    tr_torrent_rename_done_func const& callback,
+    std::function<void(int)> const& on_rename_done)
-    void* const callback_user_data)
 {
     using namespace rename_helpers;
 
@@ -2511,33 +2510,26 @@ void tr_torrent::rename_path_in_session_thread(
 
     mark_changed();
 
-    if (callback != nullptr)
+    if (on_rename_done)
     {
-        auto const szold = tr_pathbuf{ oldpath };
+        on_rename_done(error);
-        auto const sznew = tr_pathbuf{ newname };
-        callback(this, szold.c_str(), sznew.c_str(), error, callback_user_data);
     }
 }
 
-void tr_torrent::rename_path(
+void tr_torrent::rename_path(std::string_view oldpath, std::string_view newname, std::function<void(int)> on_rename_done)
-    std::string_view oldpath,
-    std::string_view newname,
-    tr_torrent_rename_done_func&& callback,
-    void* callback_user_data)
 {
     this->session->run_in_session_thread(
-        [this, oldpath = std::string(oldpath), newname = std::string(newname), cb = std::move(callback), callback_user_data]()
+        [this, oldpath = std::string(oldpath), newname = std::string(newname), cb = std::move(on_rename_done)]()
-        { rename_path_in_session_thread(oldpath, newname, std::move(cb), callback_user_data); });
+        { rename_path_in_session_thread(oldpath, newname, std::move(cb)); });
 }
 
 void tr_torrentRenamePath(
     tr_torrent* tor,
     std::string_view const oldpath,
     std::string_view const newname,
-    tr_torrent_rename_done_func callback,
+    std::function<void(int)> on_rename_done)
-    void* callback_user_data)
 {
-    tor->rename_path(oldpath, newname, std::move(callback), callback_user_data);
+    tor->rename_path(oldpath, newname, std::move(on_rename_done));
 }
 
 // ---

libtransmission/torrent.h

@@ -180,11 +180,7 @@ struct tr_torrent
 
     void set_location(std::string_view location, bool move_from_old_path, int volatile* setme_state);
 
-    void rename_path(
+    void rename_path(std::string_view oldpath, std::string_view newname, std::function<void(int)> on_rename_done);
-        std::string_view oldpath,
-        std::string_view newname,
-        tr_torrent_rename_done_func&& callback,
-        void* callback_user_data);
 
     // these functions should become private when possible,
     // but more refactoring is needed before that can happen
@@ -1332,8 +1328,7 @@ private:
     void rename_path_in_session_thread(
         std::string_view oldpath,
         std::string_view newname,
-        tr_torrent_rename_done_func const& callback,
+        std::function<void(int)> const& on_rename_done);
-        void* callback_user_data);
 
     void start_in_session_thread();
 

libtransmission/transmission.h

@@ -828,9 +828,6 @@ void tr_torrentStart(tr_torrent* torrent);
 /** @brief Stop (pause) a torrent */
 void tr_torrentStop(tr_torrent* torrent);
 
-using tr_torrent_rename_done_func = std::function<
-    void(tr_torrent* torrent, char const* oldpath, char const* newname, int error, void* user_data)>;
-
 /**
  * @brief Rename a file or directory in a torrent.
  *
@@ -877,8 +874,7 @@ void tr_torrentRenamePath(
     tr_torrent* tor,
     std::string_view oldpath,
     std::string_view newname,
-    tr_torrent_rename_done_func callback,
+    std::function<void(int error)> on_rename_done = {});
-    void* callback_user_data);
 
 enum : uint8_t
 {

macosx/FileRenameSheetController.mm

@@ -106,10 +106,18 @@ typedef void (^CompletionBlock)(BOOL);
 
 - (IBAction)rename:(id)sender
 {
-    void (^completionHandler)(BOOL) = ^(BOOL didRename) {
+    __weak FileRenameSheetController* weakSelf = self;
+    auto completionHandler = [weakSelf](bool didRename)
+    {
+        FileRenameSheetController* strongSelf = weakSelf;
+        if (strongSelf == nil)
+        {
+            return;
+        }
+
         if (didRename)
         {
-            [NSApp endSheet:self.window returnCode:NSModalResponseOK];
+            [NSApp endSheet:strongSelf.window returnCode:NSModalResponseOK];
         }
         else
         {

macosx/Torrent.h

@@ -5,6 +5,8 @@
 #import <Foundation/Foundation.h>
 #import <Quartz/Quartz.h>
 
+#include <functional>
+
 #include <libtransmission/transmission.h>
 
 @class FileListNode;
@@ -117,10 +119,10 @@ extern NSString* const kTorrentDidChangeGroupNotification;
 @property(nonatomic, readonly) NSString* lastKnownDataLocation;
 - (NSString*)fileLocation:(FileListNode*)node;
 
-- (void)renameTorrent:(NSString*)newName completionHandler:(void (^)(BOOL didRename))completionHandler;
+- (void)renameTorrent:(NSString*)newName completionHandler:(std::function<void(bool)>)completionHandler;
 - (void)renameFileNode:(FileListNode*)node
               withName:(NSString*)newName
-     completionHandler:(void (^)(BOOL didRename))completionHandler;
+     completionHandler:(std::function<void(bool)>)completionHandler;
 
 @property(nonatomic, readonly) time_t eta;
 @property(nonatomic, readonly) CGFloat progress;

macosx/Torrent.mm

@@ -2,7 +2,9 @@
 // It may be used under the MIT (SPDX: MIT) license.
 // License text can be found in the licenses/ folder.
 
+#include <memory>
 #include <optional>
+#include <utility>
 #include <vector>
 
 #include <fmt/format.h>
@@ -50,33 +52,20 @@ static dispatch_queue_t timeMachineExcludeQueue;
 
 - (void)renameFinished:(BOOL)success
                  nodes:(NSArray<FileListNode*>*)nodes
-     completionHandler:(void (^)(BOOL))completionHandler
+     completionHandler:(std::function<void(bool)>)completionHandler
                oldPath:(NSString*)oldPath
                newName:(NSString*)newName;
 
+- (void)renamePath:(NSString*)oldPath
+             withName:(NSString*)newName
+                nodes:(NSArray<FileListNode*>*)nodes
+    completionHandler:(std::function<void(bool)>)completionHandler;
+
 @property(nonatomic, readonly) BOOL shouldShowEta;
 @property(nonatomic, readonly) NSString* etaString;
 
 @end
 
-void renameCallback(tr_torrent* /*torrent*/, char const* oldPathCharString, char const* newNameCharString, int error, void* contextInfo)
-{
-    @autoreleasepool
-    {
-        NSString* oldPath = @(oldPathCharString);
-        NSString* newName = @(newNameCharString);
-
-        dispatch_async(dispatch_get_main_queue(), ^{
-            NSDictionary* contextDict = (__bridge_transfer NSDictionary*)contextInfo;
-            Torrent* torrentObject = contextDict[@"Torrent"];
-            [torrentObject renameFinished:error == 0 nodes:contextDict[@"Nodes"]
-                        completionHandler:contextDict[@"CompletionHandler"]
-                                  oldPath:oldPath
-                                  newName:newName];
-        });
-    }
-}
-
 bool trashDataFile(char const* filename, void* /*user_data*/, tr_error* error)
 {
     if (filename == NULL)
@@ -854,28 +843,64 @@ bool trashDataFile(char const* filename, void* /*user_data*/, tr_error* error)
     }
 }
 
-- (void)renameTorrent:(NSString*)newName completionHandler:(void (^)(BOOL didRename))completionHandler
+- (void)renameTorrent:(NSString*)newName completionHandler:(std::function<void(bool)>)completionHandler
 {
     NSParameterAssert(newName != nil);
     NSParameterAssert(![newName isEqualToString:@""]);
 
-    NSDictionary* contextInfo = @{ @"Torrent" : self, @"CompletionHandler" : [completionHandler copy] };
+    NSString* oldPath = tr_strv_to_utf8_nsstring(tr_torrentName(self.fHandle));
-
+    [self renamePath:oldPath withName:newName nodes:nil completionHandler:std::move(completionHandler)];
-    tr_torrentRenamePath(self.fHandle, tr_torrentName(self.fHandle), newName.UTF8String, renameCallback, (__bridge_retained void*)(contextInfo));
 }
 
 - (void)renameFileNode:(FileListNode*)node
               withName:(NSString*)newName
-     completionHandler:(void (^)(BOOL didRename))completionHandler
+     completionHandler:(std::function<void(bool)>)completionHandler
 {
     NSParameterAssert(node.torrent == self);
     NSParameterAssert(newName != nil);
     NSParameterAssert(![newName isEqualToString:@""]);
 
-    NSDictionary* contextInfo = @{ @"Torrent" : self, @"Nodes" : @[ node ], @"CompletionHandler" : [completionHandler copy] };
-
     NSString* oldPath = [node.path stringByAppendingPathComponent:node.name];
-    tr_torrentRenamePath(self.fHandle, oldPath.UTF8String, newName.UTF8String, renameCallback, (__bridge_retained void*)(contextInfo));
+    [self renamePath:oldPath withName:newName nodes:@[ node ] completionHandler:std::move(completionHandler)];
+}
+
+- (void)renamePath:(NSString*)oldPath
+             withName:(NSString*)newName
+                nodes:(NSArray<FileListNode*>*)nodes
+    completionHandler:(std::function<void(bool)>)completionHandler
+{
+    struct RenameContext
+    {
+        __strong Torrent* torrent = nil;
+        __strong NSArray<FileListNode*>* nodes = nil;
+        __strong NSString* oldPath = nil;
+        __strong NSString* newName = nil;
+        std::function<void(bool)> completionHandler;
+    };
+
+    auto context = std::make_shared<RenameContext>();
+    context->torrent = self;
+    context->nodes = nodes;
+    context->oldPath = oldPath;
+    context->newName = newName;
+    context->completionHandler = std::move(completionHandler);
+
+    tr_torrentRenamePath(
+        self.fHandle,
+        oldPath.UTF8String,
+        newName.UTF8String,
+        [context](int error)
+        {
+            @autoreleasepool
+            {
+                dispatch_async(dispatch_get_main_queue(), ^{
+                    [context->torrent renameFinished:error == 0 nodes:context->nodes
+                                   completionHandler:context->completionHandler
+                                             oldPath:context->oldPath
+                                             newName:context->newName];
+                });
+            }
+        });
 }
 
 - (time_t)eta
@@ -2098,11 +2123,11 @@ bool trashDataFile(char const* filename, void* /*user_data*/, tr_error* error)
 
 - (void)renameFinished:(BOOL)success
                  nodes:(NSArray<FileListNode*>*)nodes
-     completionHandler:(void (^)(BOOL))completionHandler
+     completionHandler:(std::function<void(bool)>)completionHandler
                oldPath:(NSString*)oldPath
                newName:(NSString*)newName
 {
-    NSParameterAssert(completionHandler != nil);
+    NSParameterAssert(static_cast<bool>(completionHandler));
     NSParameterAssert(oldPath != nil);
     NSParameterAssert(newName != nil);
 

tests/libtransmission/rename-test.cc

@@ -111,20 +111,10 @@ protected:
 
     static int torrentRenameAndWait(tr_torrent* tor, std::string_view const oldpath, std::string_view const newname)
     {
-        auto const on_rename_done =
+        auto error = std::optional<int>{};
-            [](tr_torrent* /*tor*/, char const* /*oldpath*/, char const* /*newname*/, int error, void* user_data) noexcept
+        tr_torrentRenamePath(tor, oldpath, newname, [&error](int const err) { error = err; });
-        {
+        EXPECT_TRUE(waitFor([&error]() { return error.has_value(); }, MaxWaitMsec));
-            *static_cast<int*>(user_data) = error;
+        return error.value_or(ETIME);
-        };
-
-        int error = -1;
-        tr_torrentRenamePath(tor, oldpath, newname, on_rename_done, &error);
-        auto test = [&error]()
-        {
-            return error != -1;
-        };
-        EXPECT_TRUE(waitFor(test, MaxWaitMsec));
-        return error;
     }
 };