Opened 11 years ago

Last modified 6 years ago

#1531 closed Patch

Added file exist actions

Reported by: paolozambotti Owned by: Tim Kosse
Priority: normal Component: FileZilla Client
Keywords: Cc: paolozambotti, Tim Kosse
Component version: Operating system type:
Operating system version:

Description

Hi,

I've added a couple of options that I find sometime useful to the file exist actions.
The new options are:

  • overwrite if size is different
  • overwrite if size is different or source is newer

I also made some changes around the code in order to allow future new option easy to add; I changed all (all the ones I found) refernces to fie exist actions as integer in actions as "enum CFileExistsNotification::OverwriteAction"

I tested the changes compiling the code under mingw, they seem to work fine.

Cheers,
Paolo.

Attachments (3)

filezilla_fileexist_mods_rev2390.patch (39.4 KB) - added by paolozambotti 11 years ago.
Patch file more file exist actions based on rev. 2390
FileZilla_actions_int_to_enum_rev2413.patch (3.9 KB) - added by paolozambotti 11 years ago.
some more actions int to enum changes
filezilla_more_fileexist_actions_rev2413.patch (19.3 KB) - added by paolozambotti 11 years ago.
Two more file exist action patch on rev 2413

Download all attachments as: .zip

Change History (9)

Changed 11 years ago by paolozambotti

Patch file more file exist actions based on rev. 2390

comment:1 Changed 11 years ago by Tim Kosse

Thanks for your patch.

I have applied the part of the patch that changes the action from integers into the enum and also found a few more places where it was wrong. A reminder for future patches: Please split janitorial work into a separate patch, makes it easier to review your changes.

I cannot apply the second half yet, it needs to be improved first:

  1. You cannot do things like "&o - Overwrite if different size or source newer", not all platforms have keyboard mnemonics in dialogs, e.g. they just do not exist under OS X. I think we have to live with some options not having a mnemonic in radio button groups, especially since cursor keys allow changing the selection quickly.
  1. The case where both local and remote filesizes are unknown needs to be handled for overwriteSize. Your code incorrectly skips the transfer. This case should rarely (if ever) happen, but we cannot ignore it.

Please fix these two issues and attach another patch against current SVN trunk.

Changed 11 years ago by paolozambotti

some more actions int to enum changes

comment:2 Changed 11 years ago by paolozambotti

Thanks for your notices.

As you suggested I split the new patch in janitorial work and new fuctionalities.
First of all I attached a new patch (FileZilla_actions_int_to_enum_rev2413.patch) that fixes some more places where actions are still present as integer. This patch was included in my previous one so I don't know if you missed this changes or you didn't want to merge them but, in any case, I resubmit to you.
About the second part of the patch with the new actions (size and sizeornewer) a new patch will follow but before I need to ask you a couple or things.
I don't catch your point (1): why doing "&o - Overwrite if different size or source newer" is different from "Rena&me"? Ok, this is just to let my understand, no problem to leave new actions without mnemonics.
About point (2), you are right this case must to be handled. Just to be sure, unknown size is represented by pData->local/remoteFileSize as -1 value, isn't it?

Cheers,
Paolo.
File Added: FileZilla_actions_int_to_enum_rev2413.patch

comment:3 Changed 11 years ago by Tim Kosse

I did already commit janitorial part yesterday.

You cannot use "&o - Overwrite if different size
or source newer" because some platforms have no mnemonics. Users of those platforms would see the actual labels prefixed with a totally useless "o - ". If you use a mnemonic, it has to be part of the actual label, don't just add some random letters.

Yes, unknown size is -1

Changed 11 years ago by paolozambotti

Two more file exist action patch on rev 2413

comment:4 Changed 11 years ago by paolozambotti

Yes I know you already commit janitorial part but, after doing that (rev 2413), there are still a couple of files where file exist action are managed with integers and not with enum. The file FileZilla_actions_int_to_enum_rev2413.patch fixes also those files.

About mnemonics, ok I understand your point, thanks.
Anyway, I attaced the new patch (filezilla_more_fileexist_actions_rev2413.patch) with the two new option (size and sizeornewer) with the two fixes you suggest:

  • no mnemonics at all for the two new option (I prefer to not assign mnemonics at all in place of assign them in one window and not in the other cause a lack of letters)
  • both file of unknown size is now managed (overwritten)

Just one note filezilla_more_fileexist_actions_rev2413.patch is based on the source already patched with FileZilla_actions_int_to_enum_rev2413.patch

Cheers,
Paolo.
File Added: filezilla_more_fileexist_actions_rev2413.patch

comment:5 Changed 11 years ago by Tim Kosse

I've applied the 2nd enum patch with a few small changes. You can't cast an int from an untrusted source directly into an enum, the result is undefined (actually implementation-dependent).
In some implementations, an enum's size is just one byte. Both 1 and 257 would both be casted to "overwrite". In other implementations where an enum is bigger, the 257 would fall into the default case of the switch statement.

Next I'll be reviewing the third patch.

comment:6 Changed 11 years ago by Tim Kosse

I've applied the new actions. Fixed a few mistakes:

  • unknown size comparison incorrectly used != instead of ==
  • compile error due to CFileExistsNotification::CFileExistsNotification::overwriteSize
  • moved new actions to the end of the action enum, else they would have interfered with saved actions from previous versions of FileZilla
Note: See TracTickets for help on using tickets.