GNOME Bugzilla – Bug 351217
SlotProgress used in Gnome::Vfs::Transfer should return (g)int
Last modified: 2011-01-16 23:36:16 UTC
Currently the return value is bool with a TODO "find out what the return value is meant to be for". Well, it's in http://developer.gnome.org/doc/API/2.0/gnome-vfs-2.0/gnome-vfs-20-gnome-vfs-xfer.html#GnomeVFSXferProgressCallback . It is not enough to have true and false, although it is ok to return true if you don't want to specify any specific actions. OTOH, passing Gnome::Vfs::XFER_ERROR_MODE_QUERY and Gnome::Vfs::XFER_OVERWRITE_MODE_QUERY to the transfer functions means that you want to decide in the signal handler what to do when an error an overwrite operation (is about to) occur, and you do so by returning specific values of ErrorAction and OverwriteAction enums.
Created attachment 70838 [details] [review] this small patch changes the slot definition and updates the example in examples/transfer
Thanks for researching this. I guess that documentation didn't exist when we made this API, or we didn't find it. It's a nasty API - interpreting an int as different enums depending on some other state. Unfortunately, we can't change this now. It's declared stable API/ABI. Also unfortunately, SlotProgress is used in lots of places. I suppose we could deprecate the whole Transfer namespace and replace it with a new TransferEx namespace, or something with a less awkward name. Also, I find the text at http://developer.gnome.org/doc/API/2.0/gnome-vfs-2.0/gnome-vfs-20-gnome-vfs-xfer.html#GnomeVFSXferProgressCallback very unclear. We would need to document it properly. I think it is mentioning 4 cases: 1. If the return value is `FALSE' (0), the operation is interrupted immediately. 2. If the return value is != 0, and info->status is GNOME_VFS_XFER_PROGRESS_STATUS_OK, the transfer operation is resumed normally. 3. If the return value is != 0, and info->status is GNOME_VFS_XFER_PROGRESS_STATUS_VFSERROR, the return value is interpreted as a GnomeVFSXferErrorAction and operation is interrupted, continued or retried accordingly. (true == 1, so that would mean GNOME_VFS_XFER_ERROR_ACTION_RETRY.) 4. If the return value is != 0, and info->status is GNOME_VFS_XFER_PROGRESS_STATUS_OVERWRITE, the return value is interpreted as a GnomeVFSXferOverwriteAction (true == 1, so that would mean GNOME_VFS_XFER_OVERWRITE_ACTION_REPLACE.)
Created attachment 76997 [details] proposed fix, take 1 What's left is to update async-handle stuff, and add docs.
I removed all TODOs about "Does Default do anything useful?". It does, and it's correct - it's "default behavior, which is to do a straight one to one copy" (from the C docs). I'm not completely sure if it's ok to typedef Transfer2::ProgressInfo as Transfer::ProgressInfo, as there are no changes necessary over there. But to me it seems better then copying and pasting the same code. async-handle.* use SlotProgress, so that also has to be done. The same way as Transfer2 code? I haven't added the implementation of Transfer::remove(StringArrayHandle&...). ABI break or not? I decided not to add any docs from the C lib until we fully agree about how this should look like. I must say that it looks ugly this way, mostly the same code being duplicated, though I don't see any other way to get it fixed.
> I'm not completely sure if it's ok to typedef Transfer2::ProgressInfo as > Transfer::ProgressInfo, as there are no changes necessary over there. But to me > it seems better then copying and pasting the same code. That would still leave people to use the other stuff in the Transfer namespace that's in transfer-progress.h. So they'd be using Transfer2 sometimes and Transfer at other times. I'd prefer to totally deprecate the Transfer namespace. That would mean we must create a transfer-progress-2.[h|cc] file. > mostly the same code being duplicated You might want to reimplement Transfer stuff in terms of Transfer2 to, to reduce copy/pasting. And we can lose the existing Transfer example code. It should just use the non-deprecated Transfer2 API. "Transfer2" isn't an ideal name, but I can't think of a better name. > I haven't added the implementation of Transfer::remove(StringArrayHandle&...). > ABI break or not? Sorry, I don't know what you mean. If you can produce a complete patch, I will, of course, check it for ABI breaks.
Created attachment 77314 [details] patch and new files All the above + async*. I also added some docs, the rest is left for another time. Also, I plan to write an example for async operation(s), but again, I think it's not for now.
Excellent. But why do we need to create the Async2 namespace as well? Async and Aysnc2 seem to have the same API.
Sorry, ignore that. I was only looking at one file, and I forgot that more than one file implements the namespace.
I have committed this to HEAD, with only minor changes to use the @deprecated doxygen keyword. Thanks so much for the hard work. I'll release a tarball in a few days time, if everything seems OK.
For the record, I believe that this bug was about SlotProgress, rather than "SlotTransfer".