[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 #9 from alexxcons <alexxcons at xfce.org> ---
(In reply to Cyrille Pontvieux from comment #8)
> Created attachment 9772 [details]
> format-patch to apply on latest master
> 
> For rename, I used the already written code about appending " (copy X)" and
> its related translations.
> 
> So I keep it simple in this commit. And let's improve it further in another
> commit/issue.
> 
> Thanks you two for taking time to look at this.

Patch works great for me, thanks !

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.

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

> 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 ?


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 ?
It should be done in a separate patch (I can do so later on .. though if you
have time, feel free to start over)

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