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

Use NSURLSession to download files on iOS and update error returned on Android #15

Merged
merged 1 commit into from
May 24, 2016

Conversation

jrouault
Copy link
Member

@jrouault jrouault commented May 20, 2016

Major

  • downloadFile now uses NSURLSession API to download files on iOS. If the HTTP request fails, it returns the HTTP code. Otherwise we return the same success answer. (iOS)
  • downloadFile errors returned have been updated on Android to match iOS behaviour (Android)

poke @Tatsujinichi and @ronkorving

@ronkorving
Copy link

Wonderful! I'll take a more thorough look at the code in a sec, but... docs? :)


// Run async download task
File dir = file.getParentFile();
if (dir != null && !(dir.mkdirs() || dir.isDirectory())) {

Choose a reason for hiding this comment

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

What if dir is null? Can getParentFile even return that?

Copy link
Member Author

Choose a reason for hiding this comment

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

getParentFile can return null, I'll change that.

@@ -256,10 +256,10 @@ - (void)deleteFiles:(CDVInvokedUrlCommand *)command {

- (void)backgroundDelete:(CDVInvokedUrlCommand *)command {
@autoreleasepool {
NSMutableArray *uris = [[NSMutableArray alloc] initWithArray:command.arguments copyItems:YES];
NSArray *uris = command.arguments;

Choose a reason for hiding this comment

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

I guess my assertion was correct that an NSArray would do just fine (compared to a resizable array). I didn't say anything about not copying the items though, but did you see an opportunity here that I missed? My only concern would be some weird multi threaded case (that you would be more suited to identify than me here), where not having a copy would create corrupted reads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this was legacy code of deleting files when it was not yet done in the background. There should not be any weird multi threaded cases here.

@ronkorving
Copy link

Docs on errors would still be great :)

@jrouault
Copy link
Member Author

Ah right!

@jrouault jrouault force-pushed the task/NSURLSession branch 2 times, most recently from 101319a to 5647589 Compare May 23, 2016 04:31
@jrouault
Copy link
Member Author

Updated

| 5 | `HTTP_REQUEST_CONTENT_ERROR` | Content received in HTTP request could not be read |
| 6 | `DIRECTORY_CREATION_ERROR` | Creation of the directory to save the asset failed |
| 7 | `FILE_CREATION_ERROR` | Saving the asset failed |
| 8 | `JSON_CREATION_ERROR` | Your call to WizAssets' method failed and its error could not be processed internally |

Choose a reason for hiding this comment

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

Hmm, I think from a JS point of view, getting that code as a string would be really useful. That's imho the more common paradigm in JS.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, I use the same behaviour as the one on official Cordova plugins such as the File plugin. I added a quick explanation and an example how to have access to these constants.

@ronkorving
Copy link

LGTM 👍

@jrouault jrouault force-pushed the task/NSURLSession branch from cbeaffa to e2ef1c0 Compare May 23, 2016 08:51
@jrouault jrouault merged commit 913657f into Wizcorp:master May 24, 2016
@jrouault jrouault deleted the task/NSURLSession branch July 27, 2017 07:58
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