[Pidgin] #365: Making pidgin receive multiple files.

[Pidgin]

#365: Making pidgin receive multiple files.
-----------------------------------------+----------------------------------
 Reporter:  tinloaf                      |        Owner:  rlaager  
     Type:  patch                        |       Status:  new      
Milestone:  Patches Needing Improvement  |    Component:  libpurple
  Version:  2.0                          |   Resolution:           
 Keywords:  file transfer,oscar          |  
-----------------------------------------+----------------------------------
Changes (by rlaager):

  * milestone:  3.0.0 => Patches Needing Improvement


Comment:

 tinloaf: Thank you for the patch. I'm very sorry we've been unable to
 carefully review this patch for so long.

 I have a few concerns about the API additions:

 In general, I'm not a fan of a bunch of functions with _multi in their
 names. I'd like to see how many of these we can collapse into regular
 functions that operate appropriate on single-file or multi-file transfers.

 I don't see any reason to have a separate is_multi boolean (and
 functions). Why not just set multi_total_files (which I'd rather see
 called total_files) to 1 in purple_xfer_new() to default it to a single
 transfer. Then everywhere that you care about multiple file transfers, you
 could just check the total files count. After those changes, you may also
 be able to eliminate purple_xfer_multi_file_completed().

 If you went that route, you could remove the _multi from
 purple_xfer_get_multi_files_left(). I suppose you'd have to initialize the
 files_left to 1 as well.

 You have a comment about the local_filename not being used, so you fill it
 with bogus data. Why not just let it get filled with the directory? That
 would, for example, allow UIs to open the containing directory in the case
 of multi-transfers. It should also allow you to eliminate all multi_dir
 and its getter/setter API.

 Can you explain the multi_bytes_sent stuff a little more? Is the basic
 idea that you want to track progress based on the full amount but you need
 the per-file amounts for the existing code that actually writes each file
 out? This is a more radical change, but would it make any sense to have a
 single transfer that represents the set of files and then have it have a
 list of children that are transfers for each individual file? In other
 words, something like this:
 {{{
 struct _PurpleXfer {
     ...existing fields...
     PurpleXfer parent;
     GList *children;
 }}}
 Then you'd have the size functions chain up. For example, when you
 increment the bytes transferred for a file in
 purple_xfer_set_bytes_sent(), it would do "if (xfer->parent)
 purple_xfer_set_bytes_sent(xfer, sent);" so the parent would count up
 those bytes as well. This would allow the UI to represent the file
 transfers as they really are... a tree. It could show the multi-transfer
 with an expander and show the individual files under it. The multi-
 transfer would then basically be a transfer with no actual data, but its
 progress would march towards completion as the individual files did.

 What does this comment mean: "// if one would try to implement sending
 multiple files with the same system, here has to be tested how to open the
 file: read or write?"

 A couple of minor things:

 In get_unused_filename(), you allocate your struct stat dynamically. Why?
 You should be able to do this: "struct stat stat; ... g_stat(teststring,
 &stat);"

 You add an if statement and a bunch of code around this block:
 {{{
         else if ((purple_xfer_get_type(xfer) == PURPLE_XFER_SEND) &&
                          S_ISDIR(st.st_mode)) {
                 ... code to bail on non-regular files ...
         }
         else {
                 purple_xfer_request_accepted(xfer, filename);
         }
 }}}

 As I was writing this, I realize that the S_ISDIR() check should probably
 be replaced by !S_ISREG() instead. That change is reflected in the
 suggestion below. The suggestion below does NOT take into account the idea
 of dropping the is_multi boolean in favor of just using the files count,
 as discussed above.

 How about this instead:
 {{{
         else if ((purple_xfer_get_type(xfer) == PURPLE_XFER_SEND) &&
                  !(S_ISREG(st.st_mode) || (S_ISDIR(st.st_mode) &&
 xfer->is_multi))) {
                 ... code to bail on non-regular files ...
         }
         else {
                 purple_xfer_request_accepted(xfer, filename);
         }
 }}}
 There's a "free(buf);" line in the patch. That should be "g_free(buf);"
 instead.

 Also, you were concerned about whether frame->name possibly not being NUL-
 terminated. If that's a problem, shouldn't you do "g_strndup(frame->name,
 frame->name_length + 1);" (note the + 1) instead and then do
 "filename[frame->name_length] = '\0';"?

 The patch adds #include "debug.h" to oft.c, but doesn't seem to use it
 anywhere that I can see.

 We don't use // comments, so those need to be fixed, but that's even
 trivial by trivial standards. ;)

-- 
Ticket URL: <http://developer.pidgin.im/ticket/365#comment:10>
Pidgin <http://pidgin.im>
Pidgin