Skip to content
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

Added progress callback for uploadFile #466

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brunolau
Copy link

@brunolau brunolau commented Apr 6, 2022

This adds onProgress callback for uploadFile method. Useful when uploading larger files

This commits adds onProgress callback for uploadFile method. Useful when uploading larger files
@@ -60,6 +65,23 @@ protected void sendBody(HttpRequest request) throws Exception {

request.part(uploadName, fileName, mimeType, inputStream);
}

if (hasProgressHandler) {
request.progress((transferred, total) -> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use Java 8 features (lambda expression in this case), because this will break support for old Cordova projects still configured to run with Java 7.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the lambda, please check if it's ok now

@silkimen
Copy link
Owner

Thank you for providing this feature PR, but please do not use Java 8 features as we are still supporting old Cordova projects running on Java 7.

@brunolau
Copy link
Author

Hi, lambda has been removed

@@ -180,7 +180,14 @@ module.exports = function init(exec, cookieHandler, urlUtil, helpers, globalConf
break;
case 'upload':
var fileOptions = helpers.checkUploadFileOptions(options.filePath, options.name);
exec(onSuccess, onFail, 'CordovaHttpPlugin', 'uploadFiles', [url, headers, fileOptions.filePaths, fileOptions.names, options.connectTimeout, options.readTimeout, options.followRedirect, options.responseType, reqId]);
var hasProgressCallback = options.onProgress != null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to check for falsey value here not for null, because it will be undefined if not provided.

Suggested change
var hasProgressCallback = options.onProgress != null;
var hasProgressCallback = !!options.onProgress;

Comment on lines +185 to +189
if (resp != null && resp.isProgress) {
options.onProgress(resp);
} else {
onSuccess(resp);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract this inline handler into a function expression in lines 183ff for making it more readable.

@@ -223,8 +230,8 @@ module.exports = function init(exec, cookieHandler, urlUtil, helpers, globalConf
return publicInterface.sendRequest(url, { method: 'options', params: params, headers: headers }, success, failure);
};

function uploadFile(url, params, headers, filePath, name, success, failure) {
return publicInterface.sendRequest(url, { method: 'upload', params: params, headers: headers, filePath: filePath, name: name }, success, failure);
function uploadFile(url, params, headers, filePath, name, success, failure, progress) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work for the https://github.com/danielsogl/awesome-cordova-plugins/ wrapper? I think it could be a problem but not sure, because usually the last two arguments are meant to be success and fail handlers. Would you please check?
If it causes problems we can keep the short-hand function uploadFile and make this functionality available only via sendRequest.

@silkimen
Copy link
Owner

silkimen commented May 25, 2023

Hi @brunolau, been quite some time but I checked your PR again just now. Left some comments. And also I recognized you could simplify the implementation by not checking if a progress function was supplied in native code, but instead always report the progress to the webview. And the www interface file can check if a progress callback was provided. I think this will be minimal overhead, right? What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants