[Xfce-bugs] [Bug 16686] Option to rename a file when existing copy conflicts

[bugzilla-daemon at xfce.org]

https://bugzilla.xfce.org/show_bug.cgi?id=16686

--- Comment #10 from Cyrille Pontvieux <jrd at enialis.net> ---
> Reading the code:
> - Possibly g_object_unref (renamed_file); is missing in
> thunar_transfer_job_copy_file (like for duplicate_file).
> I think you can use the scope of the loop for the variable, like for
> duplicate_file.
> ... at least something is fishy there. I dont understand that part: 
> >              if (err == NULL)
> >                target_file = renamed_file;
> Overwriting an argument usually is not what you want to have ... besides
> that, target_file is not used any more later in that method.
Well actually, a g_object_unref is required, but on target_file. I replace
target_file here, because it is use later on the loop for the next attempt to
copy the file. I know that replacing an argument is not usually a good idea,
but I'm not sure how to do it without heavily modify the function.

> - You can set  "n_rename = 0;" directly on variable declaration, no need for
> an extra line
Yes thanks

> > thunar-transfer-job.c:1048  "while (try_again)"
> Why you need to loop here, wheras before there was no loop. What would be a
> use-case which uses that additional loop ?
I tried not to modify too much this function. So when the new file name is
defined, the move function have already been tried and I didn't want to copy
the code (I like the DRY, Don't Repeat Yourself, approach). But maybe here was
not the best way to do it. And if you don't immediately understand the code,
that means it's a bad-written code. I will propose a easier solution.

> There is one thing bothering me when reading the code, not directly on your
> patch, but related:
> > THUNAR_JOB_RESPONSE_YES / THUNAR_JOB_RESPONSE_YES_ALL
> 
> As far as I can see, it would make much more sense to have
> THUNAR_JOB_RESPONSE_REPLACE / THUNAR_JOB_RESPONSE_REPLACE_ALL for our job in
> order to reflect what the buttons do ... that would make the code much
> simpler to understand.
> Since THUNAR_JOB_RESPONSE_YES is as well used in "thunar_job_ask_create", I
> would add addtional enum-values instead of renaming of existing enum-values.
> 
> Same for THUNAR_JOB_RESPONSE_NO / THUNAR_JOB_RESPONSE_NO_ALL which IMO
> should be THUNAR_JOB_RESPONSE_SKIP / THUNAR_JOB_RESPONSE_SKIP_ALL
> You think such a rename would be reasonable ?
Yes, I think it will clarify the code

> It should be done in a separate patch (I can do so later on .. though if you
> have time, feel free to start over)
I'll try, so I will propose two patches, one patch with improved corrections
and one patch for the response enum additions.

I will propably try to use xfce gitlab where I already cloned the thunar repo
instead of using bugzilla attachements if it's ok for you.

-- 
You are receiving this mail because:
You are the assignee for the bug.