To reproduce: fallocate -l 20G bigfile mkdir moveto touch moveto/bigfile In Thunar: 1) Cut the first "bigfile" 2) Paste into "moveto" 3) This folder already contains a file "bigfile", hit Replace Thunar starts to copy the file instead of moving it to the subdir. At the end of the operation, the source file is removed and the destination file is replaced with a copy of the first.
Thanks for reporting, I can reproduce the problem. Theoretically, if I get it correctly, g_file_move should move the file, when G_FILE_COPY_OVERWRITE is passed. Practically I dont know what happens :) .. will have a look at it when I have time.
The g_file_move documentation says that "If native move operations are supported then this is used, otherwise a copy + delete fallback is used". So it seems that in this case, the fallback is being used rather than a "native move operation". Thus this behaviour comes from g_file_move itself, perhaps (?) triggered by the size of the file. I think that the G_FILE_COPY_OVERWRITE flag just tells g_file_move to perform the move even if the destination file already exists, rather than telling g_file_move *how* to perform the operation (i.e. native move vs fallback).
Created attachment 8871 very incomplete demonstartor In "thunar_transfer_job_execute" Thunar just calls "g_file_move" without checking if the target already exists. If I add the G_FILE_COPY_OVERWRITE flag, the move is just performed, replacing any existing target file. Without overwrite flag "g_file_move" will return FALSE (failure) Only afterward "thunar_transfer_job_verify_destination" and inside "thunar_transfer_job_copy_node" and than "thunar_transfer_job_copy_file" are called .. where finally the dialog is launched with "thunar_job_ask_replace". (But thunar_transfer_job_copy_file does not know much about move operations, so the file will be just copied) The doc of "g_file_query_exists ()" tells that it is correct to try & fail, instead of doing pre-checks, which potentially will cause race conditions. (And I think which requires gvfs) The attached patch just demonstrates how a fix could look like. It spawns a "thunar_job_ask_replace" dialog and re-executes the move with G_FILE_COPY_OVERWRITE if "overwrite" was selected (warning, this patch is very incomplete !!) This should be called in a loop, possibly in a seperate method, since the dialog offers "retry".
Created attachment 8881 patch to allow native file move in overwrite with user confirmation I see now that my previous comment here was wrong: as alexxcons said, the fallback behaviour is coming from thunar itself, not g_file_move. My bad! Here is a patch implementing alexxcons's suggestion, by retrying with G_FILE_COPY_OVERWRITE after user confirmation via thunar_job_ask_replace. However, at least on my system there is no retry option in this dialog, so I have not put the new code in a loop. The patch also fixes some (possible) small memory leaks in the same function. Criticisms and suggestions for improvement are most welcome!
Thanks alot for working on a proper patch ! Your patch works great here .. I tested different cases, so far all worked fine. Well done ! > However, at least on my system there is no retry option in this dialog Same here, I just saw that THUNAR_JOB_RESPONSE_RETRY is considered in line 566, which uses the same dialog ... possibly the option is only shown on certain circumstances. Have to figure out what may trigger a return value of THUNAR_JOB_RESPONSE_RETRY .. or if it possibly it is a historic relict, which should be removed. Some formal things to change: - Please always use spaces instead of tabs (tabs are evil, since different editors use a different ammount of spaces to display them) - Please put the g_object_unref (info) fix into a separate commit. (Always good to have very granular commits & seperation of concern)
Created attachment 8898 patch to allow native file move in overwrite with user confirmation Apologies for the newbie errors! Here is a revised patch containing the same changes as the previous ones, but split into two commits and with spaces not tabs. I have had a look through the code and I don't think thunar_job_ask_replace can return THUNAR_JOB_RESPONSE_RETRY. Indeed, the direct "return" statements in thunar_job_ask_replace only use THUNAR_JOB_RESPONSE_CANCEL, _YES, and _NO. thunar_job_ask_replace does call thunar_job_real_ask_replace (indirectly, via the call to exo_job_emit on line 439 in thunar-job.c), which in turn calls thunar_job_real_ask (via the call to g_signal_emit on line 251), but these only return _CANCEL, _YES, _YES_ALL, _NO, _NO_ALL (and thunar_job_ask_replace converts _YES_ALL and _NO_ALL to _YES and _NO).
Ah, thanks for the hint, now I got it ! Think thunar_job_ask_skip is the only dialog which adds THUNAR_JOB_RESPONSE_RETRY ... so it seems to be in use. If you think it is ready, I would push your patch to master and the 4.14 branch. To me it looks good now !
Yes, I think it's ready if you do
Ah, sorry, wait, think there is still something to do. Reading the code, If the code path of the "fallback copy and delete" should be reached, than a second "thunar_job_ask_replace" dialog will be spawned for the same file ... I guess we should prevent that in some way. So far I did not find a way to test that code path .. tried via triggering G_IO_ERROR_NO_SPACE on my full flashdrive, but without success ... any ideas ? Though I am not sure if we need that fallback at all. Currently we are just using the fallback on all other G_IO_ERROR errors. Instead it would be good to check on which G_IO_ERROR the fallback makes sense (If at all). Nautilus in addition to G_IO_EXISTS checks for G_IO_WOULD_RECURSE / G_IO_WOULD_MERGE and G_IO_CANCELLED: https://gitlab.gnome.org/GNOME/nautilus/blob/master/src/nautilus-file-operations.c#L5311 On any other error, it cancels the operation and just shows the error. On G_IO_WOULD_RECURSE / G_IO_WOULD_MERGE nautilus does as well remove the target and copy (or move?) ... possibly that would be the right trigger for the fallback copy ? I guess we should as well do something meaningfull on G_IO_CANCELLED.
Yes, you're right, the fallback code can be reached after the user has chosen to overwrite, leading to the thunar_job_ask_replace dialog appearing twice. To trigger this, you can copy one file onto another file with the same name on a different filesystem. Well spotted, my bad! Yes, I agree that it would be best to use only invoke the fallback when it makes sense, and I think G_IO_WOULD_RECURSE and _WOULD_MERGE would indeed be such cases. One case we certainly will need the fallback code is when g_file_move cannot perform a native move, for example when replacing a file with another file from a different filesystem. The simplest way I can think of to ensure that the user does not get asked twice about replacing the same file would be to add a member "replace_confirmed" to the ThunarTransferNode struct to indicate that the user has already said to replace the file and modifying thunar_transfer_job_copy_file to take an extra "don't ask for confirmation" parameter. Another thought that occurs to me is to wonder what exactly the thunar copy+delete code has that we can't get from just using the g_file_move internal copy+delete fallback, since using that would simplify the code a lot. After a *very* quick read of the code, it seems that the only extra thing the thunar code does is ask the user if they want to retry deleting the source file if this fails (although I could very easily have missed something else!). If that is all, them perhaps one could simply use the g_file_move copy+delete and just check in a loop if the source file is still there and ask if the user wants to remove it? Hmm, this might be more complicated than I thought. I will go and have a careful read through the code when I have a moment and see what I can come up with.
Hmm, for some reason I fail to reproduce the double-dialog .. which filesystems do you use to trigger it ? From https://developer.gnome.org/gio/stable/GFile.html#g-file-move > The native implementation may support moving directories (for instance on moves inside the same filesystem), but the fallback code does not. I am not sure if that means that the fallback code is not able to copy+delete directories, or if it is not able to do so across different file-systems. In any case, it sounds like that this might be the reason for our custom fallback, which is called recursively on directories. "thunar_transfer_job_copy_file" automatically creates "myfile (copy1)" if source and target are identical ... but for moving files that seems to be not relevant. (Though iirc there is some other open bug requesting a "rename" button on copy/move dialog ) A member "replace_confirmed" per file and "don't ask for confirmation" sound good to me!
So to get the dialog to appear twice my setup is as follows: I have two ext4 partitions on my disk, with a directory that I own on each (specifically, one partition is my root file system and the directory I'm using there is /var/www/html, the other partition is mounted on /home and the directory I'm using there is just a subdirectory of my home directory). In both directories I create an empty file called testfile, then I cut the testfile from one directory (in particular, from the directory under my home directory) and paste in into the other directory. When I do this, I get two confirmation dialogs one after the other. Yes, I see the value of the thunar copy+delete fallback now. I'll make a patch using the extra member and extra parameter method. I feel that the code in thunar-transfer-job.c could be refactored somewhat to reduce the complexity, but I think that would take rather more experience than I have at the moment, so maybe that's an idea for the future. Thanks!
Created attachment 8954 patch to allow native file move in overwrite with user confirmation Latest version of the patch. The patch contains two commits as before, with the first containing a few new lines to prevent memory leaks. The second commit, like the previous version, allows the use of native operations when overwriting a file by retrying (after asking for user confirmation) with the G_FILE_COPY_OVERWRITE flag added if the first g_file_move fails with error G_IO_ERROR_EXISTS. This version differs from the previous one in that it adds an extra member to ThunarTransferNode to record whether the user has confirmed file replacement (to prevent the user being asked twice), and also in that it only invokes thunar's fallback copy+delete for a small set of possible G_IO_ERROR_* errors where it makes sense to do so (taking the behaviour of nautilus as a guide). Criticisms and suggestions for improvement are welcome!
Thanks for the patch ! it goes into the right direction .. though if I am not mistaken, there is still some detail missing: Currently only "replace_confirmed", aka THUNAR_JOB_RESPONSE_YES is stored. What happens if THUNAR_JOB_RESPONSE_NO, aka "skip file" is selected ? Will that still spawn an extra dialog ? Could it be that we miss a "if (response == THUNAR_JOB_RESPONSE_YES)" which removes the node from the list when "skip" is pressed, so that the node is not considered by the fallback ? ( Or would it be better to store a variable of the type THUNAR_JOB_RESPONSE instead of the boolean replace_confirmed? For some reason I do not understand so far, I still fail to reproduce the "copy and delete" fallback, even if I simplify the "else if " which leads there ... however I am ok with relying on your testing. > I feel that the code in thunar-transfer-job.c could be refactored somewhat to reduce the complexity, > but I think that would take rather more experience than I have at the moment, so maybe that's an > idea for the future. That would be very welcome ! Looking forward to refactoring patches !! :)
Thanks for the comments! Yes, only "replace confirmed" is stored, but that is all that is needed (I think), since if the user responds THUNAR_JOB_RESPONSE_NO then the node in question is removed from the transfer job and is not passed on to the fallback code (there's a comment about this on lines 1029 to 1031), so no second dialog appears. In more detail, if the "retry" block on lines 1002 to 1033 is entered, then err is reset to NULL and the call to thunar_job_ask_replace will return one of THUNAR_JOB_RESPONSE_YES, _NO, or _CANCEL. >> If _YES, then the g_file_move is retried and move_successful and err are updated to reflect the outcome of this call. >> If _CANCEL, then we free all of the nodes in the job, leave err as NULL and break out of the loop, which results in the call to thunar_transfer_job_execute returning without error (as there was no error, the user just opted to cancel). >> If _NO, then we do nothing, leaving err as NULL and move_successful as FALSE. Thus we enter the block on lines 1036 to 1055 and free the node associated to this file (so that it is not passed on to the copy+delete fallback code), then continue with the loop to deal with any further transfer operations in the job. I hope that makes sense!
[ EDIT: I'm reposting this comment because I see that starting a line with > makes bugzilla think it's a quote and so the formatting in the previous comment was messed up. Apologies!] Thanks for the comments! Yes, only "replace confirmed" is stored, but that is all that is needed (I think), since if the user responds THUNAR_JOB_RESPONSE_NO then the node in question is removed from the transfer job and is not passed on to the fallback code (there's a comment about this on lines 1029 to 1031), so no second dialog appears. In more detail, if the "retry" block on lines 1002 to 1033 is entered, then err is reset to NULL and the call to thunar_job_ask_replace will return one of THUNAR_JOB_RESPONSE_YES, _NO, or _CANCEL. ** If _YES, then the g_file_move is retried and move_successful and err are updated to reflect the outcome of this call. ** If _CANCEL, then we free all of the nodes in the job, leave err as NULL and break out of the loop, which results in the call to thunar_transfer_job_execute returning without error (as there was no error, the user just opted to cancel). ** If _NO, then we do nothing, leaving err as NULL and move_successful as FALSE. Thus we enter the block on lines 1036 to 1055 and free the node associated to this file (so that it is not passed on to the copy+delete fallback code), then continue with the loop to deal with any further transfer operations in the job. I hope that makes sense!
> (there's a comment about this on lines 1029 to 1031) Hah, sorry, I should get into the habit of reading comments. Thanks for the detailed guidance ! Now I see how that works, it all makes sense to me now :) Will push the fix when I have time !
Reuben Green referenced this bugreport in commit 89e4b9506f757537893ec3c579c14d06ab44415f Prevent unnecessary fallback copy-delete in file move when overwriting https://git.xfce.org/xfce/thunar/commit?id=89e4b9506f757537893ec3c579c14d06ab44415f
Reuben Green referenced this bugreport in commit db49aa728b029ca1c398d0d8bee9a41db0c399ce Prevent unnecessary fallback copy-delete in file move when overwriting https://git.xfce.org/xfce/thunar/commit?id=db49aa728b029ca1c398d0d8bee9a41db0c399ce
Pushed to master and 4.14 branch. Thanks alot for your contribution !