-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upload progress recipe #8493
base: master
Are you sure you want to change the base?
Upload progress recipe #8493
Conversation
cc @swankjesse @LouisCAD in case you have thoughts, otherwise I'll review next week |
@yschimke If you officially provide the recipe, it will be very helpful to new users |
|
||
@Override | ||
public void writeTo(BufferedSink sink) throws IOException { | ||
BufferedSink bufferSink = Okio.buffer(sink(sink)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supernit: bufferedSink, not bufferSink
public void writeTo(BufferedSink sink) throws IOException { | ||
BufferedSink bufferSink = Okio.buffer(sink(sink)); | ||
requestBody.writeTo(bufferSink); | ||
bufferSink.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this, the implementation of writeTo()
shouldn’t close sink
private static class ProgressRequestBody extends RequestBody { | ||
|
||
private final ProgressListener progressListener; | ||
private final RequestBody requestBody; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to delegate
?
@Override | ||
public void close() throws IOException { | ||
super.close(); | ||
progressListener.update(totalBytesWritten, contentLength(),true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supernit: formatting. Space after ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIGHT want to make this only call progressListener.update()
once even if close()
is called multiple times. You’re allowed to call close()
multiple times but listeners shouldn’t be called more than once
Dear. @swankjesse
I improved the
upload progress recipe
discussed in #1471 #1528It was rewritten based on the discussed requirements.
After discussion, I would like to write a Kotlin recipe
I'm awaiting your comments.
Thank you for your time