Add Progress Callback

[aried3r]

This is a work in progress. Implements #28.

For now, it only works for uploading, not for downloading. I'll add the same functionality for it later, but first I'd like to have some feedback, there's a few things I'd love your input on.

• It only works for File objects, not Strings or others.
• copy() is not only called when uploading something, but also when getting the request body after the request using body(). This results in output like below. How would you suggest to work around this? I'm a bit reluctant to add a boolean flag to the copy() call.

...
491520 out of 492672 written!
492672 out of 492672 written! // end of file transfer
123 out of 492672 written! // body() called

• Naming and placement inside the class is ok?

[aried3r]

Maybe this should be initialised at the same place where totalSize is initialised. In case people upload multiple files using part(), totalSize would grow, but totalWritten would be reset all the time.

[kevinsawicki]

Initializing this where totalSize is seems like a good idea.

[aried3r]

Friendly ping.

[kevinsawicki]

Thanks for the ping, sorry for the delay, will take a look soon.

[kevinsawicki]

Sorry again for the delay in reviewing this. Looks good and I'm down for merging it once we have a unit test for it.

[aried3r]

I'll add the same code for downloading and a test for both in the coming days (will probably a week anyway).

In the meantime, how (if at all) should we address the problem with the possible body() calls after uploading? See sample progress in the initial comment.

Also what about the line note I added? Should I address this issue or should I leave it as a known "limitation".

[kevinsawicki]

Instead of checking null every time how about instead we just have a NULL_PROGRESS_CALLBACK no-op implentation that is private static final and is assigned to progress when progress() is called with null callback?

[aried3r]

I now modified the second copy() method. The difference is, that it returns -1 for total, because we don't know the length beforehand. I also added some tests that document this behaviour.

The only thing I'm not sure of is whether -1 is a good idea, since for the other copy() method, total can sometimes be 0. What's your opinion on this? Personally I'd rather not have 0 and -1 indicate "no length available".

[kevinsawicki]

If callback is null it should set progress back to NULL_PROGRESS_CALLBACK or else you won't be able to clear a previously registered callback.

[aried3r]

Good point.

[aried3r]

Will squash when you tell me everything is ready and good to go so until then it's easier for you to review my changes.

[kevinsawicki]

Yeah, I feel like the progress callback should never report 0 for the total size or written as that isn't actually progress since nothing was actually transferred.

So how about just not calling the callback at all if nothing is actually written in either case?

[aried3r]

Well, it might be correct output for the written bytes if we send something empty, no?

The problem with the total size being 0 in some cases is that the first copy() function is used by many more functions than just the two where I can actually get a size (send(final File input) and part(final String name, final String filename, final String contentType, final File part)).

[kevinsawicki]

For the body() case, how about just calling progress(null) from closeOutput() since at that moment no more progress should be reported anyway since no more output will be sent.

[aried3r]

That would work for methods like code() which close the output, but body() does not close the output, it just calls copy() and returns the body of the response as a string. So something like request.post(url).progress(callback).send(file).body() would still have the problem.

That said, maybe body() should call closeOutput()?

[kevinsawicki]

body() calls byteStream() which calls contentLength() which calls intHeader which calls closeOutputQuietly() which calls closeOutput().

[aried3r]

Heh, I completely missed the byteStream() call. I'll apply the changes.

[patrick91]

I want this into http request! Is there any ETA?

[kevinsawicki]

@aried3r is this ready?

If it is I'll merge it in and cut a new release, also if you get a chance to add an example to the README.md that would be awesome, or we can do it after merging.

[aried3r]

Sorry, not yet. I want to add further tests for more likely cases. IIRC, there can still be -1 or 0 as the total size (see code and discussion above). Any wishes for that? Hope I can finish it this week, been just too busy, but it's only so little code, gotta cram it in somehow. ;)

[kevinsawicki]

@aried3r Yeah, I think -1 is fine for total length when it is unknown.

[kevinsawicki]

I merged it in with a few tweaks:

• Renamed callback to UploadProgress since it only does upload progress for now (we can add a second interface to do download progress later).
• Rebased onto latest master.

Thanks again for your work on this, let me know if you have any feedback on my tweaks.

[kevinsawicki]

May a few subsequent changes after thinking about it a bit more:

• Use long instead of int for callback since transfer size can get big.
• Use -1 as total size when unknown.

[kevinsawicki]

Would appreciate any feedback you have if something feels weird.

[kevinsawicki]

Just pushed a 5.6 version with this in it to maven central.

[bakua]

Hello, is there any need for calling progress(null); in closeOutput() method? Because if you don't then onUpload can be used also for download, only with that you don't get total size, which is ok I suppose.