Opened 12 years ago

Closed 8 years ago

#365 closed patch (patch_rejected)

Making pidgin receive multiple files.

Reported by: tinloaf Owned by: rlaager
Milestone: Patches Needing Improvement Component: libpurple
Version: 2.0 Keywords: file transfer,oscar
Cc:

Description

What I've done consists of two parts: First, i added some support for receiving multiple files to the file transfer-part of libpurple (ft.c and ft.h), and then I implemented exactly this for OSCAR.

I guess it should be possible to implement this in many other prpls, too, but oscar is the only I really use.

What's still left to do are two things: first, I could improve the statistics while the files are beeing downloaded (file x out of y beeing downloaded, etc.), and second I could try to implement sending multiple files as well. Well, receiving works so far, so go get it. ;-)

To the files attached: The one ending in .diff is the actual patch, the other one an "annotated" version which should make it easier to verify the patch.

I tested this to work with icq 5.1, I guess 6.0 is not supported (as it is not by the already existing oscar prpl).

Have a nice day!

Lukas

Attachments (3)

purple_multifiles.diff (26.7 KB) - added by tinloaf 12 years ago.
The patch itself
purple_multifiles.diff-annotated (30.7 KB) - added by tinloaf 12 years ago.
Annotations to the patch
purple_multifiles_warnings.diff (715 bytes) - added by tinloaf 12 years ago.
A patch to the patch, eliminating some warnings. Please apply after applying the first one.

Download all attachments as: .zip

Change History (15)

Changed 12 years ago by tinloaf

The patch itself

Changed 12 years ago by tinloaf

Annotations to the patch

comment:1 Changed 12 years ago by datallah

  • Type changed from defect to patch

Changed 12 years ago by tinloaf

A patch to the patch, eliminating some warnings. Please apply after applying the first one.

comment:2 Changed 12 years ago by seanegan

  • Milestone set to 2.1.0

Targetting to 2.1.0, the first version that will add API

comment:3 Changed 12 years ago by sadrul

  • Owner set to MarkDoliner

comment:4 Changed 12 years ago by MarkDoliner

Regardless of anything else I don't think we can really accept this patch before 3.0.0 because it adds members to PurpleXfer?.

comment:5 Changed 12 years ago by MarkDoliner

  • Milestone changed from 2.2.0 to 3.0.0

comment:6 Changed 11 years ago by sadrul

  • Milestone 3.0.0 deleted

It looks like the patch adds support for only the oscar protocol. Does any other protocol support this at all?

Perhaps the fields added to PurpleXfer can be added to PeerConnection instead? That probably wouldn't require any API additions, and still allow this feature to be added. Thoughts?

comment:7 Changed 11 years ago by rekkanoryo

Yahoo supports the recipt and sending of multiple files in a batch, albeit in a sequential manner. Currently it's handled in yahoo as a bunch of individual transfers.

comment:8 Changed 11 years ago by sadrul

Ah, I see. In that case, I suppose the additional API would be useful. And I don't think adding new members to the PurpleXfer struct will require a major version bump (like the addition in PurpleConversation didn't).

On a side note, I believe a plugin could be written (perhaps even semi-trivially) to do sequential multi-file transfers using the file-transfer signals (that was one of the reasons for introducing the signals in the first place ;) ).

comment:9 Changed 10 years ago by rekkanoryo

  • Milestone set to 3.0.0
  • Owner changed from MarkDoliner to rlaager

Assigning this to Richard because he has other stuff slated for 3.0.0; fitting this into the master plan for 3.0.0 would be a good idea.

comment:10 Changed 10 years ago by rlaager

  • Milestone changed from 3.0.0 to Patches Needing Improvement

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. ;)

comment:11 Changed 9 years ago by Halan

what about this patch? ICQ file transfer is fixed now. Will this feature be added too eventually?

comment:12 Changed 8 years ago by rekkanoryo

  • Resolution set to patch_rejected
  • Status changed from new to closed

Resolving this patch as "patch_rejected" because the issues mentioned here over a year ago have not been addressed. Anyone who wants to pick this patch up and improve it is more than welcome to do so.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!